Vjylku

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;

        }

    }

}

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.

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 = '';

            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.