Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 10 down vote favorite
1

Hi all I have a piece of code that takes a couple of integers and check if performing an addition on the inputs would result in an overflow.

I was wondering if this code is solid :

    public static boolean CanAdd(int me, int... args) { 
        int total = me;
        for (int arg : args) {
            if (total >= 0) {
                if (java.lang.Integer.MAX_VALUE - total >= arg) { // since total is positive, (MaxValue - total) will never overflow
                    total += arg;
                } else {
                    return false;
                }
            } else {
                if (java.lang.Integer.MIN_VALUE- total <= arg) { // same logic as above
                    total += arg;
                } else {
                    return false;
                }
            }
        }
        return true;
    }

Btw does anyone have a better (faster) way to achieve the same thing?

share|improve this question
1  
Have you done any profiling which showed that this method is a bottleneck in your application? – palacsint Nov 24 '11 at 19:08
1  
@palacsint no this isn't a bottleneck in my application.. just that I'm interested in algorithms related to range checking and was wondering if there's a better solution (besides the casting one) =) – Pacerier Nov 24 '11 at 20:30
I have seen some (usually C/C++) questions and answers on SO with nice bitwise code, maybe you want to check them :-) – palacsint Nov 24 '11 at 22:18
I don't know your ultimate purpose, but depending on what you are trying to accomplish, perhaps, you might find Guava's checked arithmetic useful. Note this is not actually a direct response to your query. code.google.com/p/guava-libraries/wiki/MathExplained – Jubbat Jan 28 '12 at 23:56

2 Answers

up vote 14 down vote

I haven't found any input which isn't handled well by your code. Here are some tests:

assertTrue(CanAdd(0, Integer.MAX_VALUE));
assertTrue(CanAdd(0, Integer.MIN_VALUE));
assertTrue(CanAdd(Integer.MIN_VALUE, 0));
assertTrue(CanAdd(-1, Integer.MAX_VALUE));
assertFalse(CanAdd(1, Integer.MAX_VALUE));
assertFalse(CanAdd(-1, Integer.MIN_VALUE));

So, it works but it isn't an easy task to read it. If this isn't a bottleneck in an application I would use a long:

public static boolean canAdd(int... values) {
    long sum = 0;
    for (final int value: values) {
        sum += value;
        if (sum > Integer.MAX_VALUE) {
            return false;
        }
        if (sum < Integer.MIN_VALUE) {
            return false;
        }
    }
    return true;
}

I think it's easier to read and maintain.

Finally, a note: according to Code Conventions for the Java Programming Language the name of your method should be canAdd (with lowercase first letter).

Methods should be verbs, in mixed case with the first letter lowercase, with the first letter of each internal word capitalized.

share|improve this answer
up vote 2 down vote

Your logic looks solid to me. It's subtle, though.

Here is another version using long, but with much simpler logic:

public static boolean canAdd(int... values) {
    long longSum = 0;
    int intSum = 0;
    for (final int value: values) {
        intSum += value;
        longSum += value;
    }
    return intSum == longSum;
}

That's the most straightforward way I can think to write it — and it's going to be pretty definitive.

Note that there is no "early out" in this loop, meaning it will always run to the end of the list. However, not having any conditionals, it's likely to be faster in many cases, if that matters.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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