Joiner.skipNulls() does not skip nulls if they were returned by Object.toString()

[GoogleCodeExporter]

In  public Joiner.skipNulls() this code:

        while (parts.hasNext()) {
          Object part = parts.next();
          if (part != null) {
            appendable.append(Joiner.this.toString(part));
            break;
          }
        }
        while (parts.hasNext()) {
          Object part = parts.next();
          if (part != null) {
            appendable.append(separator);
            appendable.append(Joiner.this.toString(part));
          }
        }

is wrong because it assumes that Joiner.this.toString(part) does not return null

In both loops there needs to be additional checks for Joiner.this.toString() 
returning a null

Original issue reported on code.google.com by transpar...@gmail.com on 30 May 2013 at 4:41

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Under what circumstances could Joiner.this.toString(part) possibly return null?

I have to admit that I'm pretty comfortable saying that if your objects' 
toString() method returns null, all bets are off.  I would tend to interpret 
the Object.toString() spec as forbidding that behavior.

Original comment by lowas...@google.com on 30 May 2013 at 4:46

[GoogleCodeExporter]

The problem is that we are trying to report an error with an object. And 
because the object is in error toString() returned null.

In an ideal situation, sure but we are trying to report an error and part of 
the error means that the object in question is not in a good state.

Original comment by transpar...@gmail.com on 30 May 2013 at 6:12

[GoogleCodeExporter]

It sounds like you had a perfect storm. Whatever the real problem you were 
trying to report, you had an even worse problem of an object whose toString() 
method could return null. That's a programming mistake plain and simple, and 
like Louis, I don't think there's a need to add special checks to core 
libraries to protect against mistakes like that.

Original comment by kevinb@google.com on 26 Jun 2013 at 12:09

• Changed title: Joiner.skipNulls() does not skip nulls if they were returned by Object.toString()
• Changed state: WorkingAsIntended

[GoogleCodeExporter]

I also have legitimate cases of objects that are not null and implement 
CharSequence. Returning null for those objects is also legitimate.

I am puzzled why putting a null check in would violate any good practices 
rules? Please enlighten me as to exactly how a confusing exception is correct 
behavior?

I am always trying to become a better programmer.

Original comment by transpar...@gmail.com on 26 Jun 2013 at 12:43

[GoogleCodeExporter]

> I also have legitimate cases of objects that are not null and implement 
CharSequence. Returning null for those objects is also legitimate.

I really don't understand how returning null from the toString() implementation 
of a class implementing CharSequence is "legitimate". CharSequence's toString() 
method is documented as follows:

"Returns a string containing the characters in this sequence in the same order 
as this sequence. The length of the string will be the length of this sequence."

It returns "a string" (null is not a string and doesn't have a length)... if 
your CharSequence has no characters in it, toString() should return the empty 
string. 

Original comment by cgdec...@gmail.com on 26 Jun 2013 at 1:22

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<issue id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:12

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

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

• Added labels: Restrict-AddIssueComment-Commit