Closed GoogleCodeExporter closed 9 years ago
Not convinced these increase readability, though other method names could make
this more interesting.
Original comment by wasserman.louis
on 22 Dec 2011 at 5:43
well I'm not too attached to the exact method names, more just want a single
place that encapsulates the "< 0" "> 0" logic (which is kind of hard to read in
my opinion and requires you to remember the contract of compareTo).
Original comment by lexicals...@gmail.com
on 22 Dec 2011 at 6:14
How does this relate to the Ranges class? E.g., it seems to me that the first
method can be replaced with (or implemented as) one of the following two calls:
Ranges.lessThan(o2).contains(o1);
Ranges.greaterThan(o1).contains(o2);
Arguably 'isBefore' and 'isAfter' are more to the point, but Guava's philosophy
seems to be that there should be (precisely) one preferred way to do things.
Original comment by stephan...@gmail.com
on 22 Dec 2011 at 6:16
The advantage of the compareTo approach is that you use a "<=" to express "x <=
y", and so on, which is pretty cute.
My first reflex on this issue was "this is a kitchen sink," which Guava is not
the place for...but I can see how people could find the current syntax less
than readable. I'd like to hear the opinion of some other Guava developers on
this.
Original comment by wasserman.louis
on 22 Dec 2011 at 6:18
I've found (having tried something similar at one point) that "isBefore(a, b)"
isn't really much of an improvement in readability because it's not instantly
obvious whether it's checking if the first parameter is before the second one
or the other way around. Though it's still rather awkward, I like
"Ranges.lessThan(b).contains(a)" a little bit better.
What would really work well would be "a.isBefore(b)" or "a.isLessThan(b)" which
are much clearer since they read like "a < b". Perhaps JDK8 will add default
methods to the Comparable interface for this.
Original comment by cgdec...@gmail.com
on 22 Dec 2011 at 9:43
A weird idea:
Comparison.check(a).isLessThan(b)
This wraps a in an object that provides these convenience comparison methods.
Obviously this somewhat improved readability comes at a small cost which may
just make it not worth it at all, I don't know. This could obviously be
extended to Comparators as well, possibly through Ordering (e.g.
someOrdering.check(a).isLessThan(b)).
The names of both the class and the method are just something I came up with in
30 seconds so it could probably be named better.
Original comment by cgdec...@gmail.com
on 22 Dec 2011 at 9:53
The weird idea in comment #6 distracted me for a couple of hours. Here's
another suggestion on how to name those, if you decide to proceed:
Comparison.is(a).lessThan(b);
Comparison.using(comparator).is(a).lessThan(b);
API:
Comparison {
public static <T extends Comparable<? super T>> Subject<T> is(T object);
public static <T> Rule<T> using(Comparator<? super T> comparator);
}
Comparison.Subject<T> {
public boolean lessThan(T other);
public boolean lessThanOrEqualTo(T other);
public boolean equalTo(T other);
public boolean greaterThanOrEqualTo(T other);
public boolean greaterThan(T other);
}
Comparison.Rule<T> {
public Subject<T> is(T object);
}
Original comment by michael.hixson@gmail.com
on 23 Dec 2011 at 12:23
Nice, I'd also come up with "is(a).lessThan(b)" and
"using(comparator).is(a).lessThan(b)" when thinking about the names a little
more after posting that. With static imports, it's almost as compact as if
Comparable actually had the methods.
Original comment by cgdec...@gmail.com
on 23 Dec 2011 at 1:41
I had thought about it some more and was leaning towards:
Comparisons {
public static <T> By<T> by(Comparator<? super T> comparator);
public static <T extends Comparable<? super T>> Is<T> is(T object);
}
...but I hadn't considered static imports.
"Comparisons.by(comparator).is(a).lessThan(b)" reads well but
"by(comparator).is(a).lessThan(b)" is awkward.
Original comment by michael.hixson@gmail.com
on 23 Dec 2011 at 3:04
As Colin wrote in comment 6, I think it makes more sense to add the method to
Guava's Ordering class, instead of obtaining the comparison builder through a
static factory method that takes a Comparator as a parameter:
Ordering<String> ordering = ...;
return ordering.is(a).lessThan(b);
If all you have is a standard Comparator, you can obtain an Ordering using the
Ordering.from(Comparator) static factory method.
Original comment by nev...@gmail.com
on 23 Dec 2011 at 6:54
For reference, Ordering.from(comparator) is how you convert a general
Comparator to our "fluent" Ordering class, which seems like a perfectly good
place for this.
I could get behind Ordering.is(a).lessThan(b).
Original comment by wasserman.louis
on 23 Dec 2011 at 10:36
One issue with just using Ordering for this (which otherwise seems like it'd be
an excellent place for it) is that it wouldn't be possible to have "is" as both
a static method and an instance method. I think a shortcut to make using it
with Comparable types shorter than "Ordering.natural().is(...)" would be
important.
Original comment by cgdec...@gmail.com
on 23 Dec 2011 at 2:01
I've added a few commits with a sample of this functionality here:
http://code.google.com/r/cgdecker-guava/source/list?name=comparison-helper
The implementation is obviously pretty trivial and I could use some input on
the naming and the locations of these methods. I put the static method for use
with Comparable instances in a new Comparables class since I haven't thought of
any good way to provide it through Ordering yet. Of course, I'm also curious as
to whether others on the Guava team consider this a good idea at all.
Original comment by cgdec...@gmail.com
on 24 Dec 2011 at 6:16
I ended up putting it in "Comparables" too. And then to justify the existence
of the new class to myself I dumped pass throughs to all of Ordering.natural's
non-factory instance methods in there along with "is". Looks like you were more
sane about it.
I used "Ordering.Is" instead of "Ordering.Wrapper" but wasn't thrilled about
it. I figured that if people were going to store it in a variable they'd name
it like "isA", so "Is<Foo> isA = is(a);" made sense. Also considered Orderable,
Comparison, Ordering.Query, Ordering.Question.
It was a fun little project to try to make this work, but... I pictured myself
explaining to a code reviewer familiar with Comparable & Comparator why I used
this instead of "a.compareTo(b) < 0" and couldn't come up with a good argument.
Heck, half the javadoc I wrote in my implementations of this was explaining
when _not_ to use it. Oh well. Looking forward to hearing guava devs'
thoughts.
Original comment by michael.hixson@gmail.com
on 25 Dec 2011 at 6:26
I think this is still worth considering, but not too strenously or
high-priority.
Original comment by wasserman.louis
on 28 Dec 2011 at 6:24
"I pictured myself explaining to a code reviewer familiar with Comparable &
Comparator why I used this instead of "a.compareTo(b) < 0" and couldn't come up
with a good argument. Heck, half the javadoc I wrote in my implementations of
this was explaining when _not_ to use it."
This is exactly how I feel about it. compare/compareTo in conjunction with <= <
> >= is the universal Java idiom and I don't think we can really improve on it.
We did spend some time trying to, but it's better to just accept the convention
we have in this case.
Original comment by kevinb@google.com
on 10 Jan 2012 at 11:57
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:14
Original comment by cgdecker@google.com
on 3 Nov 2014 at 9:09
Original issue reported on code.google.com by
lexicals...@gmail.com
on 22 Dec 2011 at 1:52