Improving a Calculator Application

Question

I have here a calculator created using C# that is running on a console.

The calculator accepts a string input (e.g. 5+5) then produces the result (5+5=10).

It will then prompt the user to enter another operator which will continue the evaluation (e.g. 10+5=15).

To terminate the program, one can type the string "done".

I would like to ask what other improvements can I make to increase the effiency of my code. Thank You!.

       using System;
    using System.Collections.Generic;
    using System.Linq;
    using System.Text.RegularExpressions;
    using System.Xml.Linq;
    using System.Text;
    using System.Threading.Tasks;
    using System.Diagnostics;

    namespace Calculator
    {
        class Calculate
        {
            ulong firstNumber { get; set; }
            ulong secondNumber { get; set; }
            public static ulong result = 0;
            string equationOperator { get; set; }

            public Calculate(ulong firstNumber, ulong secondNumber, string equationOperator)
            {

                this.firstNumber = firstNumber;
                this.secondNumber = secondNumber;
                this.equationOperator = equationOperator;

                switch (this.equationOperator)
                {
                    case "+":
                        result = this.firstNumber + this.secondNumber;
                        break;
                    case "-":
                        if (result < this.secondNumber)
                        {
                           result = this.secondNumber - this.firstNumber;
                           break;
                        }
                        result = this.firstNumber - this.secondNumber;
                        break;
                    case "/":
                        if (this.secondNumber == 0)
                        {
                            Console.WriteLine("\nCannot Divide by Zero!");
                            break;
                        }
                        result = this.firstNumber / this.secondNumber;
                        break;
                    case "*":
                        result = this.firstNumber * this.secondNumber;
                        break;
                    case "%":
                        result = this.firstNumber % this.secondNumber;
                        break;
                }

                Console.WriteLine("\n{0}\t{1}\t{2}\t= {3}", this.firstNumber, this.equationOperator, this.secondNumber, result);
            }


        }

        class ContinueEval
        {
            public static void continueEvaluate()
            {
                string nextInput = "", equationOperator = "", secondNumber = "";
                ulong prevResult;
                while (nextInput != "done")
                {
                    Console.Write("\n{0}",Calculate.result);
                    prevResult = Calculate.result;
                    nextInput = Console.ReadLine();
                    if (nextInput == "done")               
                        break;            
                    ushort n = 1;
                    for (n = 0; n < nextInput.Length; n++)
                    {
                        if (!(Regex.IsMatch(nextInput.Substring(0, n), @"^\d+$")))
                        {
                            equationOperator = nextInput[n].ToString();
                            break;
                        }
                    }
                    secondNumber = nextInput.Substring((n + 1), (nextInput.Length - 1));

                    switch (equationOperator)
                    {
                        case "+":
                            Calculate.result += ulong.Parse(secondNumber);
                            break;
                        case "-":
                            if (Calculate.result < ulong.Parse(secondNumber))
                            {
                                Calculate.result = ulong.Parse(secondNumber) - Calculate.result;
                                break;
                            }
                            Calculate.result -= ulong.Parse(secondNumber);
                            break;
                        case "/":
                            if (ulong.Parse(secondNumber) == 0)
                            {
                                Console.WriteLine("\nCannot Divide by Zero!");
                                break;
                            }
                            Calculate.result /= ulong.Parse(secondNumber);
                            break;
                        case "*":
                            Calculate.result *= ulong.Parse(secondNumber);
                            break;
                        case "%":
                            Calculate.result %= ulong.Parse(secondNumber);
                            break;
                    }
                    Console.WriteLine("\n{0}\t{1}\t{2}\t= {3}", prevResult, equationOperator, secondNumber, Calculate.result);

                };
            }
        }

        class ESlicer
        {
            public static void sliceEquation(string inputValue)
            {
                if (inputValue == "done")
                    Environment.Exit(0);
                string firstNumber = "", secondNumber = "", equationOperator = "";
                ushort n = 1, end = 0;

                while (Regex.IsMatch(inputValue.Substring(0, n), @"^\d+$"))
                {
                    firstNumber = inputValue.Substring(0, n);
                    if (!(Regex.IsMatch(inputValue[n].ToString(), @"^\d+$")))
                    {
                        equationOperator = inputValue[n].ToString();
                        break;
                    }
                    if (n > inputValue.Length)
                        break;
                    n++;
                }
                end = (ushort)(inputValue.Length - (firstNumber.Length + 1));
                secondNumber = inputValue.Substring((n + 1), end);
                Calculate p = new Calculate(ulong.Parse(firstNumber), ulong.Parse(secondNumber), equationOperator);
            }
        }


        class Program
        {
            static void Main()
            {
                string inputValue;
                Console.Write("Enter string of Equation: ");
                inputValue = Console.ReadLine();


          ESlicer.sliceEquation(inputValue);
            ContinueEval.continueEvaluate();
            Console.Write("Thank you for using!");
            Console.Read();
        }

    }
}

Answer 1

From your last comment, what you're actually looking for is not more efficiency (which is difficult to measure anyway), but more quality, and I think we can do that.

The biggest problem is that the program is very hard to read and follow, and thus hard to maintain. This is for several reasons:

• Mixed responsibilities. You have split the work that your program needs to do into several classes, but not all of them do just one thing.
• Unclear program flow. This is in part a result the first issue. At any level of the program it is very hard to tell what the current state is, where it comes from and exactly how the current code influences what happens later, or which other code the current part directly depends on. This is also due to your intermixing of class instance code, static code and semi-static constructors. This is a rather small program that can be written in different styles, but not all at the same time.

The most striking specific issue just from scrolling through the code is that the evaluation of the operator and the calculation of the result are present twice, and from looking more closely, it becomes clear that pretty much the whole program is present twice - the ContinueEval class has almost everything that is needed; that is because after the very first equation, the program only keeps looping within the continueEvaluate() method for all subsequent operations.

That means that the ContinueEval class has a number of responsibilities:

• Taking the new input from the console
• Parsing the input
• Evaluating the operator
• Doing the actual calculation
• Displaying the result
• Looping back to take the next input

That is a bit much to ask of one class. In addition, it duplicates the core functionality of the ESlicer (more or less) and Calculate classes in the process. One big problem this causes is that whenever you decide to change the way the input is parsed or add support for a new mathematical operation, you will have to make changes in two places, which is cumbersome and error prone.

Let's break down the program flow that is currently a bit hidden, not least because of the use of global state in the Calculate class (try to avoid using global/static state as much as you can, and instead look for sensible ways to pass everything you need to the classes that require it; this will help a lot in making program flow and dependencies more explicit and easier to follow).

• Take the first input
• Parse it for the arguments and operator
• Calculate the first result
• (Start of the loop) Take the next input
• Parse it for the operator and argument
• Calculate the new result
• Loop back to take the next input

Looking at this shows that there are three tasks that are being repeated - taking an input, parsing it and calculating the result. At least the latter two are non-trivial, and we should do what is necessary to only have the code for them once; this will help a lot in understanding and maintaining the program.

I have tidied up the responsibilities and ended up with two classes for the main tasks, along with one to transfer the result of the parsed equation and a main loop which I left in Program.Main() for the time being[1]:

using System;
using System.Text.RegularExpressions;

namespace CalculatorX
{
    public class Program
    {
        private static void Main()
        {
            Calculator calculator = new Calculator();
            EquationParser parser = new EquationParser();

            Console.Write("Enter string of Equation: ");

            bool doLoop = true;
            bool isFirstLoop = true;

            do
            {
                if (!isFirstLoop)
                {
                    Console.Write(calculator.Result);
                }

                string inputValue = Console.ReadLine();
                if ("done" == inputValue)
                {
                    doLoop = false;
                }
                else
                {
                    ParseResult parsedInput = parser.Parse(inputValue);

                    long firstArgumentForOutput = (isFirstLoop ? parsedInput.FirstArgument : calculator.Result);

                    calculator.Process(parsedInput);

                    Console.WriteLine("\n{0}\t{1}\t{2}\t= {3}", firstArgumentForOutput, parsedInput.Operator, parsedInput.SecondArgument, calculator.Result);
                }

                isFirstLoop = false;
            }
            while (doLoop);

            Console.Write("\r\n\r\nThank you for using!");
            Console.Read();
        }
    }

    public class Calculator
    {
        private bool _isInitialized;

        public long Result { get; private set; }

        public void Process(ParseResult parsedInput)
        {
            if (!this._isInitialized)
            {
                this.Result = parsedInput.FirstArgument;

                this._isInitialized = true;
            }

            switch (parsedInput.Operator)
            {
                case "+":
                    this.Result += parsedInput.SecondArgument;
                    break;

                case "-":
                    this.Result -= parsedInput.SecondArgument;
                    break;

                case "*":
                    this.Result *= parsedInput.SecondArgument;
                    break;

                case "/":
                    this.Result /= parsedInput.SecondArgument;
                    break;

                case "%":
                    this.Result %= parsedInput.SecondArgument;
                    break;

                default:
                    break;
            }
        }
    }

    public class ParseResult
    {
        public long FirstArgument { get; set; }

        public string Operator { get; set; }

        public long SecondArgument { get; set; }
    }

    public class EquationParser
    {
        public ParseResult Parse(string input)
        {
            long firstArgument = 0;
            Match firstMatch = Regex.Match(input, "^\\d+");
            if (firstMatch.Success)
            {
                firstArgument = Convert.ToInt64(firstMatch.Value);
            }

            string operatorString = Regex.Match(input, "\\D+").Value;

            Match secondMatch = Regex.Match(input, "\\d+$");
            long secondArgument = Convert.ToInt64(secondMatch.Value);

            return new ParseResult()
                   {
                       FirstArgument = firstArgument,
                       Operator = operatorString,
                       SecondArgument = secondArgument
                   };
        }
    }
}

The significant differences to your solution are as follows:

• No hidden global state. Everything that is important can be followed on its way through the code, and each Calculator object completely controls everything its calculation depends on. You can now have hundreds of them at the same time, and they would work correctly, while with your Calculate class, they all would have shared and modified the same result.
• No hidden dependencies on other classes or static members. Your ESlicer class needs the Calculate class, but that is not apparent unless you actually happen to look into the code of that class and read that to the very last line.
• Testability. You would have no way to test the parsing of the input and the actual calculations separately in your program - you can only run the program, enter equations and look at the output to verify everything works correctly. I can use a unit test framework to write code that tests each of my classes independently - I can check if equations are parsed correctly, and I can check if all the calculations work without having to parse an equation first. Actually, both of these classes were written using the so-called Test-Driven Development (TDD) method, which is a great way to build code with a better structure and quality from the outset.
• Maintainability. When you find that something is not working correctly or need to extend the application (like adding 'power' as a new operation), it is very easy to make changes, because it's always very clear which parts of the code will be affected, and you can make changes with much less risk of breaking something, and thus with much greater confidence. This is also because you have no code duplications, so every change needs to be made in a single place only. (For example, my EquationParser class cuts quite a few corners with respect to input validation - I could now easily go ahead and do this, because I can be sure that I only need to make changes in that one class, and as long as it produces a correct ParseResult, the whole program will still work.)

If you want to learn how to write well-structured, maintainable and evolvable high-quality code, making yourself familiar with the SOLID principles and the aforementioned Test-Driven Development is the way to go (I learned most of what I know from a guy named Mark Seemann - look him up; also look on YouTube for talks from Miško Hevery). Those are certainly no easy topics and may seem daunting, but they are worth every minute spent on them, because it is incredibly rewarding to see those things play out when you get the hang of them. Also, try to keep your code as DRY as possible - DRY stands for "Don't Repeat Yourself" and means you should have as little duplicate code as possible, because duplicate code means more places to make changes, more places that can have bugs and more places to make mistakes.

Most importantly - practice. The more, the better. Read up on the topics I mentioned, and try following those principles. It will often seem impossible at first, but you'll get ahead eventually. And don't forget to come back here with your results and ask for advice, because trying to get your head around everything on your own is often very difficult.

---

[1] This would probably look slightly different if I was to write it as production code, but I wanted to keep it comprehensible in the first place.