6
\$\begingroup\$

So I made this calculator in c# console, I'm pretty nooby-ish in programming. I want to know if I can make it better, because my coding skills suck. Pretty sure my logic sucks too.

Any advice and tips?

   public static void Main() {
        bool returnToStart;

        do {
            returnToStart = Calc();

        } while(returnToStart == true);

        Console.ReadLine();

    }
    public static bool Calc() {
        ExitProgram();
        Console.Clear();

        float firstNumber; float secondNumber; float result; 
        string operation;

        Console.Write("First number: ");
        firstNumber = float.Parse(Console.ReadLine());

        Console.Write("Choose your operator: addition(+), subtraction(-), " +
                     "multiplication(*) or division(/): ");
        operation = Console.ReadLine();

        Console.Write("Second number: ");
        secondNumber = float.Parse(Console.ReadLine());

        Console.Clear();


        switch(operation) {
            case "+":
                result = (firstNumber + secondNumber);
                Console.WriteLine("Calculation: {0}", result);
                break;
            case "-":
                result = (firstNumber - secondNumber);
                Console.WriteLine("Calculation: {0}", result);
                break;
            case "*":
                result = (firstNumber * secondNumber);
                Console.WriteLine("Calculation: {0}", result);
                break;
            case "/":
                if(firstNumber == 0) {
                    Console.WriteLine("Cant divide by zero!");
                } else {
                    result = (firstNumber / secondNumber);
                    Console.WriteLine(result);
                }
                break;

            default:
                Console.WriteLine("Error! Try again.");
                break;


        }
        return true;

    }
    public static bool ExitProgram() {
        string exit = "Exit";

        Console.WriteLine("If you're done with your calculations,\nthen you can exit this program by simply typing in 'Exit' ");
        Console.Write("Want to quit the program?: ");
        exit = Console.ReadLine();

        if(exit == "Exit") {
           Environment.Exit(0);

        }return true;

    }

}

}

\$\endgroup\$
7
\$\begingroup\$

Keep It Simple For the User Rather than making the user type the word Exit it might be better to ask the user Are you done yet(y/n)? and accept a simple yes/no answer. It also might be better if the answer was not case sensitive so accept Y, y, N and n.

Exit From Main() When Possible The Main() function may contain clean up code so exiting from the ExitProgram() function may not be the best idea. A second problem I see with the ExitProgram() function is that it is called before the user ever enters a calculation. There are two possible ways to handle this, one would be to move the call to ExitProgram() to after the calculation is performed, the other would be to have a function that contains a do while loop that calls Calc within the loop and tests ExitProgram() in the while condition.

Function Complexity
The function Calc() is overly complex (it does too much) and should be multiple functions. One of the functions should get the user input, and a second function should do the calculation. This would be applying two programming principles, the Single Responsibility Principle and the KISS Principle.

The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

The Keep It Simple (KISS) Principle is an engineering principle that predates computers, basically it is keep the implementation as simple as possible.

| improve this answer | |
\$\endgroup\$
4
\$\begingroup\$

Use Intention-Revealing Names

Based on naming alone, the intent of the Calc() method is not clear. As the reader of your code, I have to look at the implementation details of this method to understand what we are calculating.

Method Characteristics

Pay careful attention to your method signatures.

Do ExitProgram() and Calc() need to both be static? If Calc() no longer used the console to display I/O information and had a dependency on something else used for display, unit testing becomes much more difficult due to it being a static method.

Should ExitProgram() be public? If this was used in a larger application, would you want others to call this method? It seems like the answer is no, as the UI output of this method is coupled with information about the calculations.

Control Flow and Validation

Ideally, main should delegate all processing elsewhere. In the Calc() method, we should be performing validation on user input. If I enter an invalid operation, I get the message "Error! Try again." without knowing exactly what I did wrong. I am also forced to re-enter all inputs if I want to try again. This may be an opportunity to practice throwing and catching exceptions for cleaner code and a better user experience.

Conclusion

I like the way you have started to separate your methods into ways that make sense. I also appreciate the line breaks within your methods to further separate things into logical groupings. I will leave a link to my live code review below and I hope this feedback helps!

Live code review: https://www.youtube.com/watch?v=bFVBYoTrTiM

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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