search

jamesbrowder / guava-libraries

Automatically exported from code.google.com/p/guava-libraries
Apache License 2.0
0 stars 0 forks source link

Make Ints.min() with no arguments not compile (etc.) #750

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
hi,all
       I saw the implemention of Ints's method min and max. In my opinion, it can be better. such as

public static int min(int first, int... array) {
       int min = first;
       for (int i = 1; i < array.length; i++) {
     if (array[i] < min) {
       min = array[i];
     }
   }
   return min;
}

When client uses this implemention without arguments, it will get a
complier error, not a runtime exception.
The same as Longs and other *s

Original issue reported on code.google.com by liuyon...@gmail.com on 11 Oct 2011 at 7:45

GoogleCodeExporter commented 9 years ago 
You will want to increment i starting from 0, not 1 :)

Original comment by stephan...@gmail.com on 11 Oct 2011 at 8:00

GoogleCodeExporter commented 9 years ago 
That's a good point, but it will stop users from passing a standard array 
(varargs accept both an arbitrary number of arguments, as well as an array):

int[] array = computeArray();
int min = Ints.min(array);

Original comment by nev...@gmail.com on 11 Oct 2011 at 8:25

GoogleCodeExporter commented 9 years ago 
Another method can be created then, it shouldn't be a big deal:

public static int min (int[] array) { // No varargs
  ... // Keep the same implementation as the current one.
}

Original comment by ogregoire on 11 Oct 2011 at 9:38

GoogleCodeExporter commented 9 years ago 
@stephan,tks. The i should starting from 0, not 1. :)

@ogregoire, yes, it shouldn't be a big deal to add such a method

Original comment by liuyon...@gmail.com on 11 Oct 2011 at 10:53

GoogleCodeExporter commented 9 years ago 
This all is perfectly sensible, but,

I'm skeptical that there really are enough people out there trying to write 
Ints.min() with no arguments.  I just doubt that it happens often enough that 
this change would even pay for itself (for the time spent to do, review and 
test the change itself, and the fact that there's that much more bulk in the 
javadoc from then on).

The hazard that I think *does* occasionally happen is empty arrays -- which 
this won't even catch.

Does anyone have a compelling argument?

Original comment by kevinb@google.com on 11 Oct 2011 at 2:45

GoogleCodeExporter commented 9 years ago 
I have finished such improvements to these classes. These classes have been 
attached.

Original comment by liuyon...@gmail.com on 12 Oct 2011 at 12:54

Attachments:

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
There are also ten testCases modified. They are also in the attachment.

Original comment by liuyon...@gmail.com on 12 Oct 2011 at 12:59

Attachments:

GoogleCodeExporter commented 9 years ago 
I agree with Kevin: -1 on this change.  The increased API clutter isn't worth 
the payoff.

Original comment by wasserman.louis on 16 Oct 2011 at 10:43

GoogleCodeExporter commented 9 years ago 
I agree with Kevin and Louis that it isn't really worth it to add API clutter 
to solve this edge-case. If people really try to call Ints.min() with no 
arguments, it'll fail fast enough (at runtime)... Findbugs and other static 
analysis tools might even be able to detect this at compile-time.

Sadly, liuyongpo already spent time creating a patch, before a decision was 
even made as to whether this code should be added :/ I think this is a good 
example of a patch that was made with good intentions, but might have been 
submitted a little too fast. It might not be accepted, as mentioned by Kevin 
here: https://plus.google.com/113026104107031516488/posts/ZRdtjTL1MpM (BTW, I 
agree with Kevin's post and understand his reasons)

Original comment by nev...@gmail.com on 17 Oct 2011 at 10:20

GoogleCodeExporter commented 9 years ago

Original comment by kevinb@google.com on 15 Nov 2011 at 10:22

GoogleCodeExporter commented 9 years ago 
This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:15

GoogleCodeExporter commented 9 years ago

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:09