What's Wrong With This Code (#18)

Tuesday, October 9, 2007

Here is another golden oldie:

Numeric overflows are a type of software bug that occur when a calculation produces a result that doesn't fit in the intended storage location. One of the most famous cases of an overflow bug is the overflow bug that destroyed an Ariane 5 rocket in 1996*.

Fortunately, .NET has a dedicated exception to highlight overflow problems. Just run the following VB code and watch the OverflowException appear.

Dim i As Integer
i += Integer.MaxValue
i +=
Integer.MaxValue

So, given this class:

public class LineItem
{
    
private int _quantity;
    
public int Quantity
    {
        
get { return _quantity; }
    }

    
public int AddQuantity(int additionalQuantity)
    {
        
// some logic ...
        return _quantity += additionalQuantity;
    }

    
// other stuff...
}

Why does the following unit test fail?

[TestFixture]
public class LineItemTest
{
    [
Test]
    [
ExpectedException(typeof(OverflowException))]
    
public void TestForOverflow()
    {
        
LineItem lineItem = new LineItem();
        lineItem.AddQuantity(
Int32.MaxValue);
        lineItem.AddQuantity(
Int32.MaxValue);
    }
}

What options are available to fix the problem?

* The article Design By Contract: The Lessons Of Ariane says the real error was a "reuse specification error". Quote: "To attempt to reuse software without Eiffel-like assertions is to invite failures of potentially disastrous consequences".


Comments
Jonas Tuesday, October 9, 2007
The unit test fails because when Int32.MaxValue is passed to AddQuantity for the second time, _quantity overflows, as it's already at Int32.MaxValue.
Jonathan Malek Tuesday, October 9, 2007
I think it should be:

return checked(_quantity += additionalQuantity);

So that runtime overflow checking is applied. You can also use /checked+ as a compiler switch instead of the statement or block.
Kalpesh Tuesday, October 9, 2007
The unit test would fail because the value is returned before += could execute? To make it throw the exception, it should read

_quantity += additionalQuantity;
return _quantity;

Just a wild guess. I didn't try running it.
Jason Stangroome Tuesday, October 9, 2007
VB projects have an advanced compile optimization to remove integer overflow checks. This is off by default so VB checks for and throws OverflowExceptions.

C# projects have advanced build settings to check for arithmetic overflow/underflow. This is off by default so C# does not check for overflow and does not throw exceptions.

To pass the unit test without enabling overflow checks for the whole project and assuming only non-negative numbers are valid for the additionalQuantity field (yet another unit test) I would add the following guard clause:

if (_quantity > Int32.MaxValue - additionalQuantity) throw new OverflowException();
Ronin Tuesday, October 9, 2007
Well i suppose you should you checked construct in AddQuantity, else compiler won't throw an exception, will just round up.
Ronin Tuesday, October 9, 2007
You need to use checked construct in AddQuantity method in order to ensure that the compiler will throw an exception.
The default behavior is to round-up.
Dave Tuesday, October 9, 2007
I believe this is down to the fact that by default VB checks for overflows at runtime which C# by default does not. Thus, the test will fail .

The simple of way of getting the exception to throw (and the test to pass) is to use the checked keyword so that the addition of the two integers is checked at runtime or you also set the /checked switch on the compiler to make sure that all integer arithmetic is overflow checked.
Eric D. Burdo Tuesday, October 9, 2007
Another potential problem. You have an Int32, and an Int. This doesn't guarantee that the Int will be 32 bits (depends on the platform and such).
scott Wednesday, October 10, 2007
Good catch!
Scott McMaster Friday, October 12, 2007
Re: int versus Int32 a couple of comments above, "int" in C# IS guaranteed to be 32 bits == "Int32" per spec. The "platform" in this case IS .NET. The designers didn't want to propagate the old C/C++ data type sizing mess.
Comments are now closed.
by K. Scott Allen K.Scott Allen
My Pluralsight Courses
The Podcast!