Basic C# calculator (+,-,*,/) - V2

Question

Original question:

Basic C# calculator (+,-,*,/)

My 'teacher' requested this problem be solved by parsing the input into a tree based on the order of operations. The class Calc variables are specific to his instruction (except precedence) as well as the getValue() method. Everything else is subject to review.

I'm hoping to receive detailed input on how to make my code meet best practices as closely as possible in the perspective of a much larger program to be more familiar with programs to come.

Program.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace calcTree
{
 class Program
 {
 static void Main(string[] args)
 {
 // Various prompt and response strings
 const string WELCOME = "Welcome to the Calculator!";
 const string INSTRUCTIONS = "\nPlease enter an equation (do not use '=' or parenthesis):" +
 "\nEnter \"exit\" or \"bye\" to quit.\n";
 const string INVALID_INPUT = "\nYour input was invalid.";
 const string EXIT = "\nGoodbye!";
 string[] EXIT_KEY = { "exit", "bye" };
 Console.WriteLine(WELCOME);
 while (true)
 {
 Console.WriteLine(INSTRUCTIONS);
 string equation = Console.ReadLine(); // Read equation
 equation = equation.Replace(" ", String.Empty); // Remove spaces
 // Check for program exit keywords
 if (equation.EndsWith(EXIT_KEY[0], StringComparison.OrdinalIgnoreCase) ||
 equation.EndsWith(EXIT_KEY[1], StringComparison.OrdinalIgnoreCase))
 {
 Console.WriteLine(EXIT);
 break;
 }
 // Validate the equation
 else if (!Calc.ValidateEquation(equation))
 {
 Console.WriteLine(INVALID_INPUT);
 }
 // Solve the equation
 else
 {
 string[] answers = Calc.Solve(equation);
 for (int i = 0; i < answers.Length; i++) // Print the answer
 {
 Console.WriteLine(answers[i]);
 }
 }
 }
 Console.Read(); // Waits for 'enter' to end.
 }
 }
}

CalcTree.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace calcTree
{
 class Calc
 {
 char oper;
 decimal num;
 int precedence; // Assists with print order, not the actual calculation.
 Calc left;
 Calc right;
 /// <summary>
 /// Tests for NULL, that all chars are numbers or operators and that the last
 /// char is a number.
 /// </summary>
 /// <param name="equation">User input equation</param>
 /// <returns>True if valid</returns>
 public static bool ValidateEquation(string equation)
 {
 if (String.IsNullOrWhiteSpace(equation) || // test for null
 !Char.IsNumber(equation[equation.Length-1])) // last char must be a number
 {
 return false;
 }
 // All chars must be a number or operator
 for (int i = 0; i < equation.Length; i++)
 {
 if (!Char.IsNumber(equation[i]))
 {
 if (equation[i] != '*' &&
 equation[i] != '/' &&
 equation[i] != '+' &&
 equation[i] != '-' &&
 equation[i] != '.')
 {
 return false;
 }
 }
 }
 return true;
 }
 /// <summary>
 /// Solves the equation
 /// </summary>
 /// <param name="equation">String of valid numbers and operators</param>
 /// <returns>String[] of the solution in the order of operations</returns>
 public static string[] Solve(string equation)
 {
 string[] parsedInput = ParseEquation(equation);
 Calc[] logicTree = AllocateLogicTree(parsedInput);
 logicTree = ApplyLeftLogic(logicTree);
 logicTree = ApplyRightLogic(logicTree);
 string[] answer = SolveLogicTree(logicTree);
 return answer;
 }
 /// <summary>
 /// Parse input string into an array so that assignments to the logic tree are clear.
 /// </summary>
 /// <param name="equation">User input equation</param>
 /// <returns>string[] containing separated numbers and operators</returns>
 static string[] ParseEquation(string equation)
 {
 int size = 0; // "word" count for array size
 for (int i = 0; i < equation.Length; i++)
 {
 if (i + 1 < equation.Length)
 {
 if (equation[i + 1] == '*' || equation[i + 1] == '/' || equation[i + 1] == '+' || equation[i + 1] == '-')
 size++;
 }
 }
 size = size * 2 + 1; // operators * 2 + 1 == array positions required
 string[] parsedInput = new string[size]; // Allocates array size
 // Parse string into array by numbers and operators
 StringBuilder sb = new StringBuilder();
 size = 0;
 for (int j = 0; j < equation.Length; j++)
 {
 if (equation[j] != '*' && equation[j] != '/' && equation[j] != '+' && equation[j] != '-')
 {
 sb.Append(equation[j]);
 }
 else
 {
 parsedInput[size] = sb.ToString(); // Add number to array
 sb.Clear();
 size++;
 parsedInput[size] = equation[j].ToString(); // Add operator to array
 size++;
 }
 }
 parsedInput[size] = sb.ToString(); // Add last number to array
 return parsedInput;
 }
 /// <summary>
 /// Assigns string[] to eqaul sized Calc[], string[] value is either Calc.oper or Calc.num.
 /// </summary>
 /// <param name="parsedEquation">String[] containing separated numbers and operators</param>
 /// <returns>Calc[] with either .num or .oper assigned to each index</returns>
 static Calc[] AllocateLogicTree(string[] parsedEquation)
 {
 Calc[] logicTree = new Calc[parsedEquation.Length];
 for (int i = 0; i < parsedEquation.Length; i++)
 logicTree[i] = new Calc();
 for (int j = 0; j < parsedEquation.Length; j++)
 {
 if (parsedEquation[j] == "*" || parsedEquation[j] == "/" || parsedEquation[j] == "+" || parsedEquation[j] == "-")
 {
 logicTree[j].oper = Char.Parse(parsedEquation[j]);
 }
 else
 {
 logicTree[j].num = Decimal.Parse(parsedEquation[j]);
 }
 }
 return logicTree;
 }
 /// <summary>
 /// Applies the order of operations logic for the .left branches.
 /// </summary>
 /// <param name="logicTree">Each index contains .num or .oper</param>
 /// <returns>logicTree with .left assigned and precedence.</returns>
 static Calc[] ApplyLeftLogic(Calc[] logicTree)
 {
 // .left requirements & assign initial values before .right can be determined for + and -.
 for (int i = 0; i < logicTree.Length; i++)
 {
 if (logicTree[i].oper == '*' || logicTree[i].oper == '/')
 {
 // Previous oper must be + or - // else .left = previous * or / oper
 if (i - 2 >= 0) // bounds checking
 {
 if (logicTree[i - 2].oper == '+' || logicTree[i - 2].oper == '-')
 logicTree[i].left = logicTree[i - 1];
 else // previous operator must be * or /
 logicTree[i].left = logicTree[i - 2];
 }
 else logicTree[i].left = logicTree[i - 1];
 logicTree[i].right = logicTree[i + 1]; // always
 logicTree[i].precedence = 1; // Calculate this 1st
 }
 else if (logicTree[i].oper == '+' || logicTree[i].oper == '-')
 {
 // Previous oper must not exist or link to previous + or -
 if (i - 2 >= 0) // bounds checking
 {
 for (int j = i - 2; j >= 0; j--)
 {
 if (logicTree[j].oper == '+' || logicTree[j].oper == '-')
 {
 logicTree[i].left = logicTree[j];
 break;
 }
 j--;
 }
 if (logicTree[i].left == null) // logicTree[l - 2] must be the last * or / & the correct assignment
 logicTree[i].left = logicTree[i - 2];
 }
 else logicTree[i].left = logicTree[i - 1];
 // wait to assign .right
 logicTree[i].precedence = 2; // Calculate this 2nd
 }
 }
 return logicTree;
 }
 /// <summary>
 /// Applies the order of operations logic for the .right branches.
 /// </summary>
 /// <param name="logicTree">Each index contains .num or .oper</param>
 /// <returns>logicTree with .right assigned.</returns>
 static Calc[] ApplyRightLogic(Calc[] logicTree)
 {
 // .right requirements for + and -
 for (int i = 1; i < logicTree.Length; i++)
 {
 if (logicTree[i].oper == '+' || logicTree[i].oper == '-')
 {
 // if tree.oper + 2 == * or /, check next tree.oper and assign .right to the last consecutive * or /
 if (i + 2 < logicTree.Length) // bounds checking
 {
 if (logicTree[i + 2].oper == '*' || logicTree[i + 2].oper == '/')
 {
 int j; // represents last * or /
 for (j = i + 2; j < logicTree.Length; j++) // assign .right to last consecutive * or /
 {
 if (logicTree[j].oper != '*' && logicTree[j].oper != '/')
 {
 logicTree[i].right = logicTree[j - 2];
 break;
 }
 j++;
 }
 if (logicTree[i].right == null) // if not assigned from the for loop, last * or / must be (o - 2)
 logicTree[i].right = logicTree[j - 2];
 }
 else logicTree[i].right = logicTree[i + 1];
 }
 else logicTree[i].right = logicTree[i + 1];
 }
 i++;
 }
 return logicTree;
 }
 /// <summary>
 /// Returns decimal value or if performed on an operator returns the solution of
 /// left (operator) right.
 /// </summary>
 /// <returns>num value or solution</returns>
 decimal GetValue()
 {
 if (oper == 0) // tests for operator
 return num;
 else
 switch (oper)
 { // recursively calculates down the "tree", logical order determined in applyLogic()
 case '+':
 return left.GetValue() + right.GetValue();
 case '-':
 return left.GetValue() - right.GetValue();
 case '*':
 return left.GetValue() * right.GetValue();
 case '/':
 return left.GetValue() / right.GetValue();
 default: return 0; // never returns 0
 }
 }
 /// <summary>
 /// Solves logicTree by the order of operations.
 /// </summary>
 /// <param name="logicTree">.left, .right, and precedence are assigned</param>
 /// <returns>string[] of answers in the order of operations</returns>
 static string[] SolveLogicTree(Calc[] logicTree)
 {
 int answersIndex = 1; // 1 extra index for final answer
 for (int i = 0; i < logicTree.Length; i ++)
 {
 if (logicTree[i].oper != '0円')
 answersIndex++;
 }
 string[] answers = new string[answersIndex];
 answersIndex = 0;
 int precedence = 1; // starts with * and /
 while (precedence != 3)
 {
 for (int j = 0; j < logicTree.Length; j++)
 {
 if (logicTree[j].oper != 0 && logicTree[j].precedence == precedence)
 answers[answersIndex] = "\n" + logicTree[j].left.GetValue() + " " + logicTree[j].oper + " " + logicTree[j].right.GetValue() + " = " + logicTree[j].GetValue();
 }
 precedence++;
 }
 answers[answers.Length - 1] = "\nThe answer is: " + logicTree[logicTree.Length - 2].GetValue();
 return answers;
 }
 }
}

• Jamal: The original code should still remain unchanged from answers, even for follow-up questions.

Answer 1

Just at a real quick glance, inside the loop in ValidateEquation, you access equation[i] many times.

 for (int i = 0; i < equation.Length; i++)
 {
 if (!Char.IsNumber(equation[i]))
 {
 if (equation[i] != '*' &&
 equation[i] != '/' &&
 equation[i] != '+' &&
 equation[i] != '-' &&
 equation[i] != '.')
 {
 return false;

I would consider using a local character variable so that you don't have to continually get the value from the string, but simply return it once.

for (int i = 0; i < equation.Length; i++)
{
 char character = equation[i]; 
 if (!Char.IsNumber(character))
 {
 if (character != '*' &&
 character != '/' &&
 character != '+' &&
 character != '-' &&
 character != '.')
 {
 return false;
 }

Also, I think most C# devs would expect ValidateEquation to be named IsValidEquation to indicate that it returns a boolean value.

• ehance: Thanks, I have implemented your suggestions in my code and above. I like making equation[i] more descriptive.

Answer 2

ParseEquation() method

You are iterating 2 times over the characters of equation. The first time to get the size of the array you wish to return, the second to parse the equation.

A better approach would be to use a ILIst<String>.

You are using a for loop where a foreach loop would be better, as you would have less to type.

Because the AllocateLogicTree is the only method using the output, we can let the ParseEquation method return an IEnumerable<String> which we can then use as input parameter for the AllocateLogicTree method.

After applying these, your method could be refactored to

static IEnumerable<String> ParseEquation(string equation)
{
 IList<String> parsedInput = new List<String>();
 StringBuilder sb = new StringBuilder();
 foreach (Char c in equation)
 {
 if (c != '*' && c != '/' && c != '+' && c != '-')
 {
 sb.Append(c);
 }
 else
 {
 parsedInput.Add(sb.ToString());
 parsedInput.Add(c.ToString());
 sb.Clear();
 }
 }
 parsedInput.Add(sb.ToString());
 return parsedInput;
}

AllocateLogicTree() method

Again you are iterating 2 times over the array. This isn't necessary.

You are using Char.Parse for strings where you know that it contains an operator. You can access the char by using the index.

You can use a foreach also.

As you aren't just allocationg an logic tree, but building one by filling with values a better name would be CreateLogicTree.

After applying these, your method could be refactored to

static Calc[] CreateLogicTree(IEnumerable<String> parsedEquation)
{
 IList<Calc> logicTree = new List<Calc>();
 foreach (String currentEquationItem in parsedEquation)
 {
 if (currentEquationItem == "*" || currentEquationItem == "/"
 || currentEquationItem == "+" || currentEquationItem == "-")
 {
 logicTree.Add(new Calc() { oper = currentEquationItem[0] });
 }
 else
 {
 logicTree.Add(new Calc() { num = Decimal.Parse(currentEquationItem) });
 }
 }
 return logicTree.ToArray();
}

This can be then called in the Solve() method like

 public static string[] Solve(string equation)
 {
 IEnumerable<String> parsedInput = ParseEquation(equation);
 Calc[] logicTree = CreateLogicTree(parsedInput);
 logicTree = ApplyLeftLogic(logicTree);
 logicTree = ApplyRightLogic(logicTree);
 string[] answer = SolveLogicTree(logicTree);
 return answer;
 }

• ehance: Thanks for really exposing to me the benefits of lists. Reduced my code by about 20 lines and I also implemented it in SolveLogicTree(). Great stuff!