2
\$\begingroup\$

I'm trying to learn C#, I made this calculator with my current knowledge. It works as I intended, but I'd appreciate some input on my code.

using System;

namespace Calculator
{
    class Program
    {
        static void Main(string[] args)
        {
            double input1;
            string input2;
            double input3;

            Console.WriteLine("Super cool calculator 1.0");

            Console.WriteLine("This can perform +, -, / or * with two numbers.");

            try
            {
                Console.WriteLine("Enter first number:");
                input1 = Convert.ToDouble(Console.ReadLine());

                Console.WriteLine("Enter operator (+, -, /, *)");
                input2 = Console.ReadLine();

                Console.WriteLine("Enter second number:");
                input3 = Convert.ToDouble(Console.ReadLine());

                Calculate(input1, input2, input3);
            }
            catch (Exception e)
            {
                Console.WriteLine("Wrong Input! Try again!");
                Main(null);
            }

        }


        static void Calculate(double n1, string n2, double n3)
        {
            string op = n2;
            switch (op)
            {

                case "+":
                    Console.WriteLine($"The answer is {n1} + {n3} = {n1 + n3}");
                    Main(null);
                    break;

                case "-":
                    Console.WriteLine($"The answer is {n1} - {n3} = {n1 - n3}");
                    Main(null);
                    break;

                case "/":
                    Console.WriteLine($"The answer is {n1} / {n3} = {n1 / n3}");
                    Main(null);
                    break;

                case "*":
                    Console.WriteLine($"The answer is {n1} * {n3} = {n1 * n3}");
                    Main(null);
                    break;

                default:
                    Console.WriteLine("Invalid Operation!");
                    Main(null);
                    break;
            }

        }




    }
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ I love your Main(null) calls :). Later today I will leave a proper review. \$\endgroup\$ Sep 22 at 11:50

1 Answer 1

4
\$\begingroup\$
static void Main(string[] args)

In C# you can change the Main method's signature to one of the followings depending on your needs:

public static void Main() { }
public static int Main() { }
public static void Main(string[] args) { }
public static int Main(string[] args) { }
public static async Task Main() { }
public static async Task<int> Main() { }
public static async Task Main(string[] args) { }
public static async Task<int> Main(string[] args) { }

So, you don't need to use the string[] args version.


double input1;
string input2;
double input3;

Try to express your intent with the variable names. I would suggest the following renamings:

  • input1 >> lhsOperand
  • input3 >> rhsOperand
  • input2 >> infixOperator

try
{
     ...
}
catch (Exception e)
{
     Console.WriteLine("Wrong Input! Try again!");
     Main(null);
}

I would suggest to try to avoid recursion if you are a beginner in C#. Please prefer do-while loop instead. With that structure you can express your code like this "please provide input data until all are considered valid".


input1 = Convert.ToDouble(Console.ReadLine());

Please prefer double.TryParse which is a tester-doer pattern.

  • If the input can be parsed as double then the method will return true and will populate the second parameter with the parsed value.
  • If it can't parse the input then the method will return false and will populate the second parameter with default value (0).

default:
    Console.WriteLine("Invalid Operation!");
    Main(null);
    break;

I would suggest to perform the operator validity check inside the Main method. Main should request input data and validate them. It should call Calculate only if all the input data are considered valid.


void Calculate(double n1, string n2, double n3)

Please use more expressive parameter names than these.


string op = n2;

This line of code is unnecessary, you can do the branching against the method parameter.


case "+":
    Console.WriteLine($"The answer is {n1} + {n3} = {n1 + n3}");
    Main(null);
    break;

In all of your branches you are repeating yourself:

  • Write out the result of the calculation
  • Re-execute the program

The only difference is the printed text. So, I would suggest to change the return type of Calculate to double and do the following:

static double Calculate(double lhsOperand, string infixOperator, double rhsOperand)
{
    switch (infixOperator)
    {
        case "+": return lhsOperand + rhsOperand;
        case "-": return lhsOperand - rhsOperand;
        case "/": return lhsOperand / rhsOperand;
        case "*": return lhsOperand * rhsOperand;
        default: return double.NaN; //or replace it with anything appropriate
    }
}

And use it like this

Console.WriteLine($"The answer for {lhsOperand} {infixOperator} {rhsOperand} is {Calculate(lhsOperand, infixOperator, rhsOperand)}");

Or a super-concise version could be:

var result = infixOperator switch
{
    "+" => lhsOperand + rhsOperand,
    "-" => lhsOperand - rhsOperand,
    "/" => lhsOperand / rhsOperand,
    "*" => lhsOperand * rhsOperand,
    _ => double.NaN,
};
Console.WriteLine($"The answer is {lhsOperand} {infixOperator} {rhsOperand} = {result}");

which would eliminate the need for the Calculate function entirely.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Thank you for the detailed review :) \$\endgroup\$
    – Adalwolf
    Sep 23 at 21:38
  • 2
    \$\begingroup\$ I would avoid this recursion as a non-beginner as well to be honest \$\endgroup\$
    – harold
    Sep 24 at 23:18
  • \$\begingroup\$ @harold Yes, you are right :) \$\endgroup\$ Sep 25 at 5:53
  • \$\begingroup\$ @PeterCsala I disagree with the use of double.TryParse the value is set to 0 if it fails... How would you check if the client passed in 0 or was it the TryParse that set it to 0? I think the try-catch-finally approach here is totally fine. \$\endgroup\$ Nov 9 at 23:30
  • \$\begingroup\$ @I_throw_but_dont_catch If the app user provides 0 then the TryParse returns true. If the app user provides zero as a string then the TryParse returns false. So, you can distinguish 0 as a valid input from malformed inputs. \$\endgroup\$ Nov 10 at 5:56

Your Answer

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

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