Original comment posted by adam.g...@evocatus.com on 2012-07-12 at 01:56 PM

I meant to label this as an enhancement.

Original comment posted by cpovirk@google.com on 2012-07-12 at 02:04 PM

Prefix and suffix seem to be mostly about quoting, whether in SQL, CSV, or something else. Since proper quoting requires escaping, we'd probably go a step further than this to something like issue 412 or issue 813. For the moment, you may want to use Iterables.transform() on the input elements.

Original comment posted by wasserman.louis on 2012-07-12 at 02:17 PM

@cpovirk: I'm not entirely convinced by that argument, just because...most Guava-internal uses of Joiner are for toString() implementations, and those add varying brackets and the like. No escaping required.

This isn't a +1, mind...but the idea is kind of tempting.

Original comment posted by wasserman.louis on 2012-07-12 at 02:17 PM

(No comment entered for this change.)

Labels: Type-Enhancement, Package-Base

Original comment posted by adam.g...@evocatus.com on 2012-07-12 at 03:04 PM

@cpovirk ... Truthfully I am using it for a crappy SQL prototype of which I know the values are safe but I have needed it some times for just messaging.

That being said I can easily go into candy land with enhancements like: Joiner.onFirst('') and Joiner.onLast(", or ") ... so that I can do "'1','2', or '3'"

Maybe what I really need is an AbstractJoiner that I can extend.

public abstract class AbstractJoiner<J> {
  ....
  //for fluent
  protected abstract J getSelf();
}

Original comment posted by wasserman.louis on 2012-07-12 at 03:07 PM

At the moment, we have thread-safety and immutability guarantees for Joiner that are really nice. You can always put a Joiner in a static final constant field.

All that goes away if we let Joiner be extended.

That said...at the moment, there are workarounds for this that are effective, if a bit awkward.

   return Joiner.on(',')
     .appendTo(new StringBuilder("{"), elements)
     .append("}")
     .toString();

Original comment posted by adam.g...@evocatus.com on 2012-07-12 at 04:03 PM

You can make the Abstract class and the derived OOTB Joiner thread safe.

The custom extended class would just have to make their own "public static MyJoiner on(String separator) { }" and call the Abstract classes super constructors.

Original comment posted by wasserman.louis on 2012-07-12 at 04:09 PM

Eh? We can't control users making unsafe subclasses, and then dropping them in for use as Joiners in places that expect thread safety.

Or...I guess you're suggesting introducing a superclass in between? I'm not sure what advantage that provides over just letting users write their own classes that aren't "related" to Joiner.

Original comment posted by adam.g...@evocatus.com on 2012-07-12 at 04:30 PM

The benefit is not having to rewrite the dozen of methods that depend on "appendTo" that are useful. The inheritance is for code reuse and not contractual drop-in replacement support.

I suppose I can just open up the source code for Joiner and copy'n paste it into my own custom class so that I can make my own "JoinerAndMore" (most of my customization would be in the appendTo method).

I'll probably do that for now.

Original comment posted by wasserman.louis on 2012-07-12 at 04:34 PM

Eh. I mean, when we address figuring out an API that lets us deal with the escaping needs (as discussed in the issues Chris linked to earlier), I'm sure something like this will be considered.

And for specific needs, it's probably simpler to maintain a static utility method in your project that does X and Y on top of your Joiner.

Original comment posted by adam.g...@evocatus.com on 2012-07-12 at 05:24 PM

I hate to belabor the issue but just for doc purposes/clarification:

Maybe I'm missing something but there is no way to write a static method on top that will be as efficient (in general) because I don't have access to the Appendable.

I can transform collection/iterable/iterator and wrap the elements by creating new strings but that's not the same.

As for the escaping route proposed in issue 412 has the same problem in that you don't have access to the appendable.

Joiner.wrap(new SomeInterface() {
  public wrap(Appendable a, @Nullable String string) {
    return doescape(string);
  }
}).join("blah>><<<");

But that is god awful, requires another interface, exposes the appendable while its in process, and is less declarative then .prefix(String s) and .suffix(String s).

Original comment posted by cpovirk@google.com on 2012-07-12 at 05:31 PM

Hmm, I agree that, for full efficiency, you need to be able to get at the Appendable (unless Louis has any clever ideas...?). I'll make a note about that (as well as the SQL use case) on issue 412.

Original comment posted by wasserman.louis on 2012-07-12 at 05:33 PM

Eh? I'm suggesting a static method joinBracketed(Joiner, String leftBracket, String rightBracket, Iterable<Object> elements), for your personal utilties.

Original comment posted by adam.g...@evocatus.com on 2012-07-12 at 06:00 PM

@wassermain.louis I want to prefix and suffix each element in the "Iterable<Object> elements" and not the end result.

For example if I want to turn Iterable<Integer> into a String of "'1','2', and '3'" (that is I want to prefix and suffix the numbers with "'" and have the last one have an "and".

I believe there is no way with static methods operating on the joiner to do that efficiently. Do it I would have to do something like transform(Iterable<Object> input, Interable<String>) and then hand off the Iterable<String> to the joiner.

The to ability to handle the ", and" at the end... is basically the same logic as the joiner.

Original comment posted by wasserman.louis on 2012-07-12 at 06:09 PM

Ah. I've totally misunderstood the whole time. Yep, you'd need the transform.

Original comment posted by cgdecker@google.com on 2012-07-12 at 06:57 PM

Personally, I've been feeling somewhat sympathetic to the idea of adding an option to Joiner for wrapping elements with a prefix/suffix recently. It's a request that seems to have come up a number of times now, and Joiner certainly doesn't provide a decent way of doing it currently (not that either of those are necessarily a good reason to add it).

However, I don't feel like escaping is necessarily something that Joiner should have at all... it just feels too heavyweight somehow. To me, Joiner has always been about joining for relatively simple things, like toString() or other output, not for creating a string intended to be parsed later. For something like that, I feel like a CSV writing/parsing library is called for, and I don't want to see Joiner try to become that.

As far as the idea of an option that allows for inserting a different separator before the last element... I'm not too sure how I feel about that right now.

Original comment posted by wasserman.louis on 2012-07-12 at 07:00 PM

I mean, I can also imagine in THE FUTURE that we might have some kind of heavy-duty formatting utility with lots of bells and whistles that supports all kinds of these fancy options, and one of its components might be a Joiner. In that case, we might leave Joiner as-is, and add all the other stuff to this MagicFormatter utility...

I really don't know, but I'm not sure we should be ruling anything out at this point.

Original comment posted by adam.g...@evocatus.com on 2012-07-12 at 07:16 PM

One sort of crappy option is making an Appendable decorator.

BetterAppendable<A> ba = new BetterAppendable(A a); // where <A extends Appendable>

BetterAppendable implements Appendable and extends it with methods like .appendSeparator()

This is what Commons StrBuilder does http://commons.apache.org/lang/api-2.4/org/apache/commons/lang/text/StrBuilder.html#appendSeparator(char) ... except it doesn't wrap appendable.

Basically Commons StrBuilder as all sorts of methods including padding methods and what not.

... but bye bye thread safety.

Original comment posted by cgdecker@google.com on 2012-07-12 at 07:22 PM

I'm not ruling out the idea of an API in Guava that can handle CSV writing and parsing (it's actually something I'm kind of interested in, having experimented with writing one a while back), just saying that I don't think Joiner is the right place for something like that.

Original comment posted by adam.g...@evocatus.com on 2012-07-12 at 10:31 PM

I went ahead and wrote my BetterAppendable which is for the most part thread safe except for the fact that most Appendables are not.

I put the code in a gist:
https://gist.github.com/3101471

Here is an example:

   @Test
   public void testSeparator() {
      String results = new BetterAppendable<Appendable>(new StringBuilder())
         .separator(",").suffix("'").prefix("'")
         .append("Hello there lets count to 5")
         .appendLine()
         .nullText("hmm cookies...")
         .appendParts(1,2,3, null)
         .toString(); // can also do .getAppendable().getBuffer().toString()
      assertEquals("Hello there lets count to 5\n" +
            "1,'2','3','hmm cookies...'", results);
   }

The reason I prefer this is that most of the classes in Guava are semi idempotent.
I find the Joiner somewhat disturbing in that it will take a String and/or Appendable.

If the Joiner only operated on Strings then it would be idempotent and truly threadsafe (any time you operate on a mutable object like Appendable your IMHO not really threadsafe).

I think all Appendable mutations should be in separate classes like my BetterAppendable.

Original comment posted by cgdecker@google.com on 2012-07-12 at 10:43 PM

Idempotency is orthogonal to thread safety, and Joiner is thread safe (by virtue of being immutable). The Appendable input to join not being thread safe doesn't make the Joiner itself not thread safe. For example, 100 different threads could pass 100 different Appendables to a Joiner at the same time and there would be no problem as long as those Appendables were each kept on only one thread. If you were to mutate an Appendable from another thread at the same time a Joiner is appending to it there could certainly be a problem, but that's because the Appendable isn't thread safe, not because of Joiner.

Original comment posted by adam.g...@evocatus.com on 2012-07-12 at 11:27 PM

Hence the IMHO. Yes I agree by the Java definition of threadsafe Joiner is threadsafe. And the Appendable implementation I submitted is threadsafe also (because all the fields are final and immutable).

Not to get to pedantic but I'm comparing it to Haskell where there is a strict separation. Pure functions can't do bad things and therefore are "more" guaranteed to be threadsafe. That is a function could never operate on an appendable (with out some monad transformation).

But this is Java...

Original comment posted by cgdecker@google.com on 2012-07-13 at 05:46 PM

Well, your BetterAppendable (like any Appendable) is definitely stateful because it has to maintain state (what's been appended to it so far) from one append call to the next. It could be technically thread safe if, say, its methods were all synchronized, but it's not generally something you'd want to use from multiple threads. Joiner is totally different from that in that it is truly stateless and there's no reason at all not to use it from multiple threads at once.

Original comment posted by adam.g...@evocatus.com on 2012-07-13 at 06:03 PM

Sorry I meant AppendableDecorator class is ThreadSafe (it doesn't store the config) in: https://gist.github.com/3101471

I got confused with my renaming of the classes. I think some of the constructors need be made "friendly" and what not to keep users from extending but AppendableDecorator is threadsafe.

I'm basically tired of the plethora of String libraries out there. I blogged about it here: http://tmblr.co/ZJrhVxPGp1Gu

Original comment posted by adam.g...@evocatus.com on 2012-07-13 at 06:11 PM

I forgot in the AppendableDecorator I need to switch it back from CharSequence to String in the configuration its still not threadsafe... but its an easy fix.

Original comment posted by j...@nwsnet.de on 2012-07-16 at 08:37 AM

I personally prefer the Joiner to join. Transforming the values before joining them is the cleanest solution to me, whether it's about prefixing, postfixing, infixing, escaping the values or combinations of that.

Instead, what about a simple builder for such functions? StringTransformer.prefix('<').suffix('>').build(), something like that. Returns Function<String, String>.
The joiner could then be extended to accept such a function: Joiner.on(", ").transformValues(ENCLOSE_IN_ANGLE_BRACKETS).join(someValues)

This should be doable regarding imports as both Joiner and Function are in the base package (as opposed to earlier issues with other, non-join-related classes in the base package that couldn't use immutable collections, for example).

Original comment posted by cgdecker on 2012-07-16 at 01:24 PM

The main issue with just transforming with a Function to do this is that you have to use string concatenation for each element when ideally you'd like to just be appending to the Appendable.

Original comment posted by adam.g...@evocatus.com on 2012-07-16 at 02:45 PM

One solution is to do something like:

Add a method and interface to Joiner:

public <A extends Appendable> A appendTo(A appendable, Joiner.PartAppender<A> partAppender, Iterator<?> parts);

public static interface PartAppender<A extends Appendable> {
   public void append(A appendable, Object part, int index);
}

Obviously this is not ideal (adding another interface and method) but would fit most of my use cases.

Thoughts?

Original comment posted by wasserman.louis on 2012-07-16 at 02:59 PM

Eh. I'm still on the side of the Function crowd, which handles more general cases, including escaping, etc. that can't just be done with directly appending.

Original comment posted by adam.g...@evocatus.com on 2012-07-16 at 03:13 PM

Actually my way is way more flexible and powerful.

In my method you could append gigabytes of text together. Not to mention you now have control over how the object is transformed to a string.

So with the proper Joiner.appender you could do something like:

The code above will join files of any size together.

Original comment posted by wasserman.louis on 2012-07-16 at 03:24 PM

Eh? As I understand it, the proposals are fully equivalent in terms of what you can do with them. You could implement a Function<T, String> with a PartAppender by appending to an empty builder, and you can implement a PartAppender with a Function<T, String> in the obvious way.

That said, any major changes to the Joiner API will require considerable debate and discussion, and I think it's more likely we'd end up with an entirely new, separate API than making that sort of major change to one of the stablest APIs in Guava.

Eh. I don't think there's any need to rush into any decisions, just because the alternative workarounds aren't especially difficult or confusing, and the long-term plan for e.g. Joiner is likely to be critically dependent on how e.g. the escaper API ends up looking whenever it comes out.

Original comment posted by adam.g...@evocatus.com on 2012-07-16 at 03:52 PM

I completely agree. No rush on decision and I agree more discussion (it was the intent of my previous post).

I don't think anything should be added to the Joiner. And I am completly happy with the code I stole from it to use for my AppendableDecorator.

As far as implementing with Function<T,String> I think you forgot that you don't have access to the Appendable. Immutable String concatenation is not efficient for giant strings.

The only way to do this efficiently is for the Joiner to delegate writing the parts to the appendable to something else because right now the Joiner will do a #toString() on the parts object. That means no matter what you will have the part converted to a string in memory by the joiner.

The reason I keep bringing this up is that I work with very large amounts of data. So most of my algorithms work with Appendable's and Iterators and not Strings and Collections. Although I am sure it won't happen I don't want to see the escaper API
do something like:

It will need to be something like:

Original comment posted by adam.g...@evocatus.com on 2012-07-16 at 03:58 PM

Oh I didn't see he (@yo..) proposed adding a method transformValues(). I suppose yes you could do that with Function (although it seems rather against the contract of Function).

Original comment posted by wasserman.louis on 2012-07-16 at 04:01 PM

No, I wasn't suggesting a Function that internally appends to an Appendable; I was suggesting the solution you thought I was -- but I wasn't worrying so much about the performance. ;)

Original comment posted by kurt.kluever on 2012-07-24 at 05:41 PM

Let's discuss this at API review.

Status: Research

Original comment posted by cgdecker@google.com on 2012-07-26 at 10:44 PM

I think we've decided that we aren't going to add this functionality to Joiner. It will stay focused on simple joining. If we add something to do this, we'd want it to be part of (or at least based on) a more full-featured API for producing parseable strings (e.g. writing CSV strings), which would include escaping and such.

Status: WontFix

Original comment posted by wasserman.louis on 2013-04-30 at 03:19 PM

Issue #1392 has been merged into this issue.

List elements = Lists.newArrayList();
elements.add("ABC");
elements.add("DEF");
String str = Joiner.on("','").appendTo(new StringBuilder("'"), elements).append("'").toString();
==> 'ABC','DEF'

@jingwei027git and if the list is empty, it returns the string '', which is most likely an unwanted feature.