Objects.toStringHelper has minor bugs now

[GoogleCodeExporter]

Glad to see ToStringHelper just use a StringBuilder internally now, I had 
wanted something like that back in issue 400.

But two behavior changes were introduced with the change:

- If a null name is provided to add() then the separator will be appended prior 
to the NPE being thrown. Ideally, a precondition happens pre anything 
destructive, right?

- toString() cannot be called repeatedly without it adding extra close curlies. 
I'm not entirely sure why you'd want to call it repeatedly, but the original 
implementation seemed to accommodate it. Perhaps one would want to toString() 
some initial details, then add more to the same ToStringHelper, and toString() 
it again...

Both of these are simple to fix. See attached diff.

Original issue reported on code.google.com by ray.j.gr...@gmail.com on 3 Jun 2011 at 1:27

Attachments:

• Objects.diff

[GoogleCodeExporter]

I imagine it wouldn't hurt to make these changes, but I don't feel like either 
could really be considered a bug given how ToStringHelper is intended to be 
used (for building one String and returning it in toString). I can't imagine 
anyone catching NullPointerException and then using the ToStringHelper anyway 
in toString (it can only fail from programmer error generally) and I can't 
really imagine anyone needing to make two Strings with one ToStringHelper 
either.

Original comment by cgdec...@gmail.com on 3 Jun 2011 at 2:10

[GoogleCodeExporter]

"Bugs" may have been the wrong word, even prefixed with "minor".

Feel free to skip it, I personally wouldn't use ToStringHelper in a way that 
triggered these issues.

I suppose there's a distinction to be made between API contracts and typical 
use. Iterator is typically used by calling hasNext() prior to any next(), but 
the documentation on the interface doesn't require that hasNext() be called to 
prepare the next element. Even so, I bet there are Iterators out there in the 
world that do need to have hasNext() prior to each next(). These would work 
perfectly fine in normal use, but they don't quite follow the spec and might 
break if someone decided to forego hasNext() and instead stop iterating by 
catching NoSuchElementException.

So I only brought up these issues because I had previously looked at the 
implementation of ToStringHelper and saw that it supported repeat calling of 
toString(). I figured the idempotency was a feature and merely wanted to point 
out that along with the implementation improvement this feature was lost. If 
this was never intended to be a feature then maybe add a little documentation 
like "The result of calling toString more than once is undefined", or something.

I'm totally OK with leaving the spec a little loose and merely documenting the 
expected usage pattern. It's not always worth it to either make it idempotent 
or track that toString() has been called and throw an IllegalStateException on 
each method after that.

Original comment by ray.j.gr...@gmail.com on 3 Jun 2011 at 5:49

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

it is not uncommon for a debugger to call toString() at arbitrary times.
I really think toString() is expected to be idempotent

Original comment by bill.cla...@gmail.com on 3 Jun 2011 at 9:08

[GoogleCodeExporter]

I guess I personally just don't see the point in worrying about this at all, 
much less adding extra code (albeit very little) that will always run when 
ToStringHelper is used just so someone trying to use ToStringHelper in a way it 
isn't intended to be used doesn't get an extra "}". I don't think a comparison 
with Iterator makes much sense here given that Iterator is an interface with an 
extremely broad range of uses and many different implementations. 
ToStringHelper has one very simple way it's intended to be used (as shown by 
examples in the javadoc) and one implementation.

@bill: It doesn't matter when or how many times an object's toString() is 
called since ToStringHelper is only intended to be used locally inside a 
toString() method, and the ToStringHelper's toString() method should only be 
called once inside toString().

Anyway, I'm not making any decision about this myself, just expressing my 
thoughts as a Guava user!

Original comment by cgdec...@gmail.com on 3 Jun 2011 at 9:40

[GoogleCodeExporter]

I'm with Bill now. toString() is inherited from Object, it's not a new method 
that can act how you want it to. There's nothing specifically requiring that an 
Object's internal state should be unmodified by calling toString(), but it 
seems like a good idea.

A debugger might do it, or someone may decide to log their ToStringHelper and 
depending on the log level toString() may or may not be called...

Re: "only intended to be used locally".
ToStringHelper may be passed around, I thought that half the point was to ease 
the addition of details in subclasses:

public class Foo
{
    public String toString ()
    {
        return toString(Objects.toStringHelper(this)).toString();
    }

    protected Objects.ToStringHelper toString (Objects.ToStringHelper tsh)
    {
        return tsh
            .add("x", x)
            .add("y", y);
    }
}

public class SubFoo extends Foo
{
    @Override protected Objects.ToStringHelper toString (Objects.ToStringHelper tsh)
    {
         return super.toString(tsh)
             .add("z", z);
    }
}

Original comment by ray.j.gr...@gmail.com on 3 Jun 2011 at 9:57

[GoogleCodeExporter]

My impression that there was only a ToStringHelper class at all because some 
type needs to exist to enable the fluent builder interface. I hadn't considered 
something like you describe where one is passed around, but even in that 
scenario toString() is only being called on the helper once (because a 
toString() method can only ever return one string). I don't know, maybe it's 
worth it to do something about this but something about it feels wrong to me.

Original comment by cgdec...@gmail.com on 3 Jun 2011 at 10:19

[GoogleCodeExporter]

I also thought it was weird when I saw that the new version of ToStringHelper 
was not idempotent. I didn't create an issue, because it's not really a problem 
if you use it as it's meant to be, but I see how it could be confusing.

I guess documenting this behavior might be a good idea, as Ray suggested. We 
could also reuse the "separator" variable and do something like:

    @Override public String toString() {
      checkNotNull(separator, "ToStringHelper.toString() must be called once");
      separator = null;
      return builder.append('}').toString();
    }

Not sure if it's really necessary, though.

@ray:

I also do something similar to simplify toString() when subclassing:

public class Foo {

    public String toString() {
        return toStringHelper().toString();
    }

    protected Objects.ToStringHelper toStringHelper() {
        return Objects.toStringHelper(this)
                .add("x", x)
                .add("y", y);
    }
}

public class SubFoo extends Foo {

    @Override
    protected Objects.ToStringHelper toStringHelper() {
        return super.toStringHelper()
                .add("z", z);
    }
}

I like this version better because it avoids the indirection of passing an 
Object.ToStringHelper as a parameter. I think it's more readable. Either way, 
they both call ToStringHelper.toString() only once, so the bug is not exerted 
by this pattern.

Original comment by nev...@gmail.com on 3 Jun 2011 at 11:10

[GoogleCodeExporter]

@nev: Yes, that's better. Less code is better code.

Original comment by ray.j.gr...@gmail.com on 3 Jun 2011 at 11:50

[GoogleCodeExporter]

And: @nev: I think having toString() throw an NPE is for sure a bad idea.

toString() should be idempotent. It should be callable repeatedly. It's 
inherited from Object, its behavior isn't something we can redefine.

Original comment by ray.j.gr...@gmail.com on 6 Jun 2011 at 6:10

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

I was not expecting toString() to throw an NPE. The "standard" use of 
Objects.toStringHelper() (as described in the Javadoc) should never call 
"build()" more than once.
I suggested the checkNotNull() to "fail fast" in case somebody misuses the 
ToStringHelper. The idea was to enforce the "correct" use. I guess the 
following would have been more appropriate, though:
    checkState(separator != null, "ToStringHelper.toString() must be called once");

That being said, I pondered about this issue in the subway, and I agree with 
your suggestion. ToStringHelper *should* be idempotent, to obey the principle 
of least surprise. Ensuring this might force us to lose a tiny bit of 
performance in the build() method, but I think it's worth it. Plus, if your 
toString() is really performance-critical, you might want to consider 
implementing it manually...

I would have fixed it by doing the following, though:

@Override
public String toString() {
   return builder.toString() + '}';
}

Not sure about the performance implications of doing this versus what you 
suggested (constructing a new String instance versus how the JVM manages a 
finally block), but I think it's cleaner...

Original comment by nev...@gmail.com on 6 Jun 2011 at 7:54

[GoogleCodeExporter]

Internally I've submitted a fix for the issues raised here...it should make it 
out in the next release.

Original comment by kurt.kluever on 6 Jun 2011 at 9:09

• Changed state: Fixed

[GoogleCodeExporter]

FYI: http://code.google.com/p/guava-libraries/source/detail?spec=svn452&r=449

Original comment by kurt.kluever on 7 Jun 2011 at 6:24

[GoogleCodeExporter]

Well, that's weird. What's the idea behind the cached string? That seems far 
less efficient than the get/set of the StringBuilder's length.

With this cachedString impl, the chars in the StringBuilder are copied into a 
new String, and then that String is copied into another new String just to add 
the "}" on the end. The whole point of a StringBuilder is to avoid the 
inefficiency of string concatenation.

Getting the length of a StringBuilder is a simple accessor, and setting it 
again does a few checks, but setting to a length smaller than the current 
length is essentially a simple setter. That's gotta be way cheaper than 
creating an extra String for no reason.

Plus, we've well established that NOT calling toString() multiple times is the 
common case. This seems like a misguided optimization for calling toString() 
multiple times...

Original comment by ray.j.gr...@gmail.com on 7 Jun 2011 at 6:37

[GoogleCodeExporter]

Can someone explain any use case for calling toString() on a ToStringHelper 
multiple times? Storing one in a field and reusing it doesn't seem viable 
considering that it's not thread safe (plus you can't remove fields from it, 
etc.).

Original comment by cgdec...@gmail.com on 7 Jun 2011 at 6:58

[GoogleCodeExporter]

Did this thread just reboot?

The only "use case" I can think of is that you, for some reason, output some 
details, then add more and output those too...

But nobody's saying they want to do that.

It was pointed out that a debugger may call toString() on arbitrary objects, 
and it seems to me like it's a bad idea to make something as fundamental as 
toString() change the internal state of the object.

Original comment by ray.j.gr...@gmail.com on 7 Jun 2011 at 7:18

[GoogleCodeExporter]

I first posted this in a comment on the commit, but I think it's better to post 
it here:

---------------------------

- I agree with moving the checkNotNull() at the beginning of the add() method 
(good for failure atomicity - Effective Java, Item 64).

- Moving the checkNotNull() at the beginning of the constructor is un-needed 
from a failure atomicity standpoint, but why not.

- I think the "cachedString" field is a bad idea and should be removed. It 
needlessly complicates the ToStringHelper's code. The common use-case is to 
call the ToStringHelper's build() method only once. Caching the String would 
improve the performance of multiple successive calls (and I'm not even sure it 
would improve the performance that much anyway). IMO, it's not necessary to try 
to improve the performance of the uncommon use-cases.

I would remove the "cachedString" and do instead:

    @Override public String toString() {
         return builder.toString() + '}';
    }

This fixes the issue, without complicating the code too much.

I also like Ray's suggestion of using setLength(), though it's a tiny bit more 
complicated (but might be more performant).

Original comment by nev...@gmail.com on 7 Jun 2011 at 8:38

[GoogleCodeExporter]

OK, based on some of the suggestions here, I'll submit a new patch internally. 
Thanks.

Original comment by kurt.kluever on 7 Jun 2011 at 8:53

[GoogleCodeExporter]

http://code.google.com/p/guava-libraries/source/detail?spec=svn459&r=458

Original comment by kurt.kluever on 10 Jun 2011 at 5:37

[GoogleCodeExporter]

Looks good.

Original comment by ray.j.gr...@gmail.com on 13 Jun 2011 at 5:13

[GoogleCodeExporter]

Looks good to me too. I also like the simplification of the separator handling 
(using a boolean instead of a String).

Original comment by nev...@gmail.com on 13 Jun 2011 at 8:02

[GoogleCodeExporter]

Original comment by kevinb@google.com on 18 Jul 2011 at 3:33

• Added labels: Milestone-Release10, Type-Defect

[GoogleCodeExporter]

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

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

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

• Added labels: Restrict-AddIssueComment-Commit