7
\$\begingroup\$

I believe that building a simple calculator is one of the most popular tasks for beginners. There are several conditions, however, in this case:

  1. Delegates should be used
  2. The user types a string that should be parsed as a math expression with just two numbers and one of the four operators (+, -, *, /)
  3. My first idea of using the Compute method of DataTable was, predictably, rejected

I know that there is one serious flaw in the current version of the program. The input string is not validated. Apart from that, what are your recommendations and suggestions with regards to my code?

GitHub

using System;
namespace CalculatorDelegate
{
    class Program

    {
        private static double Item1 { get; set; }
        private static double Item2 { get; set; }
        private static string Action { get; set; }
        private delegate double Del(double a, double b);

        private static void Main(string[] args)
        {
            while (true)
            {
                Console.WriteLine(ParseInput());
            }
        }

        private static double ParseInput()
        {
            Console.WriteLine("Enter a mathematical expression, please");
            var userInput = Console.ReadLine();
            var userInputLength = userInput.Length;
            Del handler;

            for (var i = 1; i < userInputLength; i++)
            {

                if ( userInput[i].Equals('+') || userInput[i].Equals('-') ||
                    userInput[i].Equals('*') | userInput[i].Equals('/'))
                {
                        Action = Convert.ToString(userInput[i]);
                        double.TryParse(userInput.Substring(0, i), out var tempItem1);
                        double.TryParse(userInput.Substring(i + 1, userInputLength - i - 1), out var tempItem2);

                        Item1 = tempItem1;
                        Item2 = tempItem2;

                        if (Action.Equals("/") && Item2 == 0)
                        {
                            Console.WriteLine("You can't divide by zero");
                            return 0;
                        }

                        break;
                }
            }

            switch (Action)
            {
                case "+":
                    handler = GetSum;
                    break;

                case "-":
                    handler = GetDiff;
                    break;

                case "*":
                    handler = GetMult;
                    break;

                case "/":
                    handler = GetDiv;
                    break;

                default:
                    return 0;
            }

            return handler(Item1, Item2);
        }

        private static double GetSum(double a, double b)
        {
            return a + b;
        }

        private static double GetDiff(double a, double b)
        {
            return a - b;
        }

        private static double GetMult(double a, double b)
        {
            return a * b;
        }

        private static double GetDiv(double a, double b)
        {
            return a / b;
        }

    }
}
\$\endgroup\$
4
\$\begingroup\$

I completely agree with you that such tasks is a good opportunity to learn new concepts and the language itself. And, as you asked, I have a few comments on your code


Do not call variables by reserved names

I mean, I was sweating to understand what is going on here the first time

Action = Convert.ToString(userInput[i]);

Better to call such variable as

private static string OperationString { get; set; }

Split complex task on simpler ones - one logic per function

Your function ParseInput is doing not just parsing, but also reading from standard input. So split this task - ParseInput will just do parsing.

private static double ParseInput(string inputString)

Using the == operator to compare floating-point numbers is, generally, a bad idea

So instead of

if (Action.Equals("/") && Item2 == 0)

better to use this

if (Action.Equals("/") && Math.Abs(Item2) < Double.Epsilon)

Be aware of the difference between || and | logic operations

And without a need, do no mix up this two operators. So use

if ( userInput[i].Equals('+') || userInput[i].Equals('-') || userInput[i].Equals('*') || userInput[i].Equals('/'))

instead of

if ( userInput[i].Equals('+') || userInput[i].Equals('-') || userInput[i].Equals('*') | userInput[i].Equals('/'))

Better to avoid static not-readonly field

I don't see any actual reason for you to keep variables Item1, Item2 and OperationString as private static fields - make them as local variables. In general, based on my experience, many nasty bugs (especially in multi-thread applications) happen because of an inappropriate usage of static fields. I know, this problem is far away from this simple calculator application, but anyway.


Good regular expression would solve many problems, including

  • code will become simpler and cleaner, so easy to debug and read

  • easier to solve your "serious flaw" - input validation

I would suggest to use Regular Expressions instead of buggy for-if construction.

// \d+\.? supports xxx. and xxx
// \.\d+ - .xxx
// \d+\.\d+ - xxx.xxx
Regex InputRegex = new Regex(@"^\s*([-+]?)(\d+\.?|\.\d+|\d+\.\d+)(.)(\d+\.?|\.\d+|\d+\.\d+)\s*$");

// match.Groups[0] whole pattern
// match.Groups[1] sign of the first value
// match.Groups[2] first value (without sign)
// match.Groups[3] operation
// match.Groups[4] second value
Match match = InputRegex.Match(userInput);

if (!match.Success)
{
    // we didn't find a match
}
double item1 = match.Groups[1].Value == "-" ? -Double.Parse(match.Groups[2].Value) : Double.Parse(match.Groups[2].Value);
string actionString = match.Groups[3].Value;
double item2 = double.Parse(match.Groups[4].Value);

Let user know that calculation was successful or not

Let's check which error can happen in your code:

  • RegexNotSuccess Regular expression doesn't find a match
  • DivisionByZero Second item is 0, so we have the division by zero
  • NoSuchOperation User specified not [+-*/] operation

I would suggest to create enumerated list of those errors, return one enum element as output of your function.

public enum ErrorCode
{
    Success,
    NoSuchOperation,
    DivisionByZero,
    RegexNotSuccess
}

public static ErrorCode ParseInput(string userInput, out double result)
{
    //...
    if (!match.Success)
    {
        result = Double.MinValue;
        return ErrorCode.RegexNotSuccess;
    }
    //...
    switch (actionString)
    {
        //...
        case "/":
            if (Math.Abs(item2) < Double.Epsilon)
            {
                result = Double.MinValue;
                return ErrorCode.DivisionByZero;
            }
        //...
        default:
            result = Double.MinValue;
            return ErrorCode.NoSuchOperation;
    }
    result = handler(item1, item2);
    return ErrorCode.Success;
}

The final code looks like this

Program.cs

using System;

namespace CalculatorTest
{
    class Program
    {
        private static void Main()
        {
            while (true)
            {
                Console.WriteLine("Enter a mathematical expression, please");
                string userInput = Console.ReadLine();
                Calculator.ErrorCode stdout = Calculator.ParseTwoNumberOperation(userInput, out double result);
                if (stdout == Calculator.ErrorCode.Success)
                {
                    Console.WriteLine(result);
                }
                else
                {
                    Console.Error.WriteLine("Error: " + stdout + "\t" + userInput);
                }
            }
        }
    }
}

Calculator.cs

using System;
using System.Text.RegularExpressions;

namespace CalculatorTest
{
    public static class Calculator
    {
        public enum ErrorCode
        {
            Success,
            NoSuchOperation,
            DivisionByZero,
            RegexNotSuccess
        }

        private delegate double Handler(double a, double b);
        /// <summary>
        /// \d+\.? supports xxx. and xxx
        /// \.\d+ - .xxx
        /// \d+\.\d+ - xxx.xxx
        /// </summary>
        private static readonly Regex InputRegex = new Regex(@"^\s*([-+]?)(\d+\.?|\.\d+|\d+\.\d+)(.)(\d+\.?|\.\d+|\d+\.\d+)\s*$");
        public static ErrorCode ParseTwoNumberOperation(string userInput, out double result)
        {
            Handler handler;
            // match.Groups[0] whole pattern
            // match.Groups[1] sign of the first value
            // match.Groups[2] first value (without sign)
            // match.Groups[3] operation
            // match.Groups[4] second value
            Match match = InputRegex.Match(userInput);
            if (!match.Success)
            {
                result = Double.MinValue;
                return ErrorCode.RegexNotSuccess;
            }
            double item1 = match.Groups[1].Value == "-" ? -Double.Parse(match.Groups[2].Value) : Double.Parse(match.Groups[2].Value);
            string actionString = match.Groups[3].Value;
            double item2 = double.Parse(match.Groups[4].Value);
            switch (actionString)
            {
                case "+":
                    handler = GetSum;
                    break;

                case "-":
                    handler = GetDif;
                    break;

                case "*":
                    handler = GetMul;
                    break;

                case "/":
                    if (Math.Abs(item2) < Double.Epsilon)
                    {
                        result = Double.MinValue;
                        return ErrorCode.DivisionByZero;
                    }
                    handler = GetDiv;
                    break;

                default:
                    result = Double.MinValue;
                    return ErrorCode.NoSuchOperation;
            }

            result = handler(item1, item2);
            return ErrorCode.Success;
        }

        private static double GetSum(double a, double b) => a + b;
        private static double GetDif(double a, double b) => a - b;
        private static double GetMul(double a, double b) => a * b;
        private static double GetDiv(double a, double b) => a / b;
    }
}
\$\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.