Fix for 5015163, and my first webrev

[Kevin Bourrillion]

Bumping this thread.  From the peanut gallery, I believe that Rémi's change
is extremely worthy.  I have more nitpicking I'd like to do on it, but am
holding off until we learn whether Sun would accept it in any shape.

At this point, it's still very mysterious to me how this will work (small,
simple new APIs being added to core libraries via openjdk).





On Fri, Feb 6, 2009 at 5:01 PM, Rémi Forax <forax at univ-mlv.fr> wrote:

> Kevin Bourrillion a écrit :
>
>> Hello,
>>
>> A few thoughts.
>>
>> First, this functionality is badly needed.  Absolutely everyone
>> rewrites this, in hundreds of different ways.  At Google we're no
>> exception, we have our own hand-rolled Join.java class full of static
>> methods, and it has thousands of callers in our private codebase.
>>
>> Your selection of overloads (Iterable, Object[], and
>> (Object,Object...)) is exactly correct.  Kudos.  We'd added Iterator
>> versions as well, but almost no one uses them, so forget that.
>>
>> My main concerns with your current API are:
>>
>> * I don't think that ",".join(collection) is a good idea.  It clashes
>> too strongly with the StringBuilder etc. versions.  It also plain
>> "looks weird".  A static String.join(",", collection) is preferable.
>> I don't think anyone would be surprised by this.
>>
>>
> Use the receiver or not:
> Pro:
> - join works with split and split use the receiver
> - Python use the receiver too
>
> Cons:
> - static seems least surprising.
> - Perl's join and PHP's join are functions
>
> I'm convinced, I will modify the patch.
>
>  * It should be considered separately whether to support other
>> Appendables via static methods (somewhere) like:
>>
>>  static <A extends Appendable> A join(A appendable, String delimiter,
>> Iterable<?> tokens) throws IOException
>>
>> This provides a superset of what your StringBuilder etc. methods can
>> do, but your methods are still good because of their convenience
>> especially in chains of method calls, and because they don't need to
>> throw any exception.
>>
>>
> Or perhaps, in JDK8 (when closure will be integrated) the compiler will
> be able to infer that because join is used with String, it can't throw an
> IOE
> That's not what BGGA propose but I will love to see that :)
> But that's another story.
>
>> * The biggest problem we've had with our Join class is that it turns
>> out that everyone has a different idea of how they'd like nulls
>> handled.
>>
>>   - output as "null" (your, and our, current behavior)
>>   - skip?
>>   - skip only if trailing?
>>   - throw NPE? (often the best thing)
>>
>>
>
> In my opinion (i am perhaps wong) join will be mostly used
> just before print the String. That's why I have chosen the the print(ln)
> behavior.
>
>
>  We now support everyone by having the concept of an instantiable
>> Joiner (I've seen this in other languages).  Example:
>>
>>    private static final Joiner JOINER = Joiner.with(", ").skipNulls();
>>      . . .
>>
>>        return JOINER.join("Harry", "Ron", "Hermione");
>>
>> Just to give everyone food for thought, our Joiner's API currently
>> looks like this:
>>
>>  static Joiner with(String separator)
>>
>>  Joiner skipNulls()
>>  Joiner useForNull(String nullText)
>>
>>  <A extends Appendable> A appendTo(A appendable, Iterable<?> parts)
>> throws IOException
>>  <A extends Appendable> A appendTo(A appendable, Object[] parts)
>> throws IOException
>>  <A extends Appendable> A appendTo(A appendable, Object first,
>> Object... rest) throws IOException
>>
>>  StringBuilder appendTo(StringBuilder builder, Iterable<?> parts)
>>  StringBuilder appendTo(StringBuilder builder, Object[] parts)
>>  StringBuilder appendTo(StringBuilder builder, Object first, Object...
>> rest)
>>
>>  String join(Iterable<?> parts)
>>  String join(Object[] parts)
>>  String join(Object first, Object... rest)
>>
>>  MapJoiner withKeyValueSeparator(String keyValueSeparator)
>>
>>  class MapJoiner {
>>    <A extends Appendable> A appendTo(A appendable, Map<?, ?> map)
>> throws IOException
>>    StringBuilder appendTo(StringBuilder builder, Map<?, ?> map)
>>    String join(Map<?, ?> map)
>>
>>    MapJoiner useForNull(String nullText)
>>  }
>>
>> End of random thoughts.
>>
>>
>
> Cool, i don't think this should be integrated in the JDK,
> After all, If i want a full featured joiner, I can download  the google
> collection jar :)
>
>
>> --
>> Kevin Bourrillion @ Google
>> internal:  http://go/javalibraries
>> google-collections.googlecode.com
>> google-guice.googlecode.com
>>
>>
> Rémi
>



-- 
Kevin Bourrillion @ Google
internal:  http://go/javalibraries
google-collections.googlecode.com
google-guice.googlecode.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/core-libs-dev/attachments/20090302/319cef29/attachment.html>