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)
Console.WriteLine("Please write a number:");
if(Int32.TryParse(Console.ReadLine(), out input))
{
numbers[i] = input;
sum += numbers[i];
error = false;
}
else
{
Console.WriteLine("An error occured, please try again:");
error = true;
i--;
continue;
}
}
Console.WriteLine("");
Console.WriteLine("Your typed in values:");
for(int i = 0; i < numbers.Length; i++)
{
Console.WriteLine(numbers[i]);
}
Console.WriteLine("");
Console.WriteLine("The result of your numbers:");
Console.WriteLine(sum);
Console.WriteLine("The average of your numbers:");
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
Please write a number:
> 2
Please write a number:
> a
An error occured, please try again:
> 6
Please write a number:
> 4
Please write a number:
> 8
Your typed in values:
4
2
6
4
8
The sum of your numbers:
24
The average of your numbers:
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: ");
Instead of:
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]: ");
string input = Console.ReadLine();
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 Linqs 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 {
// read input
// 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) {
Console.WriteLine("Please write a number:");
if(Int32.TryParse(Console.ReadLine(), out input))
{
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("The result of your numbers:");
Console.WriteLine(sum);
Console.WriteLine("The average of your numbers:");
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.