two integers one line calculator Planned maintenance scheduled April 23, 2019 at 23:30 UTC...

Is it dangerous to install hacking tools on my private linux machine?

Resize vertical bars (absolute-value symbols)

Does silver oxide react with hydrogen sulfide?

What is the chair depicted in Cesare Maccari's 1889 painting "Cicerone denuncia Catilina"?

Sally's older brother

Putting class ranking in CV, but against dept guidelines

As a dual citizen, my US passport will expire one day after traveling to the US. Will this work?

One-one communication

Why is std::move not [[nodiscard]] in C++20?

Co-worker has annoying ringtone

Would color changing eyes affect vision?

Random body shuffle every night—can we still function?

"klopfte jemand" or "jemand klopfte"?

Test print coming out spongy

How does light 'choose' between wave and particle behaviour?

Central Vacuuming: Is it worth it, and how does it compare to normal vacuuming?

Is there public access to the Meteor Crater in Arizona?

Why not send Voyager 3 and 4 following up the paths taken by Voyager 1 and 2 to re-transmit signals of later as they fly away from Earth?

RSA find public exponent

Mounting TV on a weird wall that has some material between the drywall and stud

How to write capital alpha?

Why is it faster to reheat something than it is to cook it?

Why complex landing gears are used instead of simple,reliability and light weight muscle wire or shape memory alloys?

How does the math work when buying airline miles?



two integers one line calculator



Planned maintenance scheduled April 23, 2019 at 23:30 UTC (7:30pm US/Eastern)
Announcing the arrival of Valued Associate #679: Cesar Manara
Unicorn Meta Zoo #1: Why another podcast?Enum vs int wrapper structAm I coding Java in C#?Converting from binary to unaryFor each line in a file, read two integers and output the minimumTwo times same code in methode except one little lineProject Euler #49 Prime permutationsHackerEarth - SimpleFunctionLeetcode 49: Group Anagrams - Hash function design talkLeetcode 10: Regular Expression MatchingRecursive function challenge





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty{ margin-bottom:0;
}







1












$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;
}
}
}









share|improve this question









$endgroup$



















    1












    $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;
    }
    }
    }









    share|improve this question









    $endgroup$















      1












      1








      1





      $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;
      }
      }
      }









      share|improve this question









      $endgroup$




      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;
      }
      }
      }






      c#






      share|improve this question













      share|improve this question











      share|improve this question




      share|improve this question










      asked 4 hours ago









      AMJAMJ

      334




      334






















          2 Answers
          2






          active

          oldest

          votes


















          2












          $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.






          share|improve this answer








          New contributor




          Zoran Horvat is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
          Check out our Code of Conduct.






          $endgroup$





















            0












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






            share|improve this answer









            $endgroup$














              Your Answer






              StackExchange.ifUsing("editor", function () {
              StackExchange.using("externalEditor", function () {
              StackExchange.using("snippets", function () {
              StackExchange.snippets.init();
              });
              });
              }, "code-snippets");

              StackExchange.ready(function() {
              var channelOptions = {
              tags: "".split(" "),
              id: "196"
              };
              initTagRenderer("".split(" "), "".split(" "), channelOptions);

              StackExchange.using("externalEditor", function() {
              // Have to fire editor after snippets, if snippets enabled
              if (StackExchange.settings.snippets.snippetsEnabled) {
              StackExchange.using("snippets", function() {
              createEditor();
              });
              }
              else {
              createEditor();
              }
              });

              function createEditor() {
              StackExchange.prepareEditor({
              heartbeatType: 'answer',
              autoActivateHeartbeat: false,
              convertImagesToLinks: false,
              noModals: true,
              showLowRepImageUploadWarning: true,
              reputationToPostImages: null,
              bindNavPrevention: true,
              postfix: "",
              imageUploader: {
              brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
              contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
              allowUrls: true
              },
              onDemand: true,
              discardSelector: ".discard-answer"
              ,immediatelyShowMarkdownHelp:true
              });


              }
              });














              draft saved

              draft discarded


















              StackExchange.ready(
              function () {
              StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f217792%2ftwo-integers-one-line-calculator%23new-answer', 'question_page');
              }
              );

              Post as a guest















              Required, but never shown

























              2 Answers
              2






              active

              oldest

              votes








              2 Answers
              2






              active

              oldest

              votes









              active

              oldest

              votes






              active

              oldest

              votes









              2












              $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.






              share|improve this answer








              New contributor




              Zoran Horvat is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
              Check out our Code of Conduct.






              $endgroup$


















                2












                $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.






                share|improve this answer








                New contributor




                Zoran Horvat is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                Check out our Code of Conduct.






                $endgroup$
















                  2












                  2








                  2





                  $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.






                  share|improve this answer








                  New contributor




                  Zoran Horvat is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                  Check out our Code of Conduct.






                  $endgroup$



                  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.







                  share|improve this answer








                  New contributor




                  Zoran Horvat is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                  Check out our Code of Conduct.









                  share|improve this answer



                  share|improve this answer






                  New contributor




                  Zoran Horvat is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                  Check out our Code of Conduct.









                  answered 2 hours ago









                  Zoran HorvatZoran Horvat

                  1212




                  1212




                  New contributor




                  Zoran Horvat is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                  Check out our Code of Conduct.





                  New contributor





                  Zoran Horvat is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                  Check out our Code of Conduct.






                  Zoran Horvat is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
                  Check out our Code of Conduct.

























                      0












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






                      share|improve this answer









                      $endgroup$


















                        0












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






                        share|improve this answer









                        $endgroup$
















                          0












                          0








                          0





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






                          share|improve this answer









                          $endgroup$



                          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.







                          share|improve this answer












                          share|improve this answer



                          share|improve this answer










                          answered 1 hour ago









                          Roland IlligRoland Illig

                          11.7k11947




                          11.7k11947






























                              draft saved

                              draft discarded




















































                              Thanks for contributing an answer to Code Review Stack Exchange!


                              • Please be sure to answer the question. Provide details and share your research!

                              But avoid



                              • Asking for help, clarification, or responding to other answers.

                              • Making statements based on opinion; back them up with references or personal experience.


                              Use MathJax to format equations. MathJax reference.


                              To learn more, see our tips on writing great answers.




                              draft saved


                              draft discarded














                              StackExchange.ready(
                              function () {
                              StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f217792%2ftwo-integers-one-line-calculator%23new-answer', 'question_page');
                              }
                              );

                              Post as a guest















                              Required, but never shown





















































                              Required, but never shown














                              Required, but never shown












                              Required, but never shown







                              Required, but never shown

































                              Required, but never shown














                              Required, but never shown












                              Required, but never shown







                              Required, but never shown







                              Popular posts from this blog

                              “%fieldName is a required field.”, in Magento2 REST API Call for GET Method Type The Next...

                              How to change City field to a dropdown in Checkout step Magento 2Magento 2 : How to change UI field(s)...

                              變成蝙蝠會怎樣? 參考資料 外部連結 导航菜单Thomas Nagel, "What is it like to be a...