6
\$\begingroup\$

Back in my calculator post, janos suggested I either find a better way to read a double from a StreamReader or else go the whole way and build a double from scratch. I looked through a few streams, but the only one I found that supported reading a double was the BinaryReader. This, unfortunately, reads exactly 8 bytes, and the data is stored as a char (or rather, as an int representation of a char), so I could enter the equation "5+5", and it would crash when it read the "+". If you know of a better solution, I would prefer using it.

private double GetNextNumber(StreamReader dataStream)
{
    double value = 0;
    int decimalLevel = 0;

    while (true)
    {
        if (!"0123456789.".Contains((char)dataStream.Peek()))
        {
            break;
        }
        char token = (char)dataStream.Read();

        if (token == '.')
        {
            if (decimalLevel == 0)
            {
                ++decimalLevel;
                continue;
            }
            throw new InvalidDataException("Invalid number format.");
        }

        int digit = int.Parse(token.ToString());

        value = value * 10 + digit;

        if (decimalLevel != 0)
        {
            ++decimalLevel;
        }
    }

    return value / Math.Pow(10, decimalLevel - 1);  // use "- 1" because value increments just before ending loop
}

Please analyze my code as thoroughly as the compiler does.

\$\endgroup\$
  • 3
    \$\begingroup\$ Even though this has a bug, it's not worth closing. You thought it was working code, and you asked for comments based on it. \$\endgroup\$ – Snowbody May 4 '15 at 4:26
  • \$\begingroup\$ Still, you shouldn't edit the existing code. I'm reverting it as per meta.codereview.stackexchange.com/questions/1763/… \$\endgroup\$ – Snowbody May 4 '15 at 15:25
7
\$\begingroup\$

You should avoid while (true) { … } loops wherever it is possible to do so. Here, it's very obviously avoidable!

while ("0123456789.".Contains((char)dataStream.Peek()))
{
    …
}

This parser leaves much to be desired in terms of handling

  • negative numbers (or you should document the fact that they are unsupported)
  • e notation for powers of ten
  • overflow and small numbers, due to excessive multiplying and dividing by powers of ten

Personally, I would avoid the entire headache by appending all plausible-looking characters to a StringBuilder, then calling double.Parse(string).

\$\endgroup\$
  • \$\begingroup\$ I had a guy once tell me on SO that while (true) { if (condition) { break; } ... } was a preference of style. I just didn't even respond. Gave up and left. \$\endgroup\$ – Millie Smith May 4 '15 at 20:08
5
\$\begingroup\$

You're multiplying the integer part of the number by 10^{decimalLevel-1} and then dividing it again. That seems to be needlessly introducing round-off error. How about splitting the code into "before decimal point" and "after decimal point" and factoring out common parts into a new method? Also that would make it easier to handle scientific notation, which your code currently doesn't.

Also, I have a problem with

int digit = int.Parse(token.ToString());

It would be better to use an array or hashtable, rather than making a new temporary string in order to call int.Parse on it.

\$\endgroup\$
3
\$\begingroup\$

I think your original code that used double.TryParse is fine. In some languages (like C), there are methods that try to find a number at the start of the given string. This is annoying when the whole string you have should be a number, but it could be useful here.

Unfortunately for you, there is no such method in C#, hence my conclusion that your original code was okay.

\$\endgroup\$
1
\$\begingroup\$

This line has a bug when returning a value with no decimal value, as pointed out by mjolka:

return value / Math.Pow(10, decimalLevel - 1);

It evaluates to value / 10 ^ -1, which essentially returns value * 10.

This can be fixed in either of two ways:

return value / Math.Pow(10, decimalLevel == 0 ? decimalLevel : decimalLevel - 1);

Or, replace this section:

        value = value * 10 + digit;

        if (decimalLevel != 0)
        {
            ++decimalLevel;
        }
    }

    return value / Math.Pow(10, decimalLevel - 1);  // use "- 1" because value increments just before ending loop
}

With this:

    if (decimalLevel == 0)
    {
        value = value * 10 + digit;
    }
    else
    {
        value += digit / Math.Pow(10, decimalLevel);
        ++decimalLevel;
    }
}

return value;

The first method is slightly faster according to my profiler.

\$\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.