HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

CC C# calculator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
calculatorstackoverflowprogramming

Problem

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 
    /// 
    /// 
    /// A mathematical epxression needing evalation.
    /// The evaluated expression.
    public static double EvaluateExpression(string equation)
    {
        dataStream = new StreamReader(StringToStream(equation));

        double evaluation = AdditiveOperators();

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

        return evaluation;
    }
}

Solution

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!

Code Snippets

var input = "19*cqwcq521sq0dqs0-dqs44sq";
var toRemove = 'c';
var result = new string(input.ToCharArray().Where(x => x != toRemove).ToArray());
if (OperatorSet.IndexOf(token) == -1)
{
    throw new InvalidDataException("Invalid or missing operator.");
}
return (Operators)OperatorSet.IndexOf(token);
getNextNum()
if (OperatorSet.IndexOf(token) == -1)
if(!"+-*/%".Contains(token))

Context

StackExchange Code Review Q#87918, answer score: 11

Revisions (0)

No revisions yet.