two integers one line calculator
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty{ margin-bottom:0;
}
$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;
}
}
}
c#
$endgroup$
add a comment |
$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;
}
}
}
c#
$endgroup$
add a comment |
$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;
}
}
}
c#
$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#
c#
asked 7 hours ago
AMJAMJ
334
334
add a comment |
add a comment |
2 Answers
2
active
oldest
votes
$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.
New contributor
$endgroup$
add a comment |
$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:
- parse a number
- parse an operator
- parse a number
- 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:
- parse a number
- parse an operator
- parse a number
- 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.
$endgroup$
add a comment |
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
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
$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.
New contributor
$endgroup$
add a comment |
$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.
New contributor
$endgroup$
add a comment |
$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.
New contributor
$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.
New contributor
New contributor
answered 6 hours ago
Zoran HorvatZoran Horvat
1212
1212
New contributor
New contributor
add a comment |
add a comment |
$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:
- parse a number
- parse an operator
- parse a number
- 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:
- parse a number
- parse an operator
- parse a number
- 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.
$endgroup$
add a comment |
$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:
- parse a number
- parse an operator
- parse a number
- 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:
- parse a number
- parse an operator
- parse a number
- 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.
$endgroup$
add a comment |
$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:
- parse a number
- parse an operator
- parse a number
- 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:
- parse a number
- parse an operator
- parse a number
- 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.
$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:
- parse a number
- parse an operator
- parse a number
- 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:
- parse a number
- parse an operator
- parse a number
- 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.
answered 5 hours ago
Roland IlligRoland Illig
11.7k11947
11.7k11947
add a comment |
add a comment |
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.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
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