11
\$\begingroup\$

This is my basic C# calculator; I'm sure there is much that can be improved. This design is based on Bjarne Stroustrup's C++ calculator, a purposely be-bugged version of which can be found at his website.

enum Operators
{
    Addition, Subtraction, Multiplication, Division, Modulo
}

static class ExtensionMethods
{
    public static string RemoveAll(this string str, char toRemove)
    {
        for (int i = 0; i < str.Length; i++)
        {
            if (str[i] == toRemove)
            {
                str = str.Remove(i, 1);
                i--;
            }
        }
        return str;
    }
}

class Calculator
{
    private static StreamReader dataStream;

    private static Operators getNextOperator()
    {
        const string OperatorSet = "+-*/%";

        char token = (char)dataStream.Read();

        if (OperatorSet.IndexOf(token) == -1)
        {
            throw new InvalidDataException("Invalid or missing operator.");
        }
        return (Operators)OperatorSet.IndexOf(token);
    }

    private static double getNextNum()
    {
        string value = string.Empty;

        while (true)
        {
            if ("0123456789.".IndexOf((char)dataStream.Peek()) == -1)
            {
                break;
            }

            value += (char)dataStream.Read();
        }

        double d;
        if (double.TryParse(value, out d))
        {
            return d;
        }
        Console.WriteLine(value);
        throw new InvalidDataException("Expected a number.");
    }

    private static double NumbersAndParens()
    {
        bool valueIsNegative = false;
        double val = -1;    // trick the compiler

        if ((char)dataStream.Peek() == '-')
        {
            valueIsNegative = true;
            dataStream.Read();
        }

        if ((char)dataStream.Peek() == '(')
        {
            dataStream.Read();
            val = AdditiveOperators();

            if ((char)dataStream.Peek() == ')')
            {
                dataStream.Read();
                return valueIsNegative ? -1 * val : val;
            }
            else
            {
                throw new InvalidDataException("Expected closing parenthesis.");
            }
        }

        val = getNextNum();

        return valueIsNegative ? -1 * val : val;
    }

    private static double MultiplicativeOperators()
    {
        double val = NumbersAndParens();

        while (true)
        {
            if ("*/%".IndexOf((char)dataStream.Peek()) == -1)
            {
                return val;
            }

            Operators op = getNextOperator();

            if (op == Operators.Multiplication)
            {
                val *= NumbersAndParens();
            }
            else if (op == Operators.Division)
            {
                double nextNum = NumbersAndParens();
                if (nextNum == 0)
                {
                    throw new DivideByZeroException("Divide by 0 error.");
                }
                val /= nextNum;
            }
            else
            {
                val %= NumbersAndParens();
            }
        }
    }

    private static double AdditiveOperators()
    {
        double val = MultiplicativeOperators();

        while (true)
        {
            if ("-+".IndexOf((char)dataStream.Peek()) == -1)
            {
                return val;
            }
            Operators op = getNextOperator();

            if (op == Operators.Addition)
            {
                val += MultiplicativeOperators();
            }
            else if (op == Operators.Subtraction)
            {
                val -= MultiplicativeOperators();
            }
        }
    }

    private static Stream StringToStream(string equation)
    {
        MemoryStream stream = new MemoryStream();
        StreamWriter writer = new StreamWriter(stream);

        writer.Write(equation.RemoveAll(' '));
        writer.Flush();

        stream.Position = 0;
        return stream;
    }

    /// <summary>
    /// 
    /// </summary>
    /// <param name="equation">A mathematical epxression needing evalation.</param>
    /// <returns>The evaluated expression.</returns>
    public static double EvaluateExpression(string equation)
    {
        dataStream = new StreamReader(StringToStream(equation));

        double evaluation = AdditiveOperators();

        if (!dataStream.EndOfStream)
        {
            throw new InvalidDataException("Invalid expression.");
        }

        return evaluation;
    }
}
\$\endgroup\$
11
+50
\$\begingroup\$

How do you even use this code? You read through 5 different methods, each doing something specific -- How do you determine which one is used?


The RemoveAll extension is expensive: you're creating a new string for each removal of a character. I would suggest a different approach where you unwrap the string into an array of characters, take all the characters that you want to keep and put those in a new array, which you then use to create a new string.

In code:

var input = "19*cqwcq521sq0dqs0-dqs44sq";
var toRemove = 'c';
var result = new string(input.ToCharArray().Where(x => x != toRemove).ToArray());

Why is everything static? Your methods use a shared resource (the StreamReader) which should indicate that it is in fact anything but static.


C# naming conventions dictate UpperCamelCase for methods.


if (OperatorSet.IndexOf(token) == -1)
{
    throw new InvalidDataException("Invalid or missing operator.");
}
return (Operators)OperatorSet.IndexOf(token);

I don't like double work. Store the IndexOf result in an intermediate variable.


getNextNum()

Num is not an an acceptable combination in Scrabble thus we don't allow it here either. Write it in full.


 if (OperatorSet.IndexOf(token) == -1)

also known as

 if(!"+-*/%".Contains(token))

If you're going to use this extensively, consider extension methods on char: IsOperator(), IsDigit(), etc.

public static bool IsDigit(this char c)
{
    return "123456789".Contains(c);
}

while (true)
{
    if ("0123456789.".IndexOf((char)dataStream.Peek()) == -1)
    {
        break;
    }

    value += (char)dataStream.Read();
}

It's time for a fun edge case!

Question: what happens when you have a reaaaaaaaaally big number? I'm talking reaaaaaaallllllyyyyy big. I'm talking 308 characters big.

That's right, it will be a correct value but your code will throw an InvalidDataException when it cannot parse it as a double. Likewise: if you have int.MaxValue characters in your string, you're going to end up with trouble as well. Though at that point, your memory will have already been depleted so it's "okay".

Unless you're building something aimed at these usecases: don't fix it. I just thought it worth mentioning.


You are using string concatenation instead of a StringBuilder though. Don't use concatenation in a loop.


Console.WriteLine(value);
throw new InvalidDataException("Expected a number.");

Don't do any (useless?) output from your calculator. That's a job for whatever classes consumes your calculator.


valueIsNegative

should be

isNegative

double val = -1;    // trick the compiler

You're also tricking the handsome reviewer. Why are you doing this?


if ((char)dataStream.Peek() == '-')
{
    valueIsNegative = true;
    dataStream.Read();
}

What if I have --9 as input? That's positive, yet you'll tell me it's negative.


Why are you using .Peek() to get the value and then placing a useless call to .Read() to discard of the value? That's not how a stream is intended to be used: simply call .Read() from the start then.


if ((char)dataStream.Peek() == '(')
{
    dataStream.Read();
    val = AdditiveOperators();

Why do you assume that after an open parenthesis, I will have an "additive" operator?


MultiplicativeOperators()

There's no such thing as that. Either it's additive, subtractive, multiplicative, divisive or any other kind of -ive. You're grouping them together for the wrong relation.


NumbersAndParens()

Methods describe an action. This does not describe an action.


else
{
    val %= NumbersAndParens();
}

You're having a fall-through statement in code that is very weakly connected: a small mistake in one of several places and suddenly you have a really hard to track bug. Use an explicit else if(op == Operator.Modulo) branch and add a fall-through exception.


I don't see any difference between MultiplicativeOperators() and AdditiveOperators() that warrants them being in different methods.


MemoryStream stream = new MemoryStream();
StreamWriter writer = new StreamWriter(stream);

using blocks!

\$\endgroup\$
  • 3
    \$\begingroup\$ "Num is not an an acceptable combination in Scrabble" I like this ;-) \$\endgroup\$ – Heslacher Apr 25 '15 at 12:56
  • \$\begingroup\$ MultiplicativeOperators have a higher precedence than AdditiveOperators - having separate methods ensures they are always calculated first. \$\endgroup\$ – user34073 Apr 25 '15 at 17:44
  • \$\begingroup\$ Instead of using a LINQ Contains method for IsDigit, he could define it as char.IsDigit(c) || c == '.'. \$\endgroup\$ – Dan Lyons May 1 '15 at 17:34
10
\$\begingroup\$

Use singular names for enums

enum Operators
{
    Addition, Subtraction, Multiplication, Division, Modulo
}

Plural name for enums is a bit unnatural in practice. Look at these uses:

private static Operators getNextOperator() { ... }

Operators op = getNextOperator();

It looks as if the type is some kind of collection, like a set or list or map. The plural "Operators" and singular "getNextOperator()" is also confusing.

So, rename it to singular Operator and all uses become naturally natural.

I'm also surprised that the operators are not defined together with their usual symbols, I only see their names.

Centralize operator symbols

The operator symbols are scattered around in the code. The various methods of Calculator use magic strings like "+-*/%", "-+", "*/%". You already have an enum for operators, it would be better to make it in charge of the symbols.

Worst of all is this kind of thing:

private static Operators getNextOperator()
{
    const string OperatorSet = "+-*/%";

Why is OperatorSet a local variable? It should have been at least a static constant.

Something like this would be a better approach:

private static Operator getNextOperator()
{
    return Operator.fromChar((char)dataStream.Read());
}

where the implementation of Operator.fromChar should throw an exception if the character is invalid.

Implementing utility classes

The class ExtensionMethods is a utility class. Its name suggests a lot more than what it is. At least for now it contains a single method that manipulates a string, so StringUtils would be a more appropriate name.

A switch is more natural for branching on enums

Enums are well suited for switch statements instead of chained if-else. It's not a problem though, just a more common writing style.

getNextNum is half-baked

I don't know C#, but a better way to read a double probably already exists in the language. I'd look for that.

The implementation looks half-baked, because you read character by character, while doing some, but not all necessary validation with dataStream.Peek(). Effectively, you're doing half of the parsing yourself, and leaving the rest to double.TryParse, which will also inevitably repeat the digit checks you already did.

It would be better to either let the language do all the parsing for you, or if you already implemented part of it, then go all the way. You could keep the structure of the current code with peeking, but instead of appending characters to a string, you could build up a proper double.

Naming booleans

This may be a matter taste, but instead of this:

    bool valueIsNegative = false;

I suggest this:

    bool negative = false;

On a related note, this can be a bit simpler:

valueIsNegative ? -1 * val : val;

like this:

valueIsNegative ? -val : val;

Removing characters from strings

I don't know strings work in C# and how the .Remove method is implemented. If strings are immutable, then every call will create a new copy. If strings are mutable, then every call might involve an array copy to shift elements. Or it might be something clever and efficient.

A more efficient way of removing characters is to use extra storage, copying characters except the ones to remove. This way there won't be multiple array copies, just one. But one always, even if there was nothing to remove.

In short, the implementation of ExtensionMethods.RemoveAll looks inefficient, and I suggest to think of something better.

\$\endgroup\$
  • \$\begingroup\$ Small note: a static class can't be instantiated so there's no need for a private constructor. \$\endgroup\$ – Jeroen Vannevel Apr 25 '15 at 11:31
  • \$\begingroup\$ @JeroenVannevel, that's no small note, looks like I was saying rubbish there, thanks for pointing out! (deleted that part now) \$\endgroup\$ – janos Apr 25 '15 at 11:36
5
\$\begingroup\$

There are 2 good answers from @janos and @Jeroen. I offer some brief additions on top of their lengthier answers.

RemoveAll method

I suggest relying more on the framework.

public static string RemoveAll(this string str, char toRemove)
{
    return str.Replace(toRemove.ToString(), string.Empty);
}

valueIsNegative and --9

As for valueIsNegative and Jeroen's question about how to handle --9, I suggest flipping the sign each time you encounter it.

if ((char)dataStream.Peek() == '-')
{
    valueIsNegative = !valueIsNegative;
    dataStream.Read();
}
\$\endgroup\$

Your Answer

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