A very basic calculator in C#

Question

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;
 }
 }
 }
}

Answer 1

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.

• user555045: I would avoid this recursion as a non-beginner as well to be honest
• Peter Csala: @harold Yes, you are right :)
• i_hate_F_sharp: @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.
• Peter Csala: @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.