1
\$\begingroup\$

I have been studying C#, and I have made a simple math quiz program.

Is my program reasonable or is there anything I need to improve?

class Program
{

    static void Main(string[] args)
    {
        Console.WriteLine("============== Math Game ===================");

        double num1, num2;
        double userInput;
        int randomIndex;
        double sum = 0;
        string operation;

        // [2]
        //int lives = 0;

        // Console.Write("How many lives do you want?\t");
        // lives = Convert.ToInt32(Console.ReadLine());
        // Console.WriteLine("You have  {0} Lives", lives);

        bool isPlaying = true;
        do
        {
            // 1 generate randon#
            Random rnd = new Random();
            randomIndex = rnd.Next(1, 4);
            num1 = rnd.Next(1, 11);
            num2 = rnd.Next(1, 11);

            // 2 cal sum && operation && Display
            sum = Cal(num1, num2, randomIndex);
            operation = Operation(randomIndex);

           // DisplayProblem(num1, num2, operation);

            // 3 request input
            userInput = RequestInput(num1,num2,operation);

            // 4 check answer
            CheckAnswer(userInput, sum);


        } while (isPlaying);

    }
    private static double Cal(double num1, double num2, int randomIndex)
    {
        double sum = 0;
        switch (randomIndex)
        {
            case 1:
                sum = num1 + num2;
                break;
            case 2:
                sum = num1 - num2;
                break;
            case 3:
                sum = num1 * num2;
                break;
            case 4:
                sum = num1 / num2;
                break;
            default:
                Console.WriteLine("Error");
                break;
        }
        return sum;
    }
    private static string Operation(int randomIndex)
    {
        string operation = "";

        switch (randomIndex)
        {
            case 1:
                operation = "+";
                break;
            case 2:
                operation = "-";
                break;
            case 3:
                operation = "*";
                break;
            case 4:
                operation = "/";
                break;
            default:
                Console.WriteLine("error");
                break;
        }
        return operation;
    }
    private static void DisplayProblem(double num1, double num2, string operation)
    {
        Console.Write($"{num1} {operation} {num2}  = ");
    }
    private static double RequestInput(double num1, double num2, string operation)
    {
        DisplayProblem(num1, num2, operation);

        double userInput;
        while (!Double.TryParse(Console.ReadLine(),out userInput))
        {
            Console.WriteLine("Invalid input. Please type a whole num");
            DisplayProblem(num1, num2, operation);
        }
        return userInput;
    }
    private static void CheckAnswer(double userInput, double sum)
    {
        if (userInput == sum)
        {
            Console.WriteLine("Correct Answer.");
        }
        else if(userInput != sum)
        {
            Console.WriteLine("Wrong Answer.");
        }
    }
}
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

The Road to Good Program Design and Code

Perhaps the worst coding horror I see day to day is absence of good practices when it comes to simple or short code. We all learn on simple examples so it feels like too much overkill effort. And we rationalize: "it's a throwaway", "it's too simple to bother", "I'm just experimenting."

Focus on good basics and don't worry so much about SOLID, patterns, etc. per se.

Never forget that you are writing for the reader and for code maintainability.


static void Main(string[] args)

Main should only drive your program, it should not be the program. This makes the class(es) portable, re-usable. Like this:

class Program {
    public MathQuiz testMe = new MathQuiz();
    testMe.Play();  // you'll need an entry point
} // Program

public class MathQuiz {
   // all the code
} // MathQuiz

Naming Things

The general starting place for method names is actionVerb-noun. And abbvr. is not bad per se but understanding must not be sacrificed. Cal - could be short for Calvin, or calendar, or California, or ????. So Calc is better but still not good. Calculate is better but not much. DoTheMath is catchy! What does the method do? That's its name.

private static double Cal(double num1, double num2, **int randomIndex**)

It is not a randomIndex, it's an arithmetic operator. Name things for what they are in the "problem domain", in this case a math quiz. I can read code but I can't understand it if all the variable names merely redundantly repeat implementation details.

Operation(int randomIndex) How about DecodeOperatorCode(int opCode). Just keep leaning toward descriptive naming and the code will be better off.


Comments

// 3 request input userInput = RequestInput(num1,num2,operation);

I can imagine sketching an outline using comments but at this point leaving these kinds of comments is just plain amateur hour. It is obviously superfluous. It adds no understanding. Good structure and good naming are worth a thousand comments.


do { ... } while(isPlaying)

I had to read ALL the code just to find out isPlaying never changes. Here a comment would be nice to let the reader know the loop does not terminate. Oh, did I mention I had to read ALL the code?


Pick Up Your Dead

    // [2]
    //int lives = 0;

    // Console.Write("How many lives do you want?\t");
    // lives = Convert.ToInt32(Console.ReadLine());
    // Console.WriteLine("You have  {0} Lives", lives);

Never leave dead code lying about. If you want to know what the previous code state is then use version control; which is what you should be doing anyway.

operator vs operation - naming precision

An operation is what is done, an operator is what it is. A plus sign is an operator. Adding things is an operation.


sum

A sum is the result of addition by definition but is used universally for the result of all operations (using the various operators). But it is used consistantly (see below) and that does mitigate potential misunderstanding.


string operation = "";

string.Empty is better. Sometimes the font and/or its size can make it hard to discern any blank space in there.


Consistency

The program uses both an operatorCode and the operator (+, -, /, *). Be consistant and use one or the other. "Translate" the opCode up front and use that throughout, or vice versa. But not both. In context using one or the other makes better sense perhaps but consistancy gives the code more of the readability and understandability qualities.


Classes are where you find them

And I find them anywhere I'm fussing with something and that code is spread around and/or duplicated and/or makes for "busy" code. The operator in this program can be a separate class. If you think this class is too small to bother with re-read the opening statement of this post.

public class MathOperators {
   protected Array operators = ["x", "/", "+", "-"];
   protected Random randomOpCode = new Random();

   public int OperatorToOpCode(string operator) { ... }
   public string OpCodeToOperator(int opCode) { ... }
   public int randomOpCode(){ ... }
}

- It's re-usable and reliable (assuming it's been tested!)
- The user doesn't know or care how the operators are stored or how to retrieve them
- Easier to test
- This class has a single, focused purpose
- Functionality encapsulation is a very good thing. You'll understand more deeply as you gain experience.
- Changes are a low risk for the using (client) code. All the client code cares about is the interface - that is, all the public class members' signatures. AKA, the API. 
- The Invisible Hand of good design and coding
   - Classes and methods tend to be smaller and simpler. 
   - Complexity stays under control as code is added.
\$\endgroup\$
1
  • \$\begingroup\$ thank you so much for valuable comments. I will keep those in mind and try to fix my program. \$\endgroup\$ Commented Oct 4, 2018 at 22:12
3
\$\begingroup\$

In the Operation method

private static string Operation(int randomIndex)
{
    string operation = "";
    switch (randomIndex)
    {
        case 1:
            operation = "+";
            break;
        case 2:
            operation = "-";
            break;
        case 3:
            operation = "*";
            break;
        case 4:
            operation = "/";
            break;
        default:
            Console.WriteLine("error");
            break;
    }
    return operation;
}

instead of doing operation = ... and then returning operation you can just return

Example :

case 1:
    return "+";

Same thing in the Cal method.

In the CheckAnswer method you should just have else instead of else if(userInput != sum)

Also in the Main your Random rnd = new Random(); should be out of the do while loop.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ I would go further and say that rnd should be a class field and not scoped to a local method. \$\endgroup\$
    – Rick Davin
    Commented Oct 4, 2018 at 15:03
  • \$\begingroup\$ yeah indeed you won't need several Randoms so that's relevant but is it any faster performance wise having it as a class field instead of a local variable? \$\endgroup\$
    – nalka
    Commented Oct 4, 2018 at 15:06
  • \$\begingroup\$ Thanks very much for your comments. I will fix my program based on your comments. \$\endgroup\$ Commented Oct 4, 2018 at 16: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.