Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Write a method named hasMidpoint that accepts three integers as parameters and returns true if one of the integers is the midpoint between the other two integers; that is, if one integer is exactly halfway between them. Your method should return false if no such midpoint relationship exists.

Here are two different versions of code that I wrote at two different occasions to accomplish the same thing. How can I make my code better, and is there a better way to accomplish this task?

public static boolean hasMidpoint(int a, int b, int c) {
        if ( a == b && b == c) {
            return true;
        } else if ((a + b) / 2.0 == c || (b + c) / 2.0 == a || (a + c) / 2.0 == b) {
            return true;
        } else {
            return false;
        }
    }


public static boolean hasMidpoint(int p1, int p2, int p3) {
        if((double)(p1 + p2) / 2 == p3 ||  (double) (p2 + p3) / 2 == p1 || (double) (p1 + p3) / 2 == p2){
            return true;
        }
        return false;
    }
share|improve this question
up vote 12 down vote accepted

The way I would do it is as so:

public static boolean hasMidpoint(int a, int b, int c) {
    // Notice that I replaced division with multiplication
    boolean aIsMidpoint = (2 * a == b + c); 
    boolean bIsMidpoint = (2 * b == a + c);
    boolean cIsMidpoint = (2 * c == a + b);
    return aIsMidpoint || bIsMidpoint || cIsMidpoint;
}

This approach makes it very clear what I'm trying to do, avoids errors that could occur from floating point arithmetic or integer divides, and is short.

Minor caveat is that this might not work when a, b, c are very close to Integer.MAX or Integer.MIN due to overflow issues. If that's an issue, cast a, b, c to long (64bit data type) before doing the calculation.

There is also an alternative solution that involves sorting the triple but that is messier unless you can use Collections.sort().

Edit: Changed declaration of variables.

share|improve this answer
2  
You should declare your variables when they are initialized, not before. – gardenhead Jan 17 at 6:57
1  
I agree with this answer completely. The verbosity and shortness make it extremely easy to read. Also no floating point arithmetic needed is always a +1. As mentioned by garden head, I would declare the variables at the same time as initializing them. – Thijs Riezebeek Jan 17 at 12:42
    
@gardenhead, Good point. It even eliminates a line of code. – mleyfman Jan 17 at 18:04
1  
This is very clear but it doesn't take advantage of short-circuit evaluation. I would replace the variables with their expressions and add a comment, instead. – hakermania Jan 17 at 22:37
2  
@hakermania I would consider that premature optimization. The difference in performance between the two approaches would be on the order of a few percentage points, and odds are the bottlenecks would be elsewhere in input/output/network/disk processing. I also consider writing code that is self-documenting to be better than well-commented code. – mleyfman Jan 17 at 22:43

All of the previous answers seem to work on averages of each of the 2 numbers (expressed as either division or multiplication). But I'm working on averages of all 3 numbers, on the assertion that the average of 3 numbers would be the midpoint of the min and the max...

It would seem to be fewer operations.

I'm not a Java developer, so be kind if I've missed something....

public static boolean hasMidpoint(int a, int b, int c) {
    int iTotal = a + b + c;
    // I took mleyfman's idea and replaced division with multiplication
    return (3 * a == iTotal) || (3 * b == iTotal) || (3 * c == iTotal);
}

Edit: Implementing suggestions, the code is now:

public static boolean hasMidpoint(int a, int b, int c) {
    long total = a + b + c;
    // I took mleyfman's idea and replaced division with multiplication
    // A midpoint will always be the average of the 3 values, so 3*midpoint must equal sum of a, b and c.
    return (3 * a == total) || (3 * b == total) || (3 * c == total);
}
share|improve this answer
1  
Absolutely this would work with negatives. It's a good solution. – Octopus Jan 17 at 8:41
    
A few notes about this answer: Please don't use Hungarian notation, just call it total. You can write the first two lines in one: int total = a + b + c;. To avoid overflow issues, I'd advise using long instead for total. I'd separate the boolean expressions like I did in my answer to make it clear what each expression means. I'd also add a comment that the average of 3 numbers is the midpoint of min/max. – mleyfman Jan 17 at 18:13
    
You could add another "shortcut": if(total % 3 !=0){ return false } and then divide total by three to replace the 3 multiplications by one ! – oliverpool Jan 18 at 14:48

Your solutions (and the solutions presented in other answers) use addition, which has me worried about integer overflow. For example, both of them have hasMidpoint(Integer.MAX_VALUE - 1, Integer.MAX_VALUE - 1, -2) returning true.

I don't recommend venturing into floating-point arithmetic if it can be avoided.

Code that looks like if (expr) return true; else return false; should generally be replaced by the expr itself.

In your first version, a == b && b == c is a superfluous special case.

Suggested solution

Check if any two of the three lengths are equal.

public static boolean hasMidpoint(int a, int b, int c) {
    long ab = (long)a - b,
         bc = (long)b - c,
         ca = (long)c - a;
    return (ab == ca)       // a is midpoint?
        || (ab == bc)       // b is midpoint?
        || (ca == bc);      // c is midpoint?
}
share|improve this answer
    
You haven't removed the problem, just moved it so that we'll now have potentially erroneous results around Integer.MIN_VALUE. Granted it's an edge-case, but then, so is MAX_VALUE. I think the only way to be 100% safe is to sort the values. – Darrel Hoffman Jan 17 at 16:34
1  
Actually, looking again, you're find since you cast to long, and the arguments are only int. However, a more robust algorithm could take long arguments and still have no overflows at either MIN or MAX. – Darrel Hoffman Jan 17 at 17:00

To avoid any potential integer overflow situations, I'd go with something like the following:

public static boolean hasMidpoint(long a, long b, long c)
{
    if ((a <= b) == (b <= c)) //b is middle value
        return (a < b) ? (b - a == c - b) : (a - b == b - c);
    if ((b <= a) == (a <= c)) //a is middle value
        return (b < a) ? (a - b == c - a) : (b - a == a - c);
    return (a < c) ? (c - a == b - c) : (a - c == c - b); //c is middle value
}

This works by first checking which is the middle value, and then checking which order they are in, eliminating any chance for overloads. Note that I've changed the arguments to long to show that this method will support any values from Long.MIN_VALUE to Long.MAX_VALUE, something that none of the other answers can do right now. (Some support all int values by casting to long to avoid overflows, but this method should work for all values in the long range. (And since int is automatically upcast to long, you can use it with int arguments as well.)

share|improve this answer

This answer is a lot like mleyfman's but it is also returns sooner two out of three times.

public static boolean hasMidpoint(int a, int b, int c) {
    if ( 2*a == b+c ) return true;
    if ( 2*b == a+c ) return true;
    if ( 2*c == a+b ) return true;
    return false;
}
share|improve this answer
2  
If you're gonna golf it you might as well combine the whole thing into a single statement :) – Max Jan 17 at 6:28
1  
@Max I don't believe he's golfing, but rather effectively taking advantage of logical short circuiting by not evaluating all the expressions before checking them. But I believe you can replace the last if statement with just return ( 2*c == a+b ); – Patrick Roberts Jan 18 at 3:28

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.