
Never Use toString() for Behaviour - sindrebn
https://java.christmas/2019/4
======
josefx
>If left unoverridden, it only yields a description of it’s class and location
in memory

It contains the hashCode(), not the memory location. The default
implementation of hashCode could use a memory location to generate the hash,
however that isn't the case in all implementations. OpenJDK seems to have used
an RNG by default in the past and currently derives a value from the thread
state[1]

[1][https://srvaroa.github.io/jvm/java/openjdk/biased-
locking/20...](https://srvaroa.github.io/jvm/java/openjdk/biased-
locking/2017/01/30/hashCode.html)

~~~
benmmurphy
for most implementations it would not be possible to use the location in
memory because most java garbage collectors will move stuff around in memory.
they could use the location initially and cache it afterwards but this would
seem tricky to do without breaking the hash code contract.

~~~
josefx
I think the code also has the option to read and store the address. However I
also think it would have other problems. Heaps with less than 4GB size would
put a limit on possible hash code values and object alignment requirements
would make it even worse, so you never get use out of the full 32 bit
hashCode. Then you have generational GCs and if I understand those correctly
you could end up with a lot of objects that started out at the same address
and if you call hashCode on them before they are moved to a long lived
generation you almost guarantee a large amount of hash collisions down the
line.

------
vlaaad
Agree in general, but note that Java itself does not follow this rule:
StringBuilder's and StringWriter's toString methods use toString for behavior
— creating a string from accumulated chars, and I'm fine with that.

~~~
nathanaldensr
This also definitely does not apply to .NET where there are many classes that
override ToString for meaningful, well-documented behavior (StringBuilder is
one, funnily enough). It's best to check the docs or look at IntelliSense
before applying this rule in a blanket fashion.

That said, it does seem like Microsoft has since moved away from overriding
ToString for such things. In all honesty, Object.ToString was probably a
mistake to begin with.

~~~
simendsjo
There's nothing wrong with returning something well-specified from toString,
but I agree this should never be relied upon. And even if toString actually
return something well-specified, the same functionality should be available as
a distinct method.

Any form of (semi-)automatic SomeObject -> String should be treated as a
convenience for the developer to avoid getting a non-descriptive memory
location reported.

Having the automatic toString isn't the problem, but maybe the name of the
method is. It could be called "asDebugHelpRepresentation" or something more
descriptive.

------
pavlov
Java's eccentric uncle Objective-C called this method "description" instead of
"toString". That was a better name IMO because it avoids the suggestion that
it's some kind of canonical string representation.

~~~
adrianmonk
Yeah, it really would be helpful if the method name made that clear.

The language documentation does make it clear what toString() is for. And it's
convenient to be able to (for example) stick objects and strings together with
the "+" operator and have conversions automatically happen.

But (understandably) not everyone reads the language documentation cover to
cover before they start writing Java code, so you can't count on that making
it obvious.

~~~
chopin
java.lang.Object has a few methods though. At least those should be known when
one starts writing productive code. Otherwise one produces even worse bugs
than relying on toString behavior (equals and hashCode, god forbid).

Cover to cover would mean understanding the memory model and maybe garbage
collection. That's too far certainly but a few basics would do no harm.
toString is surely one of them.

~~~
user1980
I'd hazard a guess that 0.1% of Java developers are aware that on some
operating systems Object.wait() can awaken spuriously. Heck, it wasn't even in
the Javadoc until 1.5. And don't even get me started on how you should handle
an InterruptedException.

Most Java developers I know consider rolling your own multi-threading scheme
about as wise as rolling your own cryptography, and just pretend
notify/notifyAll/wait don't even exist.

~~~
Bartweiss
Ironically, that's one of the few things I _do_ know off the top of my head
about Object.wait(). I'm far from an expert on Object's threading methods, but
"this will burn you, don't try it" is becoming more pervasive than "here's how
to do this". Which is almost certainly the right order to learn those things
in.

------
flerchin
We use lombok, and the toString annotation is immensely helpful for logging
and avoiding this type of thing.

~~~
fahadkhan
How does Lombok help to avoid this?

~~~
nsporillo
Lomboks annotation @ToString will automatically generate the typical
overridden toString() method based on the fields.

~~~
chopin
I just tried this in Java and I ran quickly into circularity issues (one of
the fields has an indirect reference to the same object, say a list which the
current instance is part of). How does Lombok cope with that?

It's not that easy as I thought (see my sibling comment).

------
bluesign
> If you used the returned Country to construct an API request including the
> name of the country, this will now produce strange countries named as
> “Optional[“Norway”]”

I am not too familiar with java, but should’t you call get() from optional to
get Country from Optional<Country> and toString of Country will behave as
expected.

~~~
oftenwrong
As the sibling comment says, the post's advice is really about making your
code refactoring-friendly with regard to type checking.

You generally would not want to call '.get()' without an 'isPresent()' check
because it will throw a NoSuchElementException if the Optional is empty. This
is a foreseeable case, so most would like to handle it somewhat explicitly.

Today, it is idiomatic to use '.map' to transform it:

    
    
        Optional<String> foo = optionalCountry.map(Country::getName);
    

You can also throw a domain-specific exception from that point:

    
    
        Optional<String> foo = optionalCountry
            .map(Country::getName)
            .orElseThrow(...);

~~~
chopin
orElseThrow does return a String, not an Optional. You also could use
orElse(null) or orElse("null") depending on the use case (eg. using as a part
of toString).

~~~
oftenwrong
I'm useless without a compiler to check my types!

------
selbekk
Cool article! It's a simple mistake to make when starting out - good to get a
nice explanation of why it's a bad practice

------
t0astbread
Additionally, relying on `toString()` could cause problems when multiple
different representations are desired.

------
mrkeen
Never use a language that puts a toString() implementation on every object
regardless of whether or not that object implements toString()

~~~
kanox
The ability to convert any object to a string is very useful for debugging and
most languages support this. Even in C it's frequently useful to print %p to
identify objects.

Serialization should require involve separate APIs.

~~~
drainyard
It should be opt-in though. A compiler-flag for debug builds or similar.

~~~
chris_overseas
Why so, what are the downsides? It's not like it introduces any meaningful
overhead at all, and there are plenty of situations where it is useful even in
production (eg for logging in catchall "can't happen" top level exception
handlers). Adding yet another compiler flag seems like a worse solution to me.

~~~
mrkeen
What you lose is the ability to model objects based on the behaviours that
they can perform.

I want to be able to design objects that have toString() and I want to be able
to design objects that don't have toString().

The same goes for clone(), hash(), getPtr(), getEndian(), equals(), toBytes(),
toJson(), encrypt(), delete(), isNumeric(), toXml(), serializationVersion() or
compare().

~~~
Bartweiss
I suppose you could define toString() to throw an error, but obviously that's
a messy hack in place of having that control. I can definitely appreciate the
logic for making nothing intrinsic, and I can also see an argument for making
equals() the only method of Object. (i.e. everything should have an identity
function, but nothing more.)

Now I'm wondering... are there languages which formalize "implement with a
throw" into some kind of explicit refusal to implement a method? Obviously
there are method-sharing approaches other than inheritance, but I've never
heard of "you must implement this, or explicitly choose not to".

~~~
iudqnolq
I'm not quite sure what you mean, but it's standard in python if you have a
method in a class that must be subclassed and overridden you can have the
default raise a NotImplementedError

~~~
Bartweiss
I was thinking about a condition that's specifically for "inherited but not
supported", but that's pretty close to what I had in mind, yes.

Java doesn't have a language-level exception that's specifically for unusable
methods, which can be a bit awkward. UnsupportedOperationException is the
recommended answer, but it's relatively nonspecific as to _why_ your operation
wasn't supported. Apache's Commons extends that with NotImplementedException
for cases where the call isn't definitionally impossible (e.g. adding to an
unmodifiable map), but no implementation exists.

------
kasperni
That was a complete waste of time.

To sum it up: don't rely on the format of a generic toString() method. Prefer
country.getName() over country.toString(), even if toString() returns
getName().

~~~
netsharc
It's cool to read, but yeah, it's like reading Beginner's Java, lesson 3.

~~~
simendsjo
More like "beginning software architecture". While this is "common knowledge"
after coding for years, it's actually hard-earned, battle-scarred knowledge.
Sharing experience on how to design software is very useful for all the people
just starting out.

