12
\$\begingroup\$

I'm pretty new to C# and was just wondering if there is a better way I can write this calculator that I made:

        using System;
        using System.Collections.Generic;
        using System.Linq;
        using System.Text;
        using System.Threading.Tasks;

        namespace AnthonyCalculator
               {
        class Program
          {
        static void Main(string[] args)
        {
            double num01;
            double num02;
            int userChoice = 1;

            Console.WriteLine ("What is your first number?: ");
            num01 = double.Parse (Console.ReadLine());

            Console.WriteLine ("\nPlease select one of the following (1-4)");
            Console.WriteLine ("1 - Add");
            Console.WriteLine ("2 - Subtract");
            Console.WriteLine ("3 - Divide");

            Console.Write("Select number 1-3: ");
            userChoice = int.Parse (Console.ReadLine ());

            switch (userChoice)
            {
                case 1:
                    Console.WriteLine ("\nAddition Selected: ");
                    Console.Write ("\nSecond number: ");
                    num02 = double.Parse (Console.ReadLine());
                    Console.WriteLine ("\nTotal of " + num01 + " + " + num02 + "      = " + (num01 + num02));
                    break;

                case 2:
                    Console.WriteLine ("Subtract Selected: ");
                    Console.Write("\nSecond number: ");
                    num02 = double.Parse(Console.ReadLine());
                    Console.WriteLine("\nTotal of " + num01 + " - " + num02 + " = " + (num01 - num02));
                    break;

                case 3:
                    Console.WriteLine("Division Selected: ");
                    Console.Write("\nSecond number: ");
                    num02 = double.Parse(Console.ReadLine());
                    Console.WriteLine("\n Total of " + num01 + " / " + num02 + " = " + (num01 / num02));
                    break;

                default:
                    Console.WriteLine("Sorry, incorrect input");
                    break;
            }
            Console.ReadLine();

        }
    }
}
\$\endgroup\$
22
\$\begingroup\$

I'll blame the extra-weird broken indentation on copy/paste difficulties, and ignore that.

There are as many ways to write a calculator as there are programmers willing to write one. So I'm not going to suggest other ways, instead I'm going to hint at how you could improve the flexibility of what you've got.

The first concern you have, is getting user input. Your code works when the sky is blue and the user isn't being too clever.

QA engineer walks into a bar. Orders a beer. Orders 0 beers. Orders 999999999 beers. Orders a lizard. Orders -1 beers. Orders a sfdeljknesv.

https://twitter.com/JimFraeErskine/status/527564769302433792

What happens if you play the QA engineer here?

Console.WriteLine ("What is your first number?: ");
num01 = double.Parse (Console.ReadLine());

Your code is making the assumption that the user will enter a valid double! Look into TryParse, and write a separate function with the sole purpose of turning a string into a double. I'd actually make that function return a double? (or Nullable<double>, so that the calculator knows the number isn't valid when that function returns a null).


Console.WriteLine ("\nPlease select one of the following (1-4)");
Console.WriteLine ("1 - Add");
Console.WriteLine ("2 - Subtract");
Console.WriteLine ("3 - Divide");

Console.Write("Select number 1-3: ");

That's ...rather interesting. It looks like at one point it was intended to support 4 - Multiply, but the idea was dropped and now 4 is an invalid input despite the instructions at the top saying (1-4), and the instructions at the bottom saying 1-3!

How about you take the concern of creating the menu out of there, find a way to encapsulate each operation into its own strategy object, and generate the menu based on how many available strategies you have?

Say you had an enum:

public enum CalculatorOperation
{
    Add,
    Subtract,
    Divide,
    Multiply
}

You could then "map" each enum to a function taking two double values and returning another - that's a Func<double,double,double>:

var strategies = new Dictionary<CalculatorOperation,Func<double,double,double>>
{
    { CalculatorOperation.Add,      (x, y) => x + y },
    { CalculatorOperation.Subtract, (x, y) => x - y },
    { CalculatorOperation.Divide,   (x, y) => x / y },
    { CalculatorOperation.Multiply, (x, y) => x * y },
};

What just happened here? You've given yourself the tools to eliminate that switch block - given userChoice is parsed into a CalculatorOperation instead of just an int, you could do this:

var result = strategies[userChoice](num01, num02);

How does that work? Delegates, my friend. And generics. But delegates, my friend. You see, this:

(x, y) => x * y

Is shorthand for:

delegate(double x, double y) { return x * y; }

Which could just as well be this:

private double Multiply(double x, double y)
{
    return x * y;
}

And, if you had one such method for each operator, you could just as well define your dictionary like so:

var strategies = new Dictionary<CalculatorOperation,Func<double,double,double>>
{
    { CalculatorOperation.Add, Add },
    { CalculatorOperation.Subtract, Subtract },
    { CalculatorOperation.Divide, Divide },
    { CalculatorOperation.Multiply, Multiply },
};

Because the signature double Multiply(double x, double y) matches that of a delegate taking two double parameters and returning a double - that's really all delegates are: functions that you can encapsulate, and pass around as parameters, or store inside a dictionary to execute later.

You could then generate the menu items simply by iterating the dictionary keys.

Now, you'll still have to worry about division by zero and overflows (what happens if you add double.MaxValue and double.MaxValue?), and then separate the concerns.

One of the most important principles, IMO, is the Single Responsibility Principle. Your current code has user input/feedback, validation(?) and the actual logic intertwined, which makes it harder to maintain/extend than it needs to be.

Identify what the concerns are, and extract them into their own methods; monolithic blocks of code are the easiest to write, but the hardest to maintain.

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

First off, indent your class/namespaces. It makes things look a whole lot clearer. Even it's not necessary, you should still do it.

Secondly, do you need all those usings? I don't see you using System.Collections.Generic, System.Linq, System.Text, or System.Threading.Tasks. You can get rid of these.

Finally, you shouldn't be trusting any user input. If you want user input to be converted to integers, I'd use Int32.TryParse, rather than just int.Parse.

\$\endgroup\$
1
\$\begingroup\$

Using a dictionary like in the @Mat'sMug solution is one way to do it.

Another way is to make it modular and extendable with a few interfaces and classs.

You start with an interface for a binary operation:

interface IBinaryOperation
{
    string Description { get; }
    string Operator { get; }
    bool CanCalculate(string binaryOperator);
    double Calculate(double x, double y);
}

From this interface you can derive any binary operation you want. I did only for addition:

class AddOperation : IBinaryOperation
{
    public AddOperation()
    {
        Description = "Addition";
        Operator = "+";
    }
    public string Description { get; }
    public string Operator { get; }
    public bool CanCalculate(string binaryOperator) => binaryOperator == Operator;
    public double Calculate(double x, double y) => x + y;
}

You enter the expression as a string i.e. 2.5 + 1.2, you parse it and store the result as a binary expression:

class BinaryExpression
{
    public BinaryExpression(double x, double y, string binaryOperator)
    {
        X = x;
        Y = y;
        BinaryOperator = binaryOperator;
    }

    public double X { get; }
    public double Y { get; }
    public string BinaryOperator { get; }
}

finally you write the calculator that parses the expression and calls the right operation to calculate the result:

class Calculator
{
    private readonly Regex _matcher = new Regex(
        @"(?<x>\d+([.]\d+)?)\s*(?<operator>.+?)\s*(?<y>\d+([.]\d+)?)", 
        RegexOptions.ExplicitCapture
    );

    public Calculator(IEnumerable<IBinaryOperation> operations)
    {
        Operations = operations;
    }

    public IEnumerable<IBinaryOperation> Operations { get; }

    public double? Calcualte(string expression)
    {
        var expr = Parse(expression);
        return Operations.FirstOrDefault(o => o.CanCalculate(expr.BinaryOperator))?.Calculate(expr.X, expr.Y);
    }

    private BinaryExpression Parse(string expression)
    {
        var match = _matcher.Match(expression);
        if (!match.Success)
        {
            throw new ArgumentException($"Could not parse expression: {expression}");
        }
        return new BinaryExpression(
            double.Parse(match.Groups["x"].Value, CultureInfo.InvariantCulture),
            double.Parse(match.Groups["y"].Value, CultureInfo.InvariantCulture),
            match.Groups["operator"].Value);
    }
}

This is a very simple example. The parser itself could be a module too.

And you use it like this:

class Program
{
    static void Main(string[] args)
    {
        var calc = new Calculator(new[]
        {
            new AddOperation()
        });

        Console.WriteLine("What would you like to calculate?");
        Console.WriteLine($"You can use: [ {string.Join(", ", calc.Operations.Select(o => o.Operator))} ]");
        var expression = Console.ReadLine();

        var result = calc.Calcualte(expression);

        if (result.HasValue)
        {
            Console.WriteLine();
            Console.WriteLine($"The result is: {result.Value}");
        }
        else
        {
            Console.WriteLine();
            Console.WriteLine($"This operation is not supported.");
        }
    }
}
\$\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.