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
public static int min(int a, int b, int c)
{
    int result = 0 ;
    if( a < b && a < c && b < c) result = a ;
    else if( a < b && a < c && b > c) result = a ;

    else if( a > b && a < c && b < c) result = b ;
    else if( a < b && b  > c &&  c < a) result = c ;

    else if( a > b && b < c && a > c) result = b ;
    else if( a > b && a > c && c < b) result = c ;
    return result ;
}

Is it better than nested if statements? Is there a more readable solution than this? To me it looks pretty readable, but I'm not sure whether it can be improved.

share|improve this question
2  
If if and and and look readable to you, try reading the code outloud. In before buffalo buffalo buffalo and james while john. – corsiKa Aug 1 '14 at 14:57
1  
Just something minor but you can simplify this a < b && a < c && b < c to a < b && b < c. – CyberneticTwerkGuruOrc Aug 1 '14 at 15:13
3  
It's hard to evaluate this without more context. Is there a reason you're avoiding Math.Min? Are you trying to optimize the number of comparisons? Are you sure you won't need to handle 4 inputs? – Pierre Menard Aug 1 '14 at 17:11
3  
your condidions are pretty complex. Why not do something like int result = a; if(b < result) result = b; if (c < result) result = c; return result;? This gives you the same result and has in total as many evaluations as your first if-condition. Similary the "verbose" option of this answer: codereview.stackexchange.com/a/58749/50461 – Mark Aug 4 '14 at 7:53
    
you're wasting time calculating an expression again and again if the previous conditions are false – Lưu Vĩnh Phúc Aug 4 '14 at 8:05
up vote 54 down vote accepted

To me it looks pretty readable

To me it doesn't.


Bugs:

The following method calls all incorrectly print 0:

System.out.println(min(3, 2, 2));
System.out.println(min(3, 3, 3));
System.out.println(min(1, 3, 3));
System.out.println(min(4, 2, 4));

This is because, when taking a look at your original code, it is overly complicated.

if( a < b && a < c && b < c) result = a ;
else if( a < b && a < c && b > c) result = a ;

Does it really matter if b < c or b > c? No, it doesn't here. And if b == c then neither of these current ones would be true which does the return 0. So that's a giant bug waiting to happen.

So those two first if's should be shortened into:

if (a <= b && a <= c) return a;

Note that I'm using <= here so that the case of all three having the same value gets handled correctly. Now also there's an early return so we don't need all these else.


If we group the rest of your statements according to what they return, we have for return b:

else if( a > b && a < c && b < c) result = b ;
else if( a > b && b < c && a > c) result = b ;

Which, if we always use b as the first operand and completely ignore whether or not a < c or a > c (and again, a == c is not handled here) we get:

if (b <= a && b <= c) return b;

Doing the same for c:

if (c <= a && c <= b) return c;

As one of a, b or c really has to be smallest, you can throw an exception if neither one of them is:

public static int min(int a, int b, int c) {
     if (a <= b && a <= c) return a;
     if (b <= a && b <= c) return b;
     if (c <= a && c <= b) return c;
     throw new AssertionError("No value is smallest, how did that happen?");
}

Or, if you don't like that exception:

public static int min(int a, int b, int c) {
     if (a <= b && a <= c) return a;
     if (b <= a && b <= c) return b;
     return c;
}

This is in my opinion significantly better than your original version.

However, I would recommend Pimgd's solution, either an array or using chained Math.min.

share|improve this answer
1  
Personally, if I went your way, I would just return c after finding that neither a nor b is the correct answer. – Pimgd Aug 1 '14 at 13:07
    
@Pimgd Yes, that'd also be a satisfactory solution. But when using an exception, you are really sure that c really has to be the smallest. – Simon Forsberg Aug 1 '14 at 13:13
2  
@Pimgd No, I don't know of such a situation, but if such a situation exists (highly unlikely) it will throw an Exception instead of incorrectly returning 3. – Simon Forsberg Aug 1 '14 at 13:19
1  
Oh wait. int.... leave it as it is then, but note that NaNs can happen with floating point numbers... – example Aug 3 '14 at 2:52
3  
If we get past if (a <= b && a <= c) return a; without returning a, we can safely assert that a > min(a, b, c), or equivalently, min(b, c) == min(a, b, c). Therefore, we can simplify if (b <= a && b <= c) return b; to if (b <= c) return b;, or even do away with return c; and just do return b <= c ? b : c; – bcrist Aug 3 '14 at 10:48

For these things we have java.lang.Math:

public static int min(final int a, final int b, final int c){
    return Math.min(a, Math.min(b, c));
}

Wow, look how short it is!

But it's 3 numbers today, it's 10 tomorrow.
As an alternative, how about an array?

public static int min(int... numbers){
    if(numbers.length == 0){
        throw new IllegalArgumentException("Can't determine smallest element in an empty set");
    }
    int smallest = numbers[0];
    for(int i = 1; i < numbers.length; i++){
        smallest = Math.min(smallest, numbers[i]);
    }
    return smallest;
}

I'd use the java.lang.Math solution, it's very short, and very readable.

share|improve this answer
    
For your 1st case there is java.util.Collections.min(). However, it works on collections. – Kao Aug 1 '14 at 12:59
    
I actually went looking for that, but then I realized I was using an array. Yet another possibility to use! It uses a Comparator though, so even with Integer objects you end up writing some wrapper. – Pimgd Aug 1 '14 at 13:00
    
Your second example (with the array) seems more complicated than what I have (see my answer). – Ryan Aug 1 '14 at 20:47
    
@Ryan That's because it's \$O(n)\$ and not \$O(2n)\$. It's faster because it doesn't first cast it into a list. Additionally, it doesn't autobox and unbox the int's, so that's another performance issue mitigated. – Pimgd Aug 1 '14 at 20:49
    
@Pimgd While you are correct that it has less performance, I think that readability and maintainability should be taken into account. – Ryan Aug 1 '14 at 22:21

I'd suggest using ternary operator, which is quite readable for me:

public static int min(int a, int b, int c) {
     return (a < b) ? (a < c) ? a
                              : c
                    : (b < c) ? b
                              : c;
}

However, if you prefer ifs:

public static int min(int a, int b, int c) {
    int min = a;
    if (min > b) min = b;
    if (min > c) min = c;
    return min;
}

In both cases only two comparisons will be performed.

share|improve this answer
28  
I disagree, that ternary operator is not readable. Your other method is perfectly fine though. – Simon Forsberg Aug 1 '14 at 13:11
4  
No, I don't agree. I do not think that that ternary operator is "quite readable". – Simon Forsberg Aug 1 '14 at 13:15
2  
@KyleHale actually for the scope of the problem I definitely agree with FreeAsInBeer. A ternary itself is already the less readable approach, and then unintuitively creating additional confusion like in the first, by nesting them, enrages me... – Vogel612 Aug 1 '14 at 14:38
2  
I've taken the liberty of reformatting the ternary expression to make it slightly more readable, though I still find the if-if version easier to understand. – 200_success Aug 1 '14 at 18:41
1  
The second is also unreadable. I have to think really hard when you use > in a min operation. – djechlin Aug 4 '14 at 1:53

I don't think it's really readable, as it conveys a lot of logic, which is simply unneeded.

Also you should, according to most of the common Java coding standards, not be placing { on its newline, also there should be no blank space between result and ; in return result ;

I would suggest following, though it needs Java 8:

public static int min(final int first, final int... others) {
    return IntStream.concat(IntStream.of(first), IntStream.of(others))
        .min()
        .getAsInt();
}

This way you cover the following:

  • You allow an arbitrary amount of integers to be passed to the method.
  • You get the min() in a clear way, using Java 8 streams, it checks the minimum somehow.
  • Because you know you have at least one result, you can safety get the value from the OptionalInt.

One minor nitpick is that the IntStream.concat(IntStream.of(first), IntStream.of(others)) to construct an IntStream is rather ugly.

share|improve this answer
    
Where do you get the coding standards from to say the braces should be on the same line as the method signature? – Pimgd Aug 1 '14 at 19:49
2  
@Pimgd - I need to collect the three typical code-style guides in to one place, probably the Java tag wiki – rolfl Aug 1 '14 at 19:54

Here is my idea: use an optional parameter, and use built-in libraries. Much clearer and simpler to understand. This works with 3 parameters, but also works with any # of parameters.

public static Integer min(Integer... numbers) {
    if (numbers.length == 0) return 0;
    // wanted this: assert numbers.length > 0;, but does not work
    return Collections.min(Arrays.asList(numbers));
}

Calling with:

System.out.println(min(2,1,3)); 

gives 1.

I tried to find a solution using generics but cannot get it to work.

Edit: Use IllegalArgumentException (thanks @SimonAndréForsberg!):

public static Integer min(Integer... numbers) {
    if (numbers.length == 0) throw new IllegalArgumentException("Cannot have 0 arguments, i.e. min()");
    return Collections.min(Arrays.asList(numbers));
}

Edit: The reason this solution works is that Integer... numbers allows for the program calling it to specify any number of arguments (even 0), and inside min here, it is treated as an array, which we can find the minimum of that array using Collections.min and Arrays.asList.

share|improve this answer

protected by rolfl Aug 3 '14 at 16:16

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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