6
\$\begingroup\$

It is a basic calculator where the user inputs two numbers, and an operation and the program makes it into an equation and gets the answer. For example, if the user enters number 2 number 3 and tells it to multiply, it will display the answer as 6. I just want to know how this could be improved.

Could I have used loops or more advanced procedures to get the same output?

 namespace Test
 {
    class Program
    {
            static void Main(string[] args)
            {
                int firstNum;
                int secondNum;                   //Variables for equation
                string operation;
                int answer;

                Console.WriteLine("Hello, welcome to Alex's basic calculator!");
                Console.ReadLine();

                Console.Write("Enter the first number in your basic equation: ");
                firstNum = Convert.ToInt32(Console.ReadLine());                             

                //User input for equation
                Console.Write("Now enter your second number in the basic equation: ");
                secondNum = Convert.ToInt32(Console.ReadLine());
                Console.Write("Ok now enter your operation ( x , / , +, -) ");
                operation = Console.ReadLine();

                if (operation == "x")
                {
                   answer = firstNum * secondNum;
                   Console.WriteLine(firstNum + " x " + secondNum + " = " + answer);
                   Console.ReadLine();
                }
                else if (operation == "/")
                {
                   answer = firstNum / secondNum;
                   Console.WriteLine(firstNum + " / " + secondNum + " = " + answer);
                   Console.ReadLine();
                }                                                                           
                //Getting answers
                else if (operation == "+")
                {
                   answer = firstNum + secondNum;
                   Console.WriteLine(firstNum + " + " + secondNum + " = " + answer);
                   Console.ReadLine();
                }
                else if (operation == "-")
                {
                   answer = firstNum - secondNum;
                   Console.WriteLine(firstNum + " - " + secondNum + " = " + answer);
                   Console.ReadLine();
                }
                else
                {
                   Console.WriteLine("Sorry that is not correct format! Please restart!");     //Catch
                   Console.ReadLine();
                }
           }
     }
}
\$\endgroup\$
  • \$\begingroup\$ You can check out my answer on this question : codereview.stackexchange.com/questions/126141/… it's almost the same questions as your's. It includes some useful tips like verifying the input of your program and some more ideas on how to store your data or check what the user have entered. \$\endgroup\$ – Denis Jun 5 '16 at 21:25
15
\$\begingroup\$

I'm going to go over a few things in this answer, hopefully it makes them simple enough. Then, at the end, I'm going to go way overboard and significantly over-engineer this programme while still making it shorter.


First, let's talk about your input handling (or lack thereof). As programmers, we should strive to be able to gracefully handle any/all input, and do something appropriate as a result. For invalid input, usually we reprompt (sometimes we just gracefully fail), for valid input, we go to the next step.

You assume all input will be valid, which is a huge potential for problematic behavior. What happens if I enter one for either number? Bad things, my friend, bad things.

So, let's handle all that gracefully. I wrote a specific portion of a library I have written specifically for input handling, and gracefully doing so.

Basically, you need all three files from this GitHub folder. You will also need both the bottom files, from this other GitHub folder. You'll want to modify the namespaces to suit your structure. (You may also just download the entire repo, and build Evbpc.Framework, which I recommend. All of this is beyond the scope of this answer.)

Next, you'll want to create a ConsolePrompt object:

var prompter = new ConsolePrompt(null);

Once you've done that, you can simply use:

var validOperations = new[] { "x", "/", "+", "-" };
var firstNum prompter.Prompt<int>("Enter the first number in your basic equation", PromptOptions.Required);
var secondNum = prompter.Prompt<int>("Now enter your second number in the basic equation", PromptOptions.Required);
var operation = prompter.Prompt<string>("Ok now enter your operation (" + string.Join(", ", validOperations) + ")", PromptOptions.Required, validationMethod: x => validOperations.Contains(x));

Now we've done three things here:

  1. We validate all the numbers and reprompt the user if the number is invalid.
  2. We make sure the user only entered a basic operation for the intended operation.
  3. We make a list of valid operations at the beginning, which means if we add one we can do so easily.

Next, we'll talk about your conditions. You use an if statement for all your operations, but that's unnecessary and creates extra processing in the case that the operation is the last condition. (Each previous if is evaluated before it continues to the next.)

Instead, we'll go to a switch statement:

switch (operation)
{
    case "x":
        answer = firstNum * secondNum;
        break;
    case "/":
        answer = firstNum / secondNum;
        break;
    case "+":
        answer = firstNum + secondNum;
        break;
    case "-":
        answer = firstNum - secondNum;
        break:
}

By using a switch we've made things substantially more clear: our intention is to only evaluate what operation is and do something based on it.


Next, we'll talk about your output. You repeat certain statements excessively, let's find a better way to do that.

We have firstNum and secondNum, as well as operation and answer. So how can we rewrite our Console.WriteLine methods to be more dynamic? Well, with the switch it's easy, right after the end brace for the switch we just write what each value was:

Console.WriteLine(firstNum + " " + operation + " " + secondNum + " = " + answer);

Boom. We don't need the Console.WriteLine in each switch (or if) block. We just need one at the end.

But there's a better way to write this (with C#6.0, that is):

Console.WriteLine($"{firstNum} {operation} {secondNum} = {answer}");

Using string interpolation we make it much easier to read what's going on.


You also had the idea for a while loop in your head to repeat the calculations, so let's do that:

do
{
    // All our calculator code
} while (prompter.Prompt("Enter exit to quit, anything else to continue", PromptOptions.Optional, "", parseResultMethod: x => x.ToLowerInvariant()) != "exit");

And now it will continue until the user types exit at that prompt.


So, we've improved everything you've got going, but there's still one more improvement we can make: convert all our operations to a dictionary.

What do I mean? Well I'll show you.

var operations = new Dictionary<string, Func<int, int, int>>
{
    {"x", (x, y) => x * y },
    {"/", (x, y) => x / y },
    {"+", (x, y) => x + y },
    {"-", (x, y) => x - y }
};
var operation = prompter.Prompt<string>("Ok now enter your operation (" + string.Join(", ", operations.Keys) + ")", PromptOptions.Required, null, null, x => operations.Keys.Contains(x));
var firstNum = prompter.Prompt<int>("Enter the first number in your basic equation", PromptOptions.Required);
var secondNum = prompter.Prompt<int>("Enter the second number in your basic equation", PromptOptions.Required);
var answer = operations[operation](firstNum, secondNum);

What did we just do? We just replaced the entire switch/if blocks, all the manually typing x, /, +, -, etc. with a dictionary of string -> function. Where does this benefit us? Simple. I want to add exponential calculation.

Simply add:

{"^", (x, y) => (int)Math.Pow(x, y)}

to our operations dictionary. Now all our prompts and whatnot can support the power function.

This brings up a new issue: you have no support for decimal/double/float numbers. This is really easy to modify, simply replace all our int types with double.


Lastly, as for a bonus to this answer, you can also add alias operations. (What do I mean? Well what if I naturally use * instead of x for multiplication?)

This is super easy:

// Let's add our alias operations, */x
operations.Add("*", (x, y) => operations["x"](x, y));

Simply add them after the var operations statement.

var operations = new Dictionary<string, Func<double, double, double>>
{
    {"x", (x, y) => x * y },
    {"/", (x, y) => x / y },
    {"+", (x, y) => x + y },
    {"-", (x, y) => x - y },
    {"^", (x, y) => Math.Pow(x, y) },
    {"%", (x, y) => x % y }
};

// Let's add our alias operations, */x
operations.Add("*", (x, y) => operations["x"](x, y));

Boom, now you can use * instead of x for multiplication. (But you can still use x as well.)


Our entire programme is now down to 29 lines with appropriate whitespace, and supports much more than your original did. (Your original was 44 lines.)

var prompter = new ConsolePrompt(null);

Console.WriteLine("Hello, welcome to Alex's basic calculator!");
Console.ReadLine();

var operations = new Dictionary<string, Func<double, double, double>>
{
    {"x", (x, y) => x * y },
    {"/", (x, y) => x / y },
    {"+", (x, y) => x + y },
    {"-", (x, y) => x - y },
    {"^", (x, y) => Math.Pow(x, y) },
    {"%", (x, y) => x % y }
};

// Let's add our alias operations, */x
operations.Add("*", (x, y) => operations["x"](x, y));

do
{
    var firstNum = prompter.Prompt<double>("Enter the first number in your basic equation", PromptOptions.Required);
    var secondNum = prompter.Prompt<double>("Enter the second number in your basic equation", PromptOptions.Required);
    var operation = prompter.Prompt<string>("Ok now enter your operation (" + string.Join(", ", operations.Keys) + ")", PromptOptions.Required, null, null, x => operations.Keys.Contains(x));

    var answer = operations[operation](firstNum, secondNum);

    Console.WriteLine($"{firstNum} {operation} {secondNum} = {answer}");
    Console.ReadLine();
} while (prompter.Prompt("Enter exit to quit, anything else to continue", PromptOptions.Optional, "", parseResultMethod: x => x.ToLowerInvariant()) != "exit");

Pretty cool, eh?


All-in-all, very good start. Hopefully you learned a lot from this answer and can apply some of it to future applications. :)

\$\endgroup\$
8
\$\begingroup\$

That's a pretty good start...

Be aware that entering text (like "hello") at a number prompt will cause an exception. There are several ways to approach this. The easiest would be to use Int32.TryParse. Here is an example:

    static int PromptForInteger(string message, int minValue, int maxValue)
    {
        bool done;
        int answer = 0;

        // it will run at least once, but we don't know how many times
        do
        {
            // start with an assumption
            done = true;

            // display the prompt
            Console.Write(message);

            // check to see if what was entered "looks like" an integer
            if (Int32.TryParse(Console.ReadLine(), out answer) == false)
            {
                // display an error message, and try again
                Console.WriteLine("Could not convert your input into a whole number, please try again");
                // set our sentinel to indicate a problem
                done = false;
            } else if (answer < minValue || answer > maxValue)
            // if the value within the acceptible range?
            {
                // display an error message, and try again
                Console.WriteLine("Valid values are between {0} and {1}, please try again", minValue, maxValue);
                // set our sentinel to indicate a problem
                done = false;
            }
        } while (!done);

        // if we make it here, everything is OK
        return answer;
    }
\$\endgroup\$
3
\$\begingroup\$

For starters, you did a great job at writing your first program, there are few things i will highlight here that will be useful in your future programs.

  1. DRY- Do not Repeat Yourself: in each of the decision statements you had the variable answer get assigned multiple times, Console.WriteLine and Console.ReadLine repeated. Always find a workaround when you notice repetition.

  2. Error Handling: Just as every one pointed, when reading inputs you need to validate or check their validity. For instance, in my main method I have this line of codes

    static void Main(string[] args) { // Input variables for calculation int firstNum; int secondNum;
    string operation; string answer;

            Console.WriteLine("Hello, welcome to Alex's basic calculator!");
            Console.Write("Enter the first number in your basic equation: ");
    
           while (!Int32.TryParse(Console.ReadLine(), out firstNum))
            {
                Console.WriteLine("Could not process the first Input");
                Console.Write("Enter the first number in your basic equation: ");
            }
            Console.Write("Now enter your second number in the basic equation: ");       
            while (!Int32.TryParse(Console.ReadLine(), out secondNum))
            {
                Console.WriteLine("Could not process the second Input");
                Console.Write("Now enter your second number in the basic equation: ");
            }
            Console.Write("Ok now enter your operation ( x , / , +, -) ");
            operation = Console.ReadLine();
            answer = Calculate(firstNum, secondNum, operation);
            Console.WriteLine(answer);
            Console.ReadLine();
        }
    

Notice, I used Int32.TryParse() to check if your input values can be parsed to int and if not loop till you get a valid input. The validation for the operation is taken care of by the default in the switch statement. Depending on the interaction you aim to achieve, you could implement something similar for the operation or stop the program when inaccurate results are passed.

  1. Meaningful names: For future references, it's best to have a good naming convention such as your namespace and class should depict the use of the class. For example, the class could be given be the name CalculatorSystem(Pascal case).

  2. Grouping with Methods: To make your codes reusable , its best to group them using methods. Note, methods in C# are in Pascal case as well. An example of this

I refactored your solution and i created a Calculate method that has firstNum, secondNum and operation as inputs and returns the answer as a string . Although, i tried to to change your code and make it simple.

 private static string Calculate(int firstNum, int secondNum, string operation)
    {
        int answer = 0;


        // Switch used as a replacement for if statement
        switch (operation)
        {
            case "x":
                answer = firstNum * secondNum;
                break;
            case "/":
                answer = firstNum / secondNum;
                break;
            case "+":
                answer = firstNum + secondNum;
                break;
            case "-":
                answer = firstNum - secondNum;
                break;
            default:
                return "Sorry that is not correct format! Please restart!";
                //  break;
        }
        return new StringBuilder(firstNum + operation + secondNum + " = " + answer).ToString();
    }

Notice I used a switch as opposed to an if statement and you can find the advantages of using the switch here Advantages of Switch over If statement.

Also for good practices, its good to seclude your print out statements to the Main method only because later on you might decide to have a gui appplication displaying the answer and making amends to the Main method is easier .

Note: I removed the Console.ReadLine() after the "Hello, welcome to Alex's basic calculator because it was redundant. You can make some amends by asking for users' names etc.

  1. Comments: Including comments is regarded as a good practice, which am glad you imbibed during your console application.

  2. Other Interesting activities to delve into include checking the behaviour of your program if floats, doubles were inputted( will it be truncated) and experimenting with Environment.Exit(0) to enable users exit the application. Good luck with coding.

\$\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.