4
\$\begingroup\$

I created this simple one line calculator for two numbers only where user can input any two numbers and and operator in between

for example 20+45 and computer will return result of 65.

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

namespace SimpleCalculator
{
    class Program
    {
        static void Main(string[] args)
        {
            ShowOutput();
            string user_input = UserInput();
            int result = PerformCalculation(InputToList(user_input));
            Console.WriteLine($"{user_input}={result}");
            Console.Read();            
        }

        static void ShowOutput()
        {
            Console.WriteLine("Enter numbers followed by operation eg. 1+2-4");
        }

        static string UserInput()
        {
            string User_input = Console.ReadLine();
            return User_input;
        }

        static string[] InputToList(string input)
        {
            string number1 = "";
            string number2 = ""; 
            string Oprt = ""; //Mathematical operator
            string[] Arithmetic = new string[3];
            int n = 0;
            foreach (char charecter in input)
            {
                int num;
                bool isNumerical = int.TryParse(charecter.ToString(), out num);
                n += 1;
                if (isNumerical)
                {
                    number1 += num;
                }
                else
                {
                    Oprt = charecter.ToString();
                    Arithmetic[0] = number1;
                    Arithmetic[1] = Oprt;
                    for(int i = n; i <= input.Length - 1; i++)
                    {
                        number2 += input[i];
                    }
                    Arithmetic[2] = number2;
                }

            }
            return Arithmetic;
        }

        static int PerformCalculation(string[] Input)
        {
            int result = 0;
            switch (Input[1])
            {
                case "+":
                    result = Int32.Parse(Input[0]) + Int32.Parse(Input[2]);
                    break;
                case "-":
                    result = Int32.Parse(Input[0]) - Int32.Parse(Input[2]);
                    break;
                case "*":
                    result = Int32.Parse(Input[0]) * Int32.Parse(Input[2]);
                    break;
                case "/":
                    result = Int32.Parse(Input[0]) / Int32.Parse(Input[2]);
                    break;
            }
            return result;
        }
    }
}
\$\endgroup\$
7
\$\begingroup\$

I'll go through your program from top to bottom, mentioning some details.

static void Main(string[] args)
{
    ShowOutput();
    string user_input = UserInput();
    int result = PerformCalculation(InputToList(user_input));
    Console.WriteLine($"{user_input}={result}");
    Console.Read();            
}

Starting with the Main method is good practice. The reader gets an overview about what the whole program is supposed to do.

Starting the program with ShowOutput() is confusing. The usual sequence of events is: Input — Processing — Output. By showing the output first, you are reversing this usual sequence of events.

static void ShowOutput()
{
    Console.WriteLine("Enter numbers followed by operation eg. 1+2-4");
}

Instead of ShowOutput this method should better be named ShowPrompt. This is more specific, and prompting the user clearly belongs to the input phase.

static string UserInput()
{
    string User_input = Console.ReadLine();
    return User_input;
}

Typically method names start with a verb, like in ShowOutput above. The method UserInput should rather be called ReadLine since that is exactly what happens here.

static string[] InputToList(string input)
{
    string number1 = "";
    string number2 = ""; 
    string Oprt = ""; //Mathematical operator
    string[] Arithmetic = new string[3];

Now it is getting more complicated, and inconsistencies appear. Some of your variables start with a lowercase letter, some with an uppercase letter. They should all start with a lowercase letter. That's by convention.

The usual abbreviation for operator is op. The letters prt remind me of the PrtScr key on the keyboard, which means Print Screen.

From reading the code until here, I have no idea what the variable Arithmetic might be. Granted, it's not easy to find a name for it. It could be something like "calculation atoms", "calculation parts", "words", "things", "tokens". When continuing to read the program, a better name might become apparent. This variable should then be renamed.

    int n = 0;
    foreach (char charecter in input)
    {
        int num;
        bool isNumerical = int.TryParse(charecter.ToString(), out num);
        n += 1;
        if (isNumerical)
        {
            number1 += num;
        }
        else
        {
            Oprt = charecter.ToString();
            Arithmetic[0] = number1;
            Arithmetic[1] = Oprt;
            for(int i = n; i <= input.Length - 1; i++)
            {
                number2 += input[i];
            }
            Arithmetic[2] = number2;
        }
    }

This code is long and tricky and fragile. In the upper part you parse number1 until you find a character (not charecter) that is not numeric. In that moment you save the current number1 into the result array. You can be lucky that you do this in exactly this moment, because later the variable number1 will be overwritten again, as soon as the number2 is parsed.

    return Arithmetic;
}

There must be some simpler way of expressing this idea. Imagine you explain this to a human friend. You would hopefully not choose to explain the code above, but some simpler code. To parse an expression that consists of numbers and operators:

  1. parse a number
  2. parse an operator
  3. parse a number
  4. continue with step 2

This is a high-level view on the whole topic of parsing an expression. That's how your code should look. The method should be called TryParseExpr. The current name InputToList is too unspecific.

static int PerformCalculation(string[] Input)

As Zoran already mentioned in his answer, performing a calculation on strings is inefficient and sounds strange. Calculations should be performed on numbers.

Converting the string parts into numbers and operators should be done by the TryParseExpr method I suggested above.

{
    int result = 0;
    switch (Input[1])
    {
        case "+":
            result = Int32.Parse(Input[0]) + Int32.Parse(Input[2]);
            break;
        case "-":
            result = Int32.Parse(Input[0]) - Int32.Parse(Input[2]);
            break;
        case "*":
            result = Int32.Parse(Input[0]) * Int32.Parse(Input[2]);
            break;
        case "/":
            result = Int32.Parse(Input[0]) / Int32.Parse(Input[2]);
            break;
    }
    return result;
}

This style of int result = 0; ...; result = the actual result; ...; return result; leads to long code. In most cases the code becomes easier to understand when the result is not saved in a variable but returned directly. Such as in:

static int Calculate(int left, char op, int right)
{
    switch (op)
    {
        case '+':
            return left + right;
        case '-':
            return left - right;
        case '*':
            return left * right;
        case '/':
            return left / right;
        default:
            throw new ArgumentException($"unknown operator {op}");
    }
}

This is as simple as it gets. There are no unnecessary arrays, and each parameter has the correct data type.

In your original code you suggest to the user that they may enter 1+2-4, but this is something your current code cannot handle. Given the above Calculate method, it's not too difficult to extend the calculation to an arbitrary amount of numbers and operators:

static int Calculate(IReadOnlyList<int> nums, IReadOnlyList<char> ops)
{
    int res = nums[0];
    for (int i = 0; i < ops.Count; i++)
        res = Calculate(res, ops[i], nums[i + 1]);

    return res;
}

In this method the calculation is performed strictly from left to right. The usual operator precedence (* before +) is ignored. That's only for simplicity. It could be added later.

This extended Calculate method can be used like this:

// 1 + 2 - 4
Console.WriteLine(Calculate(new List<int> {1, 2, 4}, new List<char> {'+', '-'}));

Now the remaining task is to let the user enter a line and convert this line into these two lists of numbers and operators. This is the job of a lexer. A lexer takes a string (or another input source) and repeatedly looks at the beginning to split off a small piece of data, such as a number or an operator.

The general idea I outlined above in the 4 steps can be written in C# like this:

public bool TryParseExpr(out List<int> nums, out List<char> ops)
{
    nums = new List<int>();
    ops = new List<char>();

    if (!TryParseInt(out int firstNum))
        return false;
    nums.Add(firstNum);

    while (TryParseOp(out char op))
    {
        ops.Add(op);

        if (!TryParseInt(out int num))
            return false;
        nums.Add(num);
    }

    return true;
}

Each of the paragraphs in this method corresponds roughly to one of the steps from above. Here they are again, for comparison:

  1. parse a number
  2. parse an operator
  3. parse a number
  4. continue with step 2

Now the only missing part are the basic building blocks, TryParseInt and TryParseOp. These I present together with the whole program that I built from your code:

using System;
using System.Collections.Generic;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace Tests
{
    [TestClass]
    public class Program
    {
        [TestMethod]
        public void Test()
        {
            TestOk("1", 1);
            TestOk("12345", 12345);
            TestOk("12345+11111", 23456);
            TestOk("2147483647", int.MaxValue);
            TestOk("1+2+3+4+5+6", 21);
            TestOk("1+2-3+4-5+6*5", 25);

            TestError("2147483648", "2147483648");
            TestError("a", "a");
            TestError("1+2+3+4+5+a", "a");
        }

        static void TestOk(string input, int expected)
        {
            Lexer lexer = new Lexer(input);
            Assert.AreEqual(true, lexer.TryParseExpr(out List<int> nums, out List<char> ops));
            int result = Calculate(nums, ops);
            Assert.AreEqual(expected, result);
        }

        static void TestError(string input, string expectedRest)
        {
            Lexer lexer = new Lexer(input);
            Assert.AreEqual(false, lexer.TryParseExpr(out List<int> nums, out List<char> ops));
            Assert.AreEqual(expectedRest, lexer.Rest);
        }

        static int Calculate(IReadOnlyList<int> nums, IReadOnlyList<char> ops)
        {
            int res = nums[0];
            for (int i = 0; i < ops.Count; i++)
                res = Calculate(res, ops[i], nums[i + 1]);

            return res;
        }

        static int Calculate(int left, char op, int right)
        {
            switch (op)
            {
                case '+':
                    return left + right;
                case '-':
                    return left - right;
                case '*':
                    return left * right;
                case '/':
                    return left / right;
                default:
                    throw new ArgumentException($"unknown operator {op}");
            }
        }
    }

    // The lexer takes a string and repeatedly converts the text at the
    // current position into a useful piece of data, like a number or an
    // operator.
    //
    // To do this, it remembers the whole text and the current position
    // of the next character to read. It also remembers the length of the
    // text, but this is only for performance reasons, to avoid asking for
    // text.Length again and again.
    class Lexer
    {
        private readonly string text;
        private int pos;
        private readonly int end;

        public Lexer(string text)
        {
            this.text = text;
            end = text.Length;
        }

        public string Rest => text.Substring(pos);

        public void SkipSpace()
        {
            while (pos < end && char.IsWhiteSpace(text[pos]))
                pos++;
        }

        public bool TryParseInt(out int num)
        {
            int i = pos;

            // The number may have a single sign.
            if (i < end && (text[i] == '-' || text[i] == '+'))
                i++;

            // After that, an arbitrary number of digits.
            while (i < end && char.IsDigit(text[i]))
                i++;

            // The TryParse handles the case of too many digits (overflow).
            bool ok = int.TryParse(text.Substring(pos, i - pos), out num);
            if (ok)
                pos = i;

            return ok;
        }

        public bool TryParseOp(out char op)
        {
            if (pos < end)
            {
                switch (text[pos])
                {
                    case '+':
                    case '-':
                    case '*':
                    case '/':
                        op = text[pos];
                        pos++;
                        return true;
                }
            }

            op = '\0';
            return false;
        }

        public bool TryParseExpr(out List<int> nums, out List<char> ops)
        {
            nums = new List<int>();
            ops = new List<char>();

            if (!TryParseInt(out int firstNum))
                return false;
            nums.Add(firstNum);

            while (TryParseOp(out char op))
            {
                ops.Add(op);

                if (!TryParseInt(out int num))
                    return false;
                nums.Add(num);
            }

            return true;
        }
    }
}

You can play around with this code by adding more and more test cases. There's also a method called SkipSpace that is currently unused. To allow the user to enter 1 + 2 - 4 as well, your parsing code should skip the space before and after each number or operator.

\$\endgroup\$
  • \$\begingroup\$ Thank you for in depth review :), will check that and come back with comment. \$\endgroup\$ – AMJ Apr 21 at 10:46
  • \$\begingroup\$ Thanks again for reviewing, I've been through your comments everything is understood but got few questions. in the function TryParseInt which return bool, how could it be possible to test if that function is true of false in the function itself, as in the expression TryParseInt(out int firstNum) \$\endgroup\$ – AMJ Apr 22 at 14:27
  • 1
    \$\begingroup\$ I'm not sure I understand your question correctly, I'll try to answer anyway. Inside the TryParseInt function, there is only a single return statement. To see whether the function returns true or false, you can look at the ok variable, as I have done already in the code. The if (ok) pos = i; is exactly such a test. In depth, it means: only if the TryParseInt function succeeds, does the state of the lexer change. If it fails, the lexer is left completely unchanged, so that the caller code could try some other parsing function. \$\endgroup\$ – Roland Illig Apr 22 at 16:42
  • \$\begingroup\$ Can you please tell what is => called and what it does in rest field \$\endgroup\$ – AMJ Apr 24 at 14:29
  • \$\begingroup\$ The public Rest => "" defines a property called Rest. Whenever the expression lexer.Rest is evaluated, the code to the right of the => is executed and returned. These properties can be used instead of methods. The common expectation is that evaluating a property is very fast and does not have side effects. Its value usually changes only when you modify something else in the class defining the property. Therefore, just as an example, having a property called Random is a bad idea. \$\endgroup\$ – Roland Illig Apr 24 at 16:27
3
\$\begingroup\$

In PerformCalculation function, you are repeating argument parsing several times. You should really apply processing only once, for the sake of code maintenance:

static int PerformCalculation(string[] Input)
{
    int left = int.Parse(Input[0]);
    int right = int.Parse(Input[2]);
    switch (Input[1])
    {
        case "+": return left + right;
        case "-": return left - right;
        case "*": return left * right;
        default: return left / right; // Mind possible division by zero
    }
}

Mind that the switch is more compact if it returns results (without break).

Also, note that the InputToList method an be made much more compact if you know the format of the expression:

static string[] InputToList(string input)
{
    int opIndex = input.IndexOfAny(new[] {'+', '-', '*', '/'});
    return new[]
    {
        input.Substring(0, opIndex), 
        input.Substring(opIndex, 1), 
        input.Substring(opIndex + 1)
    };
}

The IndexOfAny method is returning the first index inside string at which any of the listed characters appears.

The Substring method returns a new string which is the section of the original string. Please refer to documentation: https://docs.microsoft.com/en-us/dotnet/api/system.string.substring?view=netframework-4.8

It is tempting to use the string.Split() method for this same purpose:

static string[] InputToList(string input) =>
    input.Split('+', '-', '*', '/');

However, Split actually drops the delimiter. Hence, the string "25+36" would produce an array ["25", "36"], and the operator would be lost.

\$\endgroup\$
  • \$\begingroup\$ Thanks for reviewing, can you explain more about how you used Substring method, is it like splicing a string into array? \$\endgroup\$ – AMJ Apr 21 at 10:46
  • \$\begingroup\$ @AMJ I've added some explanations regarding substrings in the answer. \$\endgroup\$ – Zoran Horvat Apr 21 at 12:57
  • \$\begingroup\$ Thank you for explanation, from what I understood i created the following test code (I dont have access to visual studio right now so i used online compiler) I will tweak the actual code later. ` \$\endgroup\$ – AMJ Apr 22 at 12:07
2
\$\begingroup\$

To add some details to the other great answers:

  1. In modern C# it's common to use var to initalize local variables (var i = 0; instead of int i = 0;).

  2. Don't mix different naming styles. Snake case (user_input) is uncommon and doesn't match the official C# code style. For classes, methods and properties use UpperCamelCase and for local variables use lowerCamelCase.

  3. You got a typo in your foreach loop. I'd write it this way: for (var character in input) {...}

\$\endgroup\$
  • 1
    \$\begingroup\$ for (var char in input) - this won't work because char is a reserved keyword. \$\endgroup\$ – t3chb0t Apr 22 at 8:09
  • \$\begingroup\$ can i assign any value to var type variable ? \$\endgroup\$ – AMJ Apr 22 at 14:30
  • \$\begingroup\$ @t3chb0t thank you. Fixed. \$\endgroup\$ – Emaro Apr 22 at 16:28
  • 1
    \$\begingroup\$ @AMJ, yes, apart from null values. If the type is clear, it works. For null it cannot derive the type so you have to explicitly define it. However var is not a type itself, it's just a way to shorten verbose code. \$\endgroup\$ – Emaro Apr 22 at 16:29
2
\$\begingroup\$

I would use regular expressions to make a simple calculator that can only perform one binary operation. There is less overhead with tokenizing and casting. If you do decide to keep making calculator more and more complex, I suggest using the lexer-parser approach instead.

using System;
using System.Linq;
using System.Text;
using System.Collections.Generic;
using System.Globalization;
using System.Text.RegularExpressions;

public class Program
{
    public static void Main()
    {
        Console.WriteLine("1+4 = " + Evaluate("1+4"));
        Console.WriteLine("1-4 = " + Evaluate("1-4"));
        Console.WriteLine("1*4 = " + Evaluate("1*4"));
        Console.WriteLine("1/4 = " + Evaluate("1/4"));

        Console.ReadKey();
    }

    public static decimal Evaluate(string expression) 
    {
        var tokens = Regex.Match(expression, @"^(?<leftoperand>\d+)(?<operator>[+-/*])(?<rigthoperand>\d+)$");
        var leftoperandToken = tokens.Groups["leftoperand"];
        var operatorToken = tokens.Groups["operator"];
        var rigthoperandToken = tokens.Groups["rigthoperand"];

        if (!leftoperandToken.Success) {
            throw new FormatException("left operand could not be parsed");
        }

        if (!operatorToken.Success) {
            throw new FormatException("operator could not be parsed");
        }

        if (!rigthoperandToken.Success) {
            throw new FormatException("right operand could not be parsed");
        }

        // at this point, the operands can be safe-casted to integers

        var left = int.Parse(leftoperandToken.Value);
        var right = int.Parse(rigthoperandToken.Value);
        var result = 0m;

        switch (operatorToken.Value) {
            case "*":
                result = left * right;
                break;
            case "/":
                result = Convert.ToDecimal(left) / right;
                break;
            case "-":
                result = left - right;
                break;
            case "+":
                result = left + right;
                break;
            default:
                // a little bit forward-compatible
                throw new FormatException(operatorToken.Value + " is an invalid operator");
        }

        return result;
    }
}
\$\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.