11
\$\begingroup\$

This code will determine if three numbers are consecutive for any order they're supplied to the method (any permutation of [n, n+1, n+2] should be accepted). Is there any way to make this code easier for me to write, or make the code more efficient? Any feedback would be most welcome!

I tried testing with 3! combinations of 2, 3, 4, and it seemed to work. I also tried 1, 2, 9, and 1, 0, 1. All cases seem to work.

public static boolean consecutive(int a, int b, int c) {
    if ( a == b || b == c || a == c) {
        return false;
    } else if (((a == b + 1 || a == b - 1) || (a == c + 1 || a == c - 1)) && ((b == c + 1 || b == c - 1)) 
               || ((b == c + 1 || b == c - 1) || (a == b + 1 || a == b - 1)) && (a == c + 1 || a == c - 1))  {
        return true;
    } 
    return false;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You need either a symmetrical code or a rather clever one. \$\endgroup\$ – maaartinus Jun 26 '15 at 20:35
  • \$\begingroup\$ I'm looking for some sort of clever solution. My laziness seems to get the best of me. \$\endgroup\$ – cody.codes Jun 26 '15 at 20:47
  • 1
    \$\begingroup\$ Due to an anomaly of timing, I posted this as a question, not an answer: Consecutive, or not. \$\endgroup\$ – rolfl Jun 26 '15 at 22:16
  • \$\begingroup\$ @rolfl That's IMHO perfectly fine. Actually, you could post it as both. But note that neither your post not any of the two answers here really provide a code review. \$\endgroup\$ – maaartinus Jun 26 '15 at 22:21
15
\$\begingroup\$

That's a lot of cases to enumerate. You're basically doing all of the comparisons that a sorting algorithm would do, but you've "unrolled" the loop.

If you don't want to construct an array and sort it, then you could try this: check that the minimum and maximum differ by 2, and that all three numbers are distinct. Math.min() and Math.max() are just conditionals packaged in a more readable form.

public static boolean consecutive(int a, int b, int c) {
    int min = Math.min(a, Math.min(b, c));
    int max = Math.max(a, Math.max(b, c));
    return max - min == 2 && a != b && a != c && b != c;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Note that this uses a subtly different definition of "consecutive" than the one in the OP in the neighborhood of overflow. This definition is probably the intended one for most uses though. \$\endgroup\$ – Doug McClean Jun 28 '15 at 5:03
  • \$\begingroup\$ @bradvido Are you saying that this solution isn't readable? \$\endgroup\$ – 200_success Jun 29 '15 at 15:18
  • \$\begingroup\$ I'm saying that taken out of context, it's not immediately clear what it does, esp w/o comments. It's definitely clever and likely faster than checking a sorted array. \$\endgroup\$ – bradvido Jun 29 '15 at 15:20
17
\$\begingroup\$

The problem that I have with writing something like the original code is that it is complicated to be sure that it does the right thing in every situation. You've taken three values and perform whopping fifteen comparisons on them. What if you'd missed one? The code would almost work, except it would occasionally (in that one situation) return the wrong result.

A conceptually simple way to handle this is

public static boolean consecutive(int... numbers) {
    Arrays.sort(numbers);

    for (int i = 1; i < numbers.length; i++) {
        if (numbers[i] != numbers[i-1] + 1) {
            return false;
        }
    }

    return true;
}

Sorting saves a lot of logic here. Since we know the order, we can just check the differences directly. If any adjacent numbers are not consecutive, we can return false. If it makes it all the way through, they all must be consecutive.

Note that this also handles other than three numbers. For example, it always returns true if there's zero or one number passed. This may or may not be what you want.

Now we do five manual comparisons (including the comparison of i to numbers.length) to determine that three numbers are consecutive. The sort does more comparisons, but we don't have to worry if we got those correctly. The chances of there being an error in sort that made it through the Java compiler's testing is extremely low.

I find this version much easier to read and verify correctness than the original code. For most cases, the difference in runtime is going to be minimal and unimportant. My ability to write code that looks like this quickly and be sure it works is usually going to be more important. And if you really need blazing speed, there are always options like @200_success offers.

\$\endgroup\$
  • \$\begingroup\$ To stress the simplicity: after sorting three values, return numbers[0] + 1 == numbers[1] && numbers[1] + 1 == numbers[2]; \$\endgroup\$ – greybeard Feb 3 '18 at 11:19
7
\$\begingroup\$

Agreed, sorting is easiest to simplify the code. To guarantee exactly three, you would call something like int numArr = new int[]{8,6,7}; consecutive(3, numArr); using this method:

public static boolean consecutive(int count, int... numbers) {
    if (numbers.length != count) {//make sure the correct number of numbers was sent in
        return false;
    }
    Arrays.sort(numbers);//guarantee order to simplify logic

    for (int i = 1; i < numbers.length; i++) {
        //compare this num to the previous num, make sure its exactly 1 greater
        if (numbers[i] != numbers[i - 1] + 1) {
            return false;
        }
    }
    return true;
}
\$\endgroup\$
6
\$\begingroup\$

Your code is properly formatted (except for one superfluous space), however, the lengthy expression

((a == b + 1 || a == b - 1) || (a == c + 1 || a == c - 1)) && ((b == c + 1 || b == c - 1)) 
|| ((b == c + 1 || b == c - 1) || (a == b + 1 || a == b - 1)) && (a == c + 1 || a == c - 1)

is pretty hard to read. The best solution is to use either sorting or something smart as 200_success or rolfl proposed. I try an advice for the case that no such solution is available.


Note that you're doing something like

if (x) {
     return false;
} else if (y) {
     return true;
}
return false;

This is actually always sort of wrong as you can do

if (x) {
     return false;
} else {
     return y;
}

instead. You can also leave out the "else", but that's matter of style.


As your lengthy condition is a disjunction and the action is trivial, you can simply split it like

if (x) {
     return false;
} else if (y1) {
     return true;
} else if (y2) {
     return true;
}
return false;

Here, I avoided my above simplification to preserve symmetry.


Let's look at the part denoted as y1, i.e.,

((a == b + 1 || a == b - 1) || (a == c + 1 || a == c - 1)) && ((b == c + 1 || b == c - 1)) 

now. The first part states that the distance of a and b is 1, in other words Math.abs(a - b) == 1. You can use it to make your conditions slightly less repetitive.


More importantly, observe the same expression appear later again, define some local variables to keep it short (with or without abs; the idea is independent):

if (a == b || b == c || a == c) {
    return false;
}

boolean ab = a == b + 1 || a == b - 1;
boolean bc = b == c + 1 || b == c - 1;
boolean ac = a == c + 1 || a == c - 1;

return (ab | ac) & bc || (bc | ab) & ac;

My above naming is not the best, however, I consider it acceptable as the scope is very limited. Still, because of the asymmetry, it's a bit hard to tell if it's right.

You could rewrite it as

return ab & bc | ab & ac | bc & ac;

Another simplification is possible by observing that shifting all value by the same distance doesn't change anything. So you could do something like

a -= c;
b -= c;
c -= c; // i.e., c = 0

and simplify the other expression a bit. This isn't worth it here. By using the arithmetic, you make yourself susceptible to overflow just like here.


To add something funny, I propose this

public static boolean consecutive(int a, int b, int c) {
    return 2 ==
        + (Math.abs(a - (long) b) == 1 ? 1 : 0)
        + (Math.abs(b - (long) c) == 1 ? 1 : 0)
        + (Math.abs(a - (long) c) == 1 ? 1 : 0);
}

It works simply by requiring exactly two of the distances to equal one. The case to long prevents overflow (and may be left out if you don't mind wrapping around Integer.MAX_VALUE).

\$\endgroup\$
2
\$\begingroup\$

A recursive solution:

public static boolean consecutive(int a, int b, int c) {
    switch (Math.abs(c-a)) {
        case 2: return ( 2*b == a+c );     // b should be right between a and c
        case 1: return consecutive(b,c,a); // try another pair of outer numbers
        default: return false;
    }
}

The check 2*b == a+c should work fine even in case of integer overflow (not sure about false positives, though).

\$\endgroup\$
  • 1
    \$\begingroup\$ consecutive(2147483646, -1, -2147483648) returns true. \$\endgroup\$ – 200_success Jun 27 '15 at 15:47
  • \$\begingroup\$ case 2: return b == (a&c)+((a^c)>>1); \$\endgroup\$ – greybeard Feb 2 '18 at 18:29
1
\$\begingroup\$

Observing that a and b must be close together, this code is not too bad:

public static boolean consecutive(int a, int b, int c) {
    switch (b - a) {
        case +1: return c == b + 1 || c == a - 1;
        case +2: return c == a + 1;
        case -1: return c == a + 1 || c == b - 1;
        case -2: return c == b + 1;
        default: return false;
    }
}

And an alternative:

public static boolean consecutive(int a, int b, int c) {
    return (  b < a ? consecutive (b, a, c)
            : b == a + 1 ? (c == b + 1 || c == a - 1)
            : b == a + 2 ? (c == a + 1)
            : false);
}
\$\endgroup\$
  • \$\begingroup\$ These solutions both treat Integer.MIN_VALUE and Integer.MAX_VALUE as neighbors of each other. \$\endgroup\$ – 200_success Jun 28 '15 at 6:41
1
\$\begingroup\$

I prefer to break my code down into modules:

public boolean threeConsecutive (int n1, int n2, int n3) {
    int nConsecutivePairs = differ(n1 - n2, 1) + differ(n2 - n3, 1) + differ(n3 - n1, 1); 
    int nSecondPairs = differ(n1 - n2, 2) + differ(n2 - n3, 2) + differ(n3 - n1, 2);
    return (nConsecutivePairs == 2 && nSecondPairs == 1);
}
private int differ(int n1, int n2) {
if (n1 == n2 || (n1 + n2) == 0)
    return 1;
else 
    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Any reason you think your approach is better? \$\endgroup\$ – Billal Begueradj Feb 2 '18 at 7:34
  • \$\begingroup\$ I can see a logical operation factored out, even if differ() does not tell me what exactly it does. I associate modules with separate compilation. \$\endgroup\$ – greybeard Feb 2 '18 at 7:44
0
\$\begingroup\$

One possibility would be to use a Set in order to check for duplicate integers. If there are no duplicates, the length of the list should be max - min + 1:

public static boolean areConsecutiveIntegers(Integer[] numbers) {
    List<Integer> numbersList = Arrays.asList(numbers);
    Integer min = Collections.min(numbersList);
    Integer max = Collections.max(numbersList);
    Set<Integer> uniqueNumbers = new HashSet<>(numbersList);
    return numbersList.size() == uniqueNumbers.size() && numbersList.size() == max - min + 1;
}

Here are some tests:

package stackoverflow;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import org.junit.Test;


public class ConsecutiveIntegers
{
    @Test
    public void testConsecutiveIntegers() {
        Integer[][] consecutives = {
                { 1, 2, 3 },
                { 3, 2, 1 },
                { 1, 3, 2 },
                { 1, 2, 3, 5, 6, 7, 4, 0, 9, 8, -1 },
                { 1 },
        };

        Integer[][] nonConsecutives = {
                { 2, 0 },
                { 1, 2, 3, 3 },
                { 1, 2, 2 },
                { -1, 1 },
                { 1, 2, 3, 5, 6, 7, 4, 9, 8, -1 },
        };

        for (int i = 0; i < consecutives.length; i++) {
            assertTrue(areConsecutiveIntegers(consecutives[i]));
        }

        for (int i = 0; i < nonConsecutives.length; i++) {
            assertFalse(areConsecutiveIntegers(nonConsecutives[i]));
        }
    }

    public static boolean areConsecutiveIntegers(Integer[] numbers) {
        List<Integer> numbersList = Arrays.asList(numbers);
        Integer min = Collections.min(numbersList);
        Integer max = Collections.max(numbersList);
        Set<Integer> uniqueNumbers = new HashSet<>(numbersList);
        return numbersList.size() == uniqueNumbers.size() && numbersList.size() == max - min + 1;
    }
}
\$\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.