Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
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 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 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 at 17:11
2  
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 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 at 8:05

7 Answers 7

up vote 45 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 at 13:07
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 André Forsberg Aug 1 at 13:19
1  
@Malachi if (a < b < c) does not compile. if (b <= c && a <= b) compiles, but I would not recommend writing it like that. If you return a, it doesn't matter which one of b or c is bigger. –  Simon André Forsberg Aug 1 at 14:42
1  
Oh wait. int.... leave it as it is then, but note that NaNs can happen with floating point numbers... –  example Aug 3 at 2:52
2  
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 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 non existing 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 at 12:59
1  
The first method could have a varargs-signature like public static int min(int... numbers) so that it can be called like min(4, 2, 6, -1, 3) without explicitly creating a new array. –  Simon André Forsberg Aug 1 at 13:06
3  
+1 Math.min is the only way to go here. –  FreeAsInBeer Aug 1 at 13:23
1  
Why use Math.min() in the early functions but not in the latter functions? I see this perhaps being for(int i = 1; i < numbers.length; i++) { smallest = Math.min(smallest, numbers[i]); } –  corsiKa Aug 1 at 14:55
1  
@corsiKa because previously the answers were reversed. You're right, I should use Math.min! –  Pimgd Aug 1 at 15: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
27  
I disagree, that ternary operator is not readable. Your other method is perfectly fine though. –  Simon André Forsberg Aug 1 at 13:11
    
So, you agree with me? Using double negative is confusing sometimes :) –  Kao Aug 1 at 13:14
3  
No, I don't agree. I do not think that that ternary operator is "quite readable". –  Simon André Forsberg Aug 1 at 13:15
1  
@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 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 at 18:41

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));
}
share|improve this answer
1  
... and throws NoSuchElementException for min(); –  rolfl Aug 1 at 15:03
    
Good point. An assert probably will be best. –  Ryan Aug 1 at 15:04
    
@rolfl Was not able to get an assert to work, but an if condition (see edit) solves the NSEE problem. Not as clear as I wanted, but does work. –  Ryan Aug 1 at 15:12
1  
Why return 0.... instead of Integer.MAX_VALUE, or Integer.MIN_VALUE? or... 42 ;-) –  rolfl Aug 1 at 15:13
    
@rolfl That will depend on the implementation, and is why I'd rather use an assert than some value. But Integer.{MAX, MIN}_VALUE is equally good. –  Ryan Aug 1 at 15:14

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 at 19:49
1  
@Pimgd - I need to collect the three typical code-style guides in to one place, probably the Java tag wiki –  rolfl Aug 1 at 19:54

Two more suggestions

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

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

The first one is of course more readable so that's the one I would use.

share|improve this answer

Yet another solution, using divide and conquer approach:

public static int min(int... numbers) {
    if (numbers.length == 0) throw new IllegalArgumentException("Cannot have 0 arguments");
    return min(numbers, 0, numbers.length - 1);
}

private static int min(int[] numbers, int from, int to) {
    int mid = (from + to) / 2;
    return from == to ? numbers[mid] 
                      : Math.min(min(numbers, from, mid), min(numbers, mid + 1, to));
}

To understand recursion, you must understand recursion.

share|improve this answer

protected by rolfl Aug 3 at 16:16

Thank you for your interest in this question. Because it has attracted low-quality answers, 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.