10
\$\begingroup\$

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.

This is my first real program in C# but I'm hoping to receive detailed input on how to make my code meet best practices as closely as possible.

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

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
            string welcome = "Welcome to the Calculator!";
            string instructions = "\nPlease enter a space delimited equation (do not use \"x =\" or parenthesis)" +
                                  "\nor enter \"exit\" to quit:\n";
            string exit = "\nGoodbye.";
            string error = "\nYour input was invalid.";

            int end; // Determines when to exit the program

            Console.WriteLine(welcome);
            do
            {
                Console.WriteLine(instructions);

                // Read input & allocate parsed_input size
                string input = Console.ReadLine();

                // Removes spaces from input, turns it into an array, and checks for invalid input (exit value)
                string[] parsed_input = removeSpaces(input, out end);

                // Exit(1) or restart(2) the loop if input is invalid 
                if (end != 0)
                {
                    if (end == 1)
                        Console.WriteLine(exit);
                    else // 2
                        Console.WriteLine(error);
                }
                else
                {
                    // Create logic tree
                    Calc[] calc = Calc.applyLogic(parsed_input);

                    // Solve & Print Solution in order
                    Calc.solveAndPrint(calc);
                }
            } while (end != 1);
            Console.Read(); // Waits for 'enter' to end.
        } // End Main

        //*************************************************//
        // Parses a space delimited equation string into a //
        // string[] containing numbers and operators.      //
        //*************************************************//
        static string[] removeSpaces(string input, out int end)
        {
            // unused TryParse variables
            decimal fake_decimal;
            char fake_char;
            end = 0; // Reset for next input

            int size = 1;   // "word" count for parsed_input
            for (int i = 0; i < input.Length; i++)
                if (input[i] == ' ' && i != input.Length - 1)
                    size++;

            string[] parsed_input = new string[size]; // Allocates array size

            // Remove spaces from input
            StringBuilder sb = new StringBuilder();
            size = 0;
            for (int j = 0; j < input.Length; j++)
            {
                if (input[j] != ' ')
                    sb.Append(input[j]);
                if (input[j] == ' ' || j == input.Length - 1)
                {
                    parsed_input[size] = sb.ToString();
                    sb.Clear();

                    // Check for invalid input or "exit"
                    if (Decimal.TryParse(parsed_input[size], out fake_decimal) == false && Char.TryParse(parsed_input[size], out fake_char) == false)
                    {
                        if (parsed_input[size] == "exit" || parsed_input[size] == "Exit")
                            end = 1; // quits the application
                        else end = 2; // restarts the loop
                    }
                    size++;
                }
            }
            return parsed_input;
        }
    }
}

calcTree.cs

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

namespace calcTree
{
    class Calc
    {
        public char oper { get; set; }
        public decimal num { get; set; }
        public int precedence { get; set; } // Assists with print order, not the actual calculation.

        public Calc Left { get; set; }
        public Calc Right { get; set; }

        //******************************************************//
        // Returns decimal value or if performed on an operator //
        // returns the solution of Left (operator) Right.       //
        //******************************************************//
        public 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
                }
        }

        //*****************************************************//
        // Creates empty Calc object [] depending on size of   //
        // space delimited equation string, sends the array to //
        // applyLeftLogic() and applyRightLogic() so it can be //  
        // solved correctly.                                   //
        //*****************************************************//
        public static Calc[] applyLogic(string[] input)
        {
            Calc[] logicTree = new Calc[input.Length];
            for (int k = 0; k < input.Length; k++)
                logicTree[k] = new Calc();

            applyLeftLogic(logicTree, input);
            applyRightLogic(logicTree);         // Operators already copied so 'input' isn't needed

            return logicTree;
        } // End applyLogic(string[] input)

        //*********************************************************//
        // Applies order of operations logic to the .Left branches //
        // so that logic can then be applied to .Right correctly   //
        //*********************************************************//
        static Calc[] applyLeftLogic(Calc[] tree, string[] input)
        {
            // .Left requirements & assign initial values before .Right can be determined for + and -.
            for (int l = 0; l < tree.Length; l++)
            {
                if (input[l] == "*" || input[l] == "/")
                {
                    tree[l].oper = Char.Parse(input[l]);
                    // Previous oper must be + or - // else .Left = previous * or / oper
                    if (l - 2 >= 0) // bounds checking
                    {
                        if (tree[l - 2].oper == '+' || tree[l - 2].oper == '-')
                            tree[l].Left = tree[l - 1];
                        else // previous operator must be * or /
                            tree[l].Left = tree[l - 2];
                    }
                    else tree[l].Left = tree[l - 1];
                    tree[l].Right = tree[l + 1]; // always
                    tree[l].precedence = 1; // Calculate this first
                }
                else if (input[l] == "+" || input[l] == "-")
                {
                    tree[l].oper = Char.Parse(input[l]);
                    // Previous oper must not exist or link to previous + or -
                    if (l - 2 >= 0) // bounds checking
                    {
                        for (int m = l - 2; m >= 0; m--)
                        {
                            if (tree[m].oper == '+' || tree[m].oper == '-')
                            {
                                tree[l].Left = tree[m];
                                break;
                            }
                            m--;
                        }
                        if (tree[l].Left == null) // tree[l - 2] must be the last * or / & the correct assignment
                            tree[l].Left = tree[l - 2];
                    }
                    else tree[l].Left = tree[l - 1];
                    // wait to assign .Right
                    tree[l].precedence = 2; // Calculate this second
                }
                else tree[l].num = Decimal.Parse(input[l]);
            }
            return tree;
        } // End applyLeftLogic(Calc[] tree, string[] input)

        //**********************************************************//
        // Applies order of operations logic to the .Right branches //
        //**********************************************************//
        static Calc[] applyRightLogic(Calc[] tree)
        {
            // .Right requirements for + and -
            for (int n = 1; n < tree.Length; n++)
            {
                if (tree[n].oper == '+' || tree[n].oper == '-')
                {
                    // if tree.oper + 2 == * or /, check next tree.oper and assign .Right to the last consecutive * or /
                    if (n + 2 < tree.Length) // bounds checking
                    {
                        if (tree[n + 2].oper == '*' || tree[n + 2].oper == '/')
                        {
                            int o; // represents last * or /
                            for (o = n + 2; o < tree.Length; o++) // assign .Right to last consecutive * or /
                            {
                                if (tree[o].oper != '*' && tree[o].oper != '/')
                                {
                                    tree[n].Right = tree[o - 2];
                                    break;
                                }
                                o++;
                            }
                            if (tree[n].Right == null) // if not assigned from the for loop, last * or / must be (o - 2)
                                tree[n].Right = tree[o - 2];
                        }
                        else tree[n].Right = tree[n + 1];
                    }
                    else tree[n].Right = tree[n + 1];
                }
                n++;
            }
            return tree;
        } // End applyRightLogic(Calc[] tree)

        //****************************************************//
        // Solves and prints tree by the order of operations. //
        //****************************************************//
        public static void solveAndPrint(Calc[] tree)
        {
            int precedence = 1; // starts with * and /
            do
            {
                for (int p = 0; p < tree.Length; p++)
                {
                    if (tree[p].oper != 0 && tree[p].precedence == precedence)
                        Console.WriteLine("\n{0} {1} {2} = {3}", tree[p].Left.getValue(), tree[p].oper, tree[p].Right.getValue(), tree[p].getValue());
                }
                precedence++;
            } while (precedence != 3);
            Console.WriteLine("\nThe answer is: {0}", tree[tree.Length - 2].getValue());
        } // End solveAndPrint(Calc[] tree)
    }
}
\$\endgroup\$
10
\$\begingroup\$

Class Program

The removeSpaces() method is violating the single responsibility principle as it

  1. remove spaces from the given input
  2. checks for the word exit
  3. checks if the given expression contains a valid equotation

So these 3 responsibilities should be extracted to 3 methods.

Let us start with removing the spaces.

We just use the Replace() method

private static String RemoveSpaces(String input)
{
    return input.Replace(" ", String.Empty);
}

After we have removed any spaces we check for the word exit

As the instructions are clearly stating, entering the word exit and hitting enter should end the application. What do we need to do ? We only need to check if the last 4 characters of the input are the word exit in all the different writing types.

First we check if the length of the input is at minimum 4, so if the length is smaller than 4 we can't find the word exit.

Then we check if the last 4 characters of the input, converted to lower case is exit. To retrieve the last 4 characters we use the SubString() method. For converting the trimmed input to lower case we use the ToLowerInvariant(). Both of these methods are returning a new string.

private static bool ShouldExit(String input)
{
    if (input.Length < 4) { return false; }
    return input.Substring(input.Length - 4).ToLowerInvariant() == "exit";
}

Update
As mjolka has pointed out in the [chat] , this can be simplified using the EndsWith() method

private static bool ShouldExit(String input)  
{
   return input.EndsWith("exit", StringComparison.OrdinalIgnoreCase);
}

Now let us focus on validating the given input. As we don't have separating spaces anymore the current logic doesn't work anymore.

But the current logic had a flaw/bug in it, as 2 + 5 + 6 + shouldn't be valid.

As the validation of the equotation is related to the calculation, it should be part of the Calc class.

If one of the following condition is true, the equotation is invalid:

  1. The given input is null or whithespace
  2. The given input contains a char which isn't a number, an operator nor a culture specific decimal point
  3. The last char doesn't represent a number

As I don't want to give you the solution, I will give you just hints how to solve

  1. String.IsNullOrWhiteSpace() checks if a string is null or contains only whitespace characters.
  2. As you already know how to iterate through the chars of a String, you can use the Char.IsNumber() method to check if the char is a number.
    You can use the Contains() method with a char array (like myArray.Contains('x');). So providing a operator char array, can be used to check if a non number is an operator.
    In the System.Globalization namespace lives the NumberFormatInfo class which has a property NumberDecimalSeparator which provides, depending on the current culture, the decimal separator.
  3. Should be solvable by 2.

This method should just return a boolean. So we still need to parse the given input into a string array, to get the same result which the original code did return.

The flow can be like

If the current char is an operator, it can be added to the array (or better use a IList respective List).
If the char is either a number or a decimal separator it can be added to a separate array/List<T> until the current char is an operator. Then it can be added to the former array/List<T> and gets cleared afterwards.
The last step is after the e.g loop to add the remaining number or a decimal separator to the result value.

Please check your other methods also regarding SRP. Like e.g solveAndPrint().

\$\endgroup\$
8
\$\begingroup\$

I've a number of concerns about this code, but for a first attempt it's good enough if it works with the test inputs you've been given. Let's run through the code and see what comes up - good and bad. I'm going to be quite rough with you - but you'll be a better coder for it. I have included (incomplete) code examples, but not enough that you could copy/paste, they're also illustrations and not tested production code.

First off the code appears to run and is well formatted. I know it's only a matter of Ctrl+E-D in Visual Studio but you wouldn't believe how many people don't use it.

string welcome = "Welcome to the Calculator!";

There can be benefits to putting strings into variables, if they'll be reused throughout the code or changed in a multi-lingual environment. Neither of these are the case in this code so just Console.WriteLine("Whatever"); is fine. Writing two lines of code where one will do just hurts comprehension - when devs see Console.WriteLine(error); they'll need to remember what error is and if they need to modify it, it's more to remember and complexity is your enemy. If you needed to make your code multi-lingual this code would need to change anyway.

int end; // Determines when to exit the program

This is a warning sign. Using an int for controlling program flow means you have 'magic numbers' which are a warning sign in professional code because it means that a reader has to know what those numbers mean (and rely on them not to change). Unless you are actually doing some numerical work with end then you should use an enum or (better still) look at your program control to see if you actually need it.

do
{

You'll see this quite a lot in C and C# that devs don't like the do keyword. That's not as much about personal preferences as people make out, it hides the program flow. Making this code into

while (end != 1)
{

means that anyone following the code will know when it's going to end in advance. They can use this line to skim through to find out what code affects the exit condition. Using while in this way is also consistent with if and for branching/looping. Consistency helps you to make better code. Now what does that 1 mean (see what I mean about magic numbers.)

// Removes spaces from input, turns it into an array, and checks for invalid input (exit value)
string[] parsed_input = removeSpaces(input, out end);

The comment is a warning sign, it means that either the code coming up is unclear or that the comment is redundant. In this case the code is unclear - initially looking at it most programmers would see this as one of ...

string[] parsed_input = input.Split(new char[] {' '});
// or
string[] parsed_input = input.Replace(" ", "");

(...and another answer recommended something similar without reading the code.) The reason for this confusion is that your code has violated the SRP - Single Responsibility Principle. Give each routine one job (only) and if parts of that job can be further divided then that's ok so long as this one gives us the output we need here.

What is removeSpaces actually doing?

    //*************************************************//
    // Parses a space delimited equation string into a //
    // string[] containing numbers and operators.      //
    //*************************************************//

Well, no it's more than that according to the code - it's trying to:-

  • Validate the input text
  • Parse the text into an array
  • Control the program flow

Since program/UI flow is a job for Main and validation/parsing are jobs for the Calc class removeSpaces needs reworking. The base types like int have TryParse(string, out ...) and it makes sense to implement that in your Calc class. Reusing existing structures help make code easier to understand even though in this case the code has two outputs (validation AND parsing/conversion).

In the case of removeSpaces you have an opportunity to look at the Main routine and ask "What should it do?"

As pseudo code it might be... Show welcome message While the user wants to continue Get user input Validate it Valid: Give the answer Invalid: Complain Show summary message/goodbye

So if we look back to the SRP, Main might look something like...

Console.WriteLine("Welcome message and instructions");
while (true)
{
    string input = Console.ReadLine();
    if (input=="exit") break;
    Calc rootNode;
    if (Calc.tryParse(input, out rootNode))
    {
        Console.WriteLine("answer: {0}", rootNode.getValue());    
    }
    else
    {
        Console.WriteLine("invalid input");
    }
}
Console.WriteLine("goodbye");

This code is mostly UI related except where it populates and displays the Calc object, which it needs to get a result. I've also only populated one Calc object - that does not mean that the code cannot handle an actual tree structure, but we have one calculation and that should be the root of a tree structure. Anything else about how the calculation happens is not really Main's concern.

There are also no variables controlling the program flow in the above example. end from your code is gone and in it's place we have while (true) which loops forever and the break keyword which exits the current loop when it is executed. That leaves us with the simple bool result from Calc.TryParse(string, out Calc) which we don't need to store.

So... let's have a look at CalcTree.

public int precedence { get; set; } // Assists with print order, not the actual calculation.

So here again we have a violation of SRP, Calc should not really be sending output to the display, that is Main's job. Consider that Calc might be reused in an application that does not have console output like as part of an ASP page or desktop application. The inclusion of also indicates that Main and Calc might be too tightly coupled, that is to say they rely on each other too much. Calc should not care about what the program is doing and all the program should see is Calc being given input and providing a result.

    public char oper { get; set; }

There's not really much call for using { get; set; } unless you intend to put functions into them later. Also the naming for public items should be PascalCase rather than camelCase - it doesn't matter so long as it's consistent but most c# tend to stick to Microsoft Capitalization Styles so you may as well start using them now.

//******************************************************//
// Returns decimal value or if performed on an operator //
// returns the solution of Left (operator) Right.       //
//******************************************************//

Could be improved to be more useful. If you declare your function/method and then in the space above it type /// and it will auto-generate documentation for you enables built-in hints in the Visual Studio user interface.

/// <summary>
/// Performs the calculation. 
/// </summary>
/// <returns>Numeric value or the solution of the sum.</returns>
public decimal getValue()

getValue() as a name is also not very useful. What is it the value of for example? Is it the value it was given, is it the result of the calculation? It's not really clear. I know that's out of your hands...

ApplyLeftLogic and ApplyRightLogic could be significantly improved by changing how you break the string and create the nodes. They are very complicated for what should be a simple operation. This class is an example of a tree data structure where a node may have one or more 'child' nodes...

public class Item
{
    public string SomeInformation;
    public Item[] ChildItems;
}

Could this be something that would better represent your tree structure than the code you have?

There's a lot there to read through and I'm sure people will have comments if they get this far. Spend some time reading some of the many books on code quality, like Beautiful Code and Code Complete and if you make their recommendations habit now then it will serve you well later.

\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! Nice answer. \$\endgroup\$ – RubberDuck Oct 20 '14 at 18:59
  • \$\begingroup\$ @RubberDuck - thanks, I was concerned it was turning into a bit of an essay :) \$\endgroup\$ – James Snell Oct 20 '14 at 21:06
  • 2
    \$\begingroup\$ Answers here tend to be a bit... longer. =) \$\endgroup\$ – RubberDuck Oct 20 '14 at 21:10
  • 2
    \$\begingroup\$ +1 I agree with @RubberDuck, Good review. A quick nitpick, you have intput instead of input here Calc.tryParse(intput, out rootNode) ? \$\endgroup\$ – Heslacher Oct 21 '14 at 5:00
  • 1
    \$\begingroup\$ @Heslacher - well spotted. I'd love to say it was deliberate just to keep a student on their toes but who am I kidding! :) \$\endgroup\$ – James Snell Oct 21 '14 at 7:45
7
\$\begingroup\$

This is fairly good for a first program. I think you'll do well.

  • I like that you declared string variables instead of just printing them directly, but they don't (and shouldn't) change, so go ahead and declare them as Constants.

    string const exit = "\nGoodbye"
    
  • Personally, I prefer while loops over do...while. I think they're easier to read.

  • These if statements could be cleaned up with a switch. You've used one elsewhere, so I'll let you implement it.

                 if (end != 0)
                {
                    if (end == 1)
                        Console.WriteLine(exit);
                    else // 2
                        Console.WriteLine(error);
                }
                else
                {
                    // Create logic tree
                    Calc[] calc = Calc.applyLogic(parsed_input);
    
                     // Solve & Print Solution in order
                    Calc.solveAndPrint(calc);
                }
    
  • If you know you're not using a variable, go ahead and remove them.

         // unused TryParse variables
        decimal fake_decimal;
        char fake_char;
    
  • I also recommend getting into the habit of always using braces. It will save you some headaches down the road.

             if (input[i] == ' ' && i != input.Length - 1)
                size++;
    
  • Take some time to read up on Microsoft's Capitalization Guidelines. C# devs will find it odd to see snake_case names. Classes, Properties, and Methods should be PascalCased. Variables and arguments should be camelCased.

  • Don't be afraid to reuse variable names in different scopes. Your loop counters keep climbing the alphabet. They don't need to. You can read more about scope here.

  • I didn't dig too deep, but applyRightLogic looks awful similar to applyLeftLogic. Can you extract a method, or perhaps several?

There are other things that could be said, I'm sure. I didn't take a hard look at your algoritm. Honestly, I didn't think about it all. Perhaps a better programmer than I can give you some advice about that.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.