Fix for 5015163, and my first webrev

[Kevin Bourrillion]

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.

* 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.

* 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)

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.




On Fri, Feb 6, 2009 at 12:59 PM, Rémi Forax <forax at univ-mlv.fr> wrote:
>
> Hi all,
> bug 5015163 is a RFE that advocate to add a method join (like in Python)
> to String/StringBuilder/StringBuffer.
>
> Here is the corresponding webrev (using the new infrastucture :)
> http://cr.openjdk.java.net/~forax/5015163/webrev.00/
> The patch is against tl repository.
>
> Who want to review it ?
>
> For the record, i have previously proposed this fix using the jdk-collaboration forum more than
> one year ago but because i am lazy i haven't take the time to post it here.
>
> cheers,
> Rémi Forax
>
>



--
Kevin Bourrillion @ Google
internal:  http://go/javalibraries
google-collections.googlecode.com
google-guice.googlecode.com