34
\$\begingroup\$ 
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.

\$\endgroup\$
6
  • 2
    \$\begingroup\$ If if and and and look readable to you, try reading the code outloud. In before buffalo buffalo buffalo and james while john. \$\endgroup\$ Commented Aug 1, 2014 at 14:57
  • 1
    \$\begingroup\$ Just something minor but you can simplify this a < b && a < c && b < c to a < b && b < c. \$\endgroup\$ Commented Aug 1, 2014 at 15:13
  • 3
    \$\begingroup\$ 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? \$\endgroup\$ Commented Aug 1, 2014 at 17:11
  • 3
    \$\begingroup\$ 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 \$\endgroup\$ Commented Aug 4, 2014 at 7:53
  • \$\begingroup\$ you're wasting time calculating an expression again and again if the previous conditions are false \$\endgroup\$ Commented Aug 4, 2014 at 8:05

5 Answers 5

Reset to default
57
\$\begingroup\$

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.

\$\endgroup\$
9
  • 1
    \$\begingroup\$ Personally, if I went your way, I would just return c after finding that neither a nor b is the correct answer. \$\endgroup\$ Commented Aug 1, 2014 at 13:07
  • \$\begingroup\$ @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. \$\endgroup\$ Commented Aug 1, 2014 at 13:13
  • 2
    \$\begingroup\$ @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. \$\endgroup\$ Commented Aug 1, 2014 at 13:19
  • 1
    \$\begingroup\$ Oh wait. int.... leave it as it is then, but note that NaNs can happen with floating point numbers... \$\endgroup\$ Commented Aug 3, 2014 at 2:52
  • 3
    \$\begingroup\$ 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; \$\endgroup\$ Commented Aug 3, 2014 at 10:48
73
\$\begingroup\$

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.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ For your 1st case there is java.util.Collections.min(). However, it works on collections. \$\endgroup\$ Commented Aug 1, 2014 at 12:59
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Aug 1, 2014 at 13:00
  • \$\begingroup\$ Your second example (with the array) seems more complicated than what I have (see my answer). \$\endgroup\$ Commented Aug 1, 2014 at 20:47
  • \$\begingroup\$ @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. \$\endgroup\$ Commented Aug 1, 2014 at 20:49
  • \$\begingroup\$ @Pimgd While you are correct that it has less performance, I think that readability and maintainability should be taken into account. \$\endgroup\$ Commented Aug 1, 2014 at 22:21
16
\$\begingroup\$

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.

\$\endgroup\$
6
  • 29
    \$\begingroup\$ I disagree, that ternary operator is not readable. Your other method is perfectly fine though. \$\endgroup\$ Commented Aug 1, 2014 at 13:11
  • \$\begingroup\$ @Kao Perhaps, but having even a small amount of unreadable/unmaintainable code in an application only spawns more of the same. I'd prefer to keep my code clean, easily maintainable and very readable. Even if more readable code leads to a small performance drop, I'll gladly take that hit since it leads to more robust code. \$\endgroup\$ Commented Aug 1, 2014 at 14:16
  • 2
    \$\begingroup\$ @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... \$\endgroup\$ Commented Aug 1, 2014 at 14:38
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ Commented Aug 1, 2014 at 18:41
  • 1
    \$\begingroup\$ The second is also unreadable. I have to think really hard when you use > in a min operation. \$\endgroup\$ Commented Aug 4, 2014 at 1:53
11
\$\begingroup\$

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.

\$\endgroup\$
0
10
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Where do you get the coding standards from to say the braces should be on the same line as the method signature? \$\endgroup\$ Commented Aug 1, 2014 at 19:49
  • 2
    \$\begingroup\$ @Pimgd - I need to collect the three typical code-style guides in to one place, probably the Java tag wiki \$\endgroup\$ Commented Aug 1, 2014 at 19:54

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.