# Sum and average calculator

Posted on

Problem

I’ve just written some code in an online compiler, and since I’m very new to C#, I wanted to ask what could be improved about the code.

The task of the program is simply to add up the numbers the user entered before and then calculate the average.

``````public static void Main()

{
int input;
int sum = 0;
bool error = false;
int[] numbers = new int[5];

for(int i = 0; i < numbers.Length; i++)
{

if (!error)

{
numbers[i] = input;
sum += numbers[i];
error = false;
}
else
{
Console.WriteLine("An error occured, please try again:");
error = true;
i--;
continue;
}
}

Console.WriteLine("");
for(int i = 0; i < numbers.Length; i++)
{
Console.WriteLine(numbers[i]);
}

Console.WriteLine("");

Console.WriteLine(sum);

Console.WriteLine((double)sum / (double)numbers.Length);
}

``````

The code works fine, but maybe I could do some things more effectively.
Would be glad to get some help đź™‚

Just to sum things up here is the console-output:

``````Please write a number:
> 4
> 2
> a
An error occured, please try again:
> 6
> 4
> 8

4
2
6
4
8

24
4.8
``````

(‘>’ = User input)

Solution

The code works, and it seems that you’re take care of invalid input. But the logic of the workflow is not – well – that logic.

`int[] numbers = new int[5];`

Here `5` is a magic number. Why five? Why not four or six or 20?

You should announce the number of input before asking for it (and maybe why).

Alternatively you could let the user enter a number of numbers to be calculated.

``````int sum = 0;
``````

You probably have the `sum` variable for optimization reasons. Alternatively you could let the built in extension `Sum()` handle that:

``````numbers.Sum();
``````

`if (!error)`

You have this negation of a “negative” variable. Why not be positive and call it `success`:

``````if (success)
Console.Write("Enter Number: ");
``````

``````    if (Int32.TryParse(Console.ReadLine(), out input))
{
numbers[i] = input;
sum += numbers[i];
``````

you could write:

``````    success = Int32.TryParse(Console.ReadLine(), out input);

if (success)
{
...
}
else
{
...
}
``````

``````      continue;
``````

I don’t see any reason for calling `continue` in your loop? You use `continue` to step over the rest of the loop and reenter it – if the condition is still true, but here it is the last expression in the loop, so there is nothing to skip.

According to the overall design you should always split your code into meaningful methods in order to accommodate to principles like DRY and Single Responsibility:

For instance:

``````void Main()
{
if (!GetCountOfNumbers(out int numCount))
return;

if (!GetUserInput(numCount, out int[] numbers, out int sum))
return;

PrintValues(numbers);
Console.WriteLine("");
PrintResult(sum, numbers.Length);
Console.WriteLine("");
}
``````

Where:

``````private bool GetCountOfNumbers(out int numCount)
{
if (!GetIntInput("Enter count of Numbers", "Must be a positive number", x => x > 0, out numCount))
{
Console.WriteLine("User aborted the calculation");
return false;
}

return true;
}
``````

and

``````private bool GetUserInput(int numCount, out int[] numbers, out int sum)
{
numbers = new int[numCount];
sum = 0;
int input;

for (int i = 0; i < numCount; i++)
{
if (!GetIntInput(\$"Enter number {i + 1}", "Enter any valid integer (32 bit) value", x => true, out input))
{
Console.WriteLine("User aborted the calculation");
return false;
}

numbers[i] = input;
sum += input;
}

return true;
}
``````

Both methods are calling `GetIntInput()` which could be defined as follows:

``````private bool GetIntInput(string message, string errorMessage, Predicate<int> predicate, out int result)
{
result = default;

while (true)
{
Console.Write(\$"{message} ['q' to Exit]: ");
if (input == "q" || input == "Q")
return false;

if (int.TryParse(input, out int number) && predicate(number))
{
result = number;
return true;
}
Console.WriteLine(\$"Invalid input: {errorMessage}");
}
}
``````

It takes a message to the user, a predicate to validate the input against and an error message if the predicate returns false on the input, and it returns the result in an `out` variable (`result`). Further it allows the user to exit the input loop and then returns true or false accordingly.

In this design everything is counted for through descriptive method names and the logic in the workflow is easily understood and separately maintainable.

It could surely be done in more sophisticated ways, but as a simple user input driven console application like this, the above is a simple way to structure the code properly.

Henkrik’s answer is very good. However, there are some things I feel should be addressed as you are a beginner. Some of this answer will address things introduced in his answer.

First, `out` parameters should generally be avoided. They are used to force the method to initialize them before it returns; essentially, they are are saying “I have this value, but I need you to update it before you finish running so I can read the updated value.” Normally you would use a return type for that. In special cases (as in `int.TryParse`) you need two values, but those are rare, and are mainly for external-facing APIs when you don’t want to introduce and maintain a full type for two values to be used in one place. For internal methods, you should consider a tuple type with named members (introduced in C# 7). Having more than 1 `out` parameter typically is a sign that a more structured response (i.e. a type) is needed.

Henrik is right that you should pull your logic into multiple methods. However, each method should only perform one piece of work. One method, for example, could take input and return an `int[]`. Then you should have another function for performing the `Sum` and another the `Average` of these numbers. Then you can reuse your work. Performance will be slightly less due to calculating `Sum` twice, but until you hit the scale of millions of numbers, it won’t be a serious problem. As an example implementation (using `Linq`s `Sum` method):

``````private int Sum(int[] args)
{
return args.Sum();
}

private double Average(int[] args)
{
return Sum(args) / args.Length;
}
``````

You can define other methods, such as one to input a single number, one that calls that function several times to input a series of numbers, etc. Then we can call these methods all together and find the data we need.

As mentioned in a comment, I could have used Linq’s `Average` feature: `args.Average()`. However, I left it with the call to `Sum` to show how it’s good to make functions reusable.

`Console.WriteLine("");` doesn’t need the argument. If you want just a newline character fed into the output stream, you can do `Console.WriteLine()`.

If you want output without a trailing newline, you can use `Console.Write`.

In a `for` loop, you almost never want to step back (`i--`). I would have used a `for` loop to get each number, and a `while` loop within it to get a valid number and keep retrying after invalid numbers were entered. Because you need to run the code at least once, a `do/while` loop would be a good choice, since it clearly expresses that the loop will run one time before the condition is checked. Something like:

``````for (var i = 0; i < 5; i++)
{
var hasValidInput = false;
do {
// set hasValidInput
} while (!hasValidInput);
}
``````

What you did right:

You had error handling if the user didn’t input a number. That’s a common issue for new programmers.

In the loop, you’re trying to modify the iterator at 2 places. One here `for(int i = 0; i < numbers.Length; i++)` and the other inside the loop in the else block `i--;`. Reason I’m not inclined to do that is because not only `for` is maintaining the status of the iterator but the code as well. The question that needs to be asked is `for` as a looping construct suited for this problem. This is my take

``````while (i < 5) {
{
numbers[i++] = input;
sum += input;
}
else
{
Console.WriteLine("An error occured, please try again");
}
}
``````

Iâ€™m not a C# guy, but I can suggest that an error message should say something about the error. â€śErrorâ€ť says as much as â€śan error occurred.â€ť Since you tried to read an integer, perhaps â€śInteger only, please.â€ť

Speaking of which, since you are only accepting integers, perhaps the prompt should say â€śintegerâ€ť instead of â€śnumber.â€ť

## Consistency

You are evaluating the `sum` in a single pass by simply adding each `input` to the current sum. But you don’t do the same for the average.

``````if(Int32.TryParse(Console.ReadLine(), out input))
{
numbers[i] = input;
sum += numbers[i];
}

// ..

Console.WriteLine(sum);
Console.WriteLine((double)sum / (double)numbers.Length);
``````

As others have pointed out, you should just store the numbers in a single pass `numbers[i] = input;` and then evaluate the `sum` and `average` each in their own pass as:

``````var sum = numbers.Sum();
var average = numbers.Average();
``````

### Cumulative Moving Average

There is another way to be consistent, while keeping everything in a single pass. You’d have the build a cumulative moving average together with the sum:

``````if(Int32.TryParse(Console.ReadLine(), out input))
{
numbers[i] = input;
sum += numbers[i];
average += ((decimal)input - average) / (i + 1);
}
``````

I would use a decimal to mitigate rounding and truncation errors.