1
\$\begingroup\$

Problem : Imagine you are a developer and get a job in which you need to create a program for a teacher. He needs a program written in c# that calculates the average score of his students. So he wants to be able to enter each score individually and then get the final average score once he enters -1. So the tool should check if the entry is a number and should add that to the sum. Finally once he is done entering scores, the program should write onto the console what the average score is. The numbers entered should only be between 0 and 20. Make sure the program doesn't crash if the teacher enters an incorrect value. Test your program thoroughly.

My solution :

static void Main(string[] args)
{
    int digit = 0,sum=0,counter=0;
    string x;

    try
    {
        for (int i = 0; i <= counter; i++)
        {
            Console.WriteLine("Please Enter Score");
            x = Console.ReadLine();
            bool isParsable = Int32.TryParse(x,out digit);

            if (isParsable)
            {
                if (digit >= 0 && digit <= 20)
                {
                    Console.WriteLine("Valid Number");
                    sum += digit;
                    counter++;
                    Console.WriteLine($"Student number {counter} got {digit}");
                }
                else if (digit == -1)
                {
                    Console.WriteLine($"Total sum is {sum}");
                    Console.WriteLine($"Total number of students is {counter}");
                    Console.WriteLine($"Average score of {counter} students is{sum/counter}");
                    break;
                }
                else
                {
                    Console.WriteLine("Please enter a valid score");
                }
            }
            Console.WriteLine("Please enter Numerical Values only");
        }
    }
    catch (DivideByZeroException)
    {
         Console.WriteLine("Unable to get results");
    }
}
```
\$\endgroup\$
2
  • \$\begingroup\$ Hi there! One thing that makes your code hard to read are the nested if statements. I would recommend you check out guard clauses \$\endgroup\$
    – Moglash
    Mar 7, 2022 at 10:04
  • \$\begingroup\$ @DennisFink Thank you for your reply ! I am very new to coding,i just started learning C# 7 days ago ! I will look into what you said ! \$\endgroup\$ Mar 7, 2022 at 10:23

1 Answer 1

4
\$\begingroup\$

General comments on your code

  • "Unable to get results" is a very vague message that does not meaningfully inform the user. Make the message more meaningful, e.g. "No scores entered, cannot calculate average".
    • Note that in the next section I get into avoiding the division-by-zero in its entirety.
  • for (int i = 0; i <= counter; i++) is a very confusing way to have your application end after you've printed the results. It works (because printing the results does not increment the counter, making it so i now breaks the condition), but it's not very readable.
    • Overall, your code is neatly readable line by line, but it lacks a bit of abstraction which makes the flow of the overall application harder to read. I address this in the next section, step by step.
  • $"Student number {counter} got {digit}" is redundant. The user still sees the data they entered into the console, so there's no need repeating the same information on the line below it.
  • "Please enter a valid score" is needlessly vague. Tell the user which validation rule they failed to comply with. Otherwise it's just a guessing game.
  • isParsable is a technically correct name, but something like isNumeric conveys the meaning better. For readability's sake, avoid technical terms and stick to the meaning of the value rather than how you obtained it.
  • Pedantic niggle: digit is not the right name for your variable. A digit is to a number what a letter is to a word. The number 12 has two digits, but it represents a single numerical value. number would have been a better variable name. Or, keeping in line with the earlier advice, score would have been a contextually appropriate name here as that is what the value represents.
  • There's no need to declare string x; outside of the for loop. It's better to just keep it locally scoped. While the impact of doing so is tiny here, this is one of those readability straws that will eventually break a camel's back in a real life codebase.

Refactoring the code

You've written your code as a single method, which is impacting its readability. If you now need to change your flow (e.g. because of changing requirements), it requires you to pull it all apart to find the thing you need to change. That may seem simple when you've just written the code and know it by heart, but this will become significantly harder for other developers, or after you've not looked at this code for a while. While this is a short snippet; this gets more difficult for real-life-size codebases.

Separate your logic into specific behaviors that you need your application to have. This is just an overview of what you expect your application to do. This helps you with separating these concerns and developing them separately.

Without delving into the specific method implementations, this already gives you a really neat top level algorithm that makes it clear at a glance what to expect from your application:

static void Main(string[] args)
{
    var scores = GetScores();
    var average = CalculateAverage(scores);
    Print(average);
}

Some comments:

  • I prefer storing the numbers, not just the sum and counter. This is under the assumption of the current context: a classroom teacher will not have anywhere near an amount of scores to calculate that storing all the values will be problematic.
    • The benefit is that you can print the input values later on, which may be nice for UX purposes, e.g. to print a full report.
    • If you do need to conserve memory to that extent (if there is a concrete reason to do so), feel free to revert to the sum/counter storage mechanic (but beware of integer overflow if you're going to be dealing with such a large amount of values).

Now we can implement the missing methods, which helps us focus on keeping each method simple and bite-sized.

public const int ExitCode = -1;

public IEnumerable<int> GetScores()
{
    var scores = new List<int>();
    do {
        var score = GetScore();

        if(score == ExitCode)
            return scores;

        scores.Add(score);
    } while(true);
}

Some comments:

  • I used a const ExitCode to avoid magic numbers in code. This makes it clear that the value has a predetermined meaning.
  • I used while(true) because the return ensures that we exit the loop when the right exit condition is met. Generally avoid while(true), but in this case it simplifies the syntax and is therefore acceptable.
public const int ScoreMinValue = 0;
public const int ScoreMaxValue = 20;

public static int GetScore()
{
    do {
        Console.Write("Input a score: ");

        var input = Console.ReadLine();
        bool inputIsNumerical = Int32.TryParse(input, out number);

        if(!inputIsNumerical)
        {
            Console.WriteLine("Input must be numerical");
            continue;
        }
       
        if(number != ExitCode && (number < ScoreMinValue || score > ScoreMaxValue))
        {
            Console.WriteLine($"Score must range from {ScoreMinValue} to {ScoreMaxValue}");
            continue;
        }

        return number;
    } while(true);
}

Some comments:

  • Similar as before, I used a while(true) here because the continue and return keywords already handle the flow. Always be aware that you're not introducing any neverending loops when resorting to while(true).
  • I used inputIsNumerical as an intermediary step instead of putting if(!Int32.TryParse(input, out number)). This would also work, but the extra step increases readability.
  • The range limits have been made into constants to avoid magic numbers, promote reuse, and increase readability.
public double CalculateAverage(IEnumerable<int> values)
{
    // LINQ already has an averaging method
    
    if(!values.Any())
        return 0;

    return values.Average();

    // If you want to do it yourself:

    int counter = 0;
    double sum = 0;

    foreach(var value in values)
    {
        counter++;
        sum += value;
    }

    if(counter == 0)
        return 0;

    var average = sum / value;

    return average;
}

Some comments:

  • If you use LINQ, there's already an averaging method. No use reinventing the wheel. However, reinventing the wheel makes sense for educational purposes, so I also added a manual way of calculating the same thing.
  • I avoided int division by making sum a double. This bypasses the need for explicit casting later on.
  • Exceptions are useful at times, but better avoided when it is possible to handle things more elegantly. In this case, I find it better to simply return 0 when trying to average a list with no values.

I leave the implementation of the Print method to you. It's really just a matter of writing to the console what you want to write. Change the input parameters as you see fit, e.g. if you also want to print all the scores that were given.

\$\endgroup\$
3
  • \$\begingroup\$ Minor nitpicks: The SCORE_MINVALUE and SCORE_MAXVALUE should probably be public const int instead of static, and while the naming convention of C# is sparse, most people (as well as MS themselves) use pascal case for static members (and const as well) instead of shouting snake case, so: public const int ScoreMinValue = 0; and public const int ScoreMaxValue = 20; \$\endgroup\$
    – MindSwipe
    Mar 7, 2022 at 11:35
  • \$\begingroup\$ @MindSwipe Good catch, and fair point. \$\endgroup\$
    – Flater
    Mar 7, 2022 at 11:37
  • \$\begingroup\$ @Flater Thank you both of you ! Really learned a lot from those replies ! \$\endgroup\$ Mar 7, 2022 at 12:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.