Is it a good idea for toString() to return null?

[Mariia Mykhailova <ma...@microsoft.com>]

Hi,

As I'm digging through Findbugs warnings, I've found a bunch of classes in org.apache.reef.runtime.hdinsight.client.yarnrest with toString() methods which can return null. They are using jackson for serialization and return null in case of any IOException (strangely, JsonGenerationException and JsonMappingException which can also be thrown have no special processing).

Is it a good idea? I agree with Findbugs warning - A" liberal reading of the spec could be interpreted as allowing this, but it is probably a bad idea and could cause other code to break. Return the empty string or some other appropriate string rather than null."

-Mariia

Re: Is it a good idea for toString() to return null?

[Sergiy Matusevych <se...@gmail.com>]

Hi folks,

Here's my $0.02:

1) toString() is an overloaded method, and it *always* should return a
String.
2) toString() results should be composable, i.e. an expression a.toString()
+ b.toString() should never thorow, and always produce a String.

I don't really know how we want to implement a deep toString() for all the
underlying fields of an object, but on the outside we should always return
a String.

Cheers,
Sergiy.




On Tue, Jan 5, 2016 at 11:32 AM, Markus Weimer <ma...@weimo.de> wrote:

> On 2016-01-05 09:34, Mariia Mykhailova wrote:
> > Any opinions? Personally I feel we shouldn't return a special value
> > for an otherwise good class which for whatever reason failed to
> > serialize, so we should either fall back to old conversion style or
> > throw an exception
>
> We can't change the definition of `toString()`, so that is out of the
> question. We also don't want to return `null`. I think throwing an
> exception is in order here:
>
> If we silently fix the serializer with hand written code, we then have
> to maintain this forever, negating the benefits of a serialization
> library in the first place. At the same time, this code really should
> never throw exceptions in real use. Hence, getting a bug report for when
> it does seems very useful :-)
>
> Markus
>

Re: Is it a good idea for toString() to return null?

[Markus Weimer <ma...@weimo.de>]

On 2016-01-05 09:34, Mariia Mykhailova wrote:
> Any opinions? Personally I feel we shouldn't return a special value 
> for an otherwise good class which for whatever reason failed to 
> serialize, so we should either fall back to old conversion style or 
> throw an exception

We can't change the definition of `toString()`, so that is out of the
question. We also don't want to return `null`. I think throwing an
exception is in order here:

If we silently fix the serializer with hand written code, we then have
to maintain this forever, negating the benefits of a serialization
library in the first place. At the same time, this code really should
never throw exceptions in real use. Hence, getting a bug report for when
it does seems very useful :-)

Markus

RE: Is it a good idea for toString() to return null?

[Mariia Mykhailova <ma...@microsoft.com>]

Any opinions?
Personally I feel we shouldn't return a special value for an otherwise good class which for whatever reason failed to serialize, so we should either fall back to old conversion style or throw an exception

-Mariia

-----Original Message-----
From: Mariia Mykhailova [mailto:mamykhai@microsoft.com] 
Sent: Thursday, December 31, 2015 9:51 AM
To: dev@reef.apache.org
Subject: RE: Is it a good idea for toString() to return null?

I don't think we can change method signature for toString() methods.
Should we return some special value instead? Something like "Failed to serialize"? Or should we fall back to old-fashioned field-by-field conversion to string?

 Some of these methods (probably all) were introduced in https://github.com/apache/reef/pull/171/ , that's when using jackson instead of field-by-field was suggested.

-Mariia

-----Original Message-----
From: Markus Weimer [mailto:markus@weimo.de]
Sent: Wednesday, December 30, 2015 10:27 PM
To: dev@reef.apache.org
Subject: Re: Is it a good idea for toString() to return null?

> I've found a bunch of classes in
> org.apache.reef.runtime.hdinsight.client.yarnrest with toString() 
> methods which can return null. [...] Is it a good idea?

No, returning `null` is never a good idea. We generally use `Optional<T>` on the java side to indicate when a method might return nothing.

I believe I wrote that code, and in great hurry when we prepared for the classes at the machine learning summer school. Sorry about that!

Markus

RE: Is it a good idea for toString() to return null?

[Mariia Mykhailova <ma...@microsoft.com>]

I don't think we can change method signature for toString() methods.
Should we return some special value instead? Something like "Failed to serialize"? Or should we fall back to old-fashioned field-by-field conversion to string?

 Some of these methods (probably all) were introduced in https://github.com/apache/reef/pull/171/ , that's when using jackson instead of field-by-field was suggested.

-Mariia

-----Original Message-----
From: Markus Weimer [mailto:markus@weimo.de] 
Sent: Wednesday, December 30, 2015 10:27 PM
To: dev@reef.apache.org
Subject: Re: Is it a good idea for toString() to return null?

> I've found a bunch of classes in
> org.apache.reef.runtime.hdinsight.client.yarnrest with toString() 
> methods which can return null. [...] Is it a good idea?

No, returning `null` is never a good idea. We generally use `Optional<T>` on the java side to indicate when a method might return nothing.

I believe I wrote that code, and in great hurry when we prepared for the classes at the machine learning summer school. Sorry about that!

Markus

Re: Is it a good idea for toString() to return null?

[Markus Weimer <ma...@weimo.de>]

> I've found a bunch of classes in
> org.apache.reef.runtime.hdinsight.client.yarnrest with toString()
> methods which can return null. [...] Is it a good idea?

No, returning `null` is never a good idea. We generally use
`Optional<T>` on the java side to indicate when a method might return
nothing.

I believe I wrote that code, and in great hurry when we prepared for
the classes at the machine learning summer school. Sorry about that!

Markus