A lead developer on my project has taken to referring to the project's toString() implementations as "pure cruft" and is looking to remove them from the code base.
I've said that doing so would mean that any clients wishing to display the objects would have to write their own code to convert the object to string, but that was answered with "yes they would".
Now specifically, the objects in this system are graphic elements like rectangles, circles, etc and the current representation is to display x, y, scale, bounds, etc...
So, where does the crowd lie?
When should you and when shouldn't you implement toString?
What harm do they do? Why remove them if you have them. I find toString() extremely useful when emitting debugging statements.
ReplyDeletePersonally, I would always err on the side of having a workable toString() method. So little work to write.
Removing well-written (or even halfway decently written) toString() methods is pure insanity, IMO. Yes, I am often too lazy to write these (as often the objects don't end up having them used anyway), but they are extremely handy to have.
ReplyDeleteI really can't think of a good reason to want to get rid of these.
I've always made sure that my classes implemented toString.
ReplyDeleteIt provides a simple way of debugging the current state of the class when I'm debugging and when I'm logging errors, I can include it into my log messages.
I'd keep the toString() implementations. They are invaluable when it comes to debugging, and they can make for good alt text for graphical components.
ReplyDeleteI would argue the opposite that toString() should be overriden judiciously. The default toString() implementation is very uninformative and basically useless. A good toString() implementation can give a developer a very useful view of the contents of the object at a glance. You may not have to put everything in there but at least the important stuff. I think your lead developer should be actually coding and adding features rather than worrying about "cruft".
ReplyDeleteI would only implement it for more complex objects where client code doesn't care about the fine grained details of the object state, but rather care about some more human understandable, sense making message, that summarizes what is going on, state wise...
ReplyDeleteFor everything else, like JavaBeans, I would expect client code to throw my object into a ToStringBuilder method or similar, if it needs to do low-level debugging.
ToStringBuilder.reflectionToString(myObject);
Or client code should just call standard property getters and log the way they like...
In general, toString() is good stuff. In particular, it is quite useful for debugging.
ReplyDeleteImplementing toString() is not without costs and risks. Like all code, toString() implementations must be maintained with the rest of the code. This means keeping toString() in sync with class fields. For example, when a field is added or removed, toString() should be updated appropriately (you should be doing this already for methods like hashCode() and equals()).
Implementing toString() incurs risks as well. For example, suppose that two classes in your system have references to instances of the other (a bi-directional link), then an invocation of toString() could lead to a stack overflow due to unbounded recursion as the toString() implementation in each class invokes the toString() implementation of the other class.
If your system has a significant number of out-of-sync toString() methods, or methods that cause bugs like stack overflows, then your colleague might have a reasonable point. Even in such a case, I would simply comment-out the buggy toString() methods and leave them in the code. Each toString() method could be uncommented and updated individually as needed in the future.
Well, he does stramge thing.
ReplyDeleteI can not say toString() is too useful. For presentation you will need other tools.
But toString() is quite useful for debugging, because you could see the contents of collections.
I do not understand why remove it if it is written already
I think the answer depends on how complicated your toString() methods are, how much work they require to maintain, and how often they get used. Assuming you use toString() often for logging and debugging it doesn't make much sense to remove these methods. But if they are rarely used and require a lot of work to maintain each time something changes in your code, then there is perhaps a valid argument for getting rid of all or some of the toString() methods.
ReplyDeleteYou mentioned something about clients needing to display these objects. From this I'm guessing your code is or contains some kind of library or API that other developers will use. In this case I highly recommend you maintain useful toString() implementations. Even if you don't do a lot of logging and debugging, your clients might and they will definitely appreciate having useful toString() methods that they don't have to write and maintain themselves.
+1 Mike C
ReplyDeleteOver and above its usefulness for debugging, the toString() is an invaluable tool to understand the class author's perspective of the instance.
FWIW, if the output of the toString differs from what you expect to see (courtesy spec docs) you will know rightaway something went seriously wrong.
Personally, I implement them when I'm going to use objects in a JList, JTable, or other structure that uses toString() OR when I'm debugging (yes, eclipse has debug formatters but toString() is easier).
ReplyDeletePerhaps you can fire back that many JDK classes have toString(). Should they be removed as well? ;)
I would say you should implement toString if that is an expected use case or requirement, to display the object as a string representation (either in logs, on the console, or some kind of display tree).
ReplyDeleteOtherwise, I agree with the developer - every time you change something, the toString can break. You may have to be careful about nulls, etc.
Many times, though, it is in fact used in debugging or in logging, so it isn't obvious that they should be left out at all.
I agree with jsight that if they are already written, and written decently, leave them in at least until they get in the way (such as you actually add a field to a class).
For debugging purposes, no one can beat toString. It's practical both in a debugger, and in simple debug prints. Make sure it displays all the fields that your equals and hashCode methods are based on, if you override those as well!
ReplyDeleteFor display to end users, I wouldn't use toString, though. For that, I think it's better to write another method, that does the proper formatting, and i18n if you need that.
It makes good sense because you always have problems with toStrings showing too little or too much information.
ReplyDeleteIt may make sense to your team to use the ToStringBuilder in Jakarta Commons Lang instead:
System.out.println("An object: " + ToStringBuilder.reflectionToString(anObject));
which introspects the object, and prints out the public fields.
http://commons.apache.org/lang/api-2.3/org/apache/commons/lang/builder/ToStringBuilder.html
I've said that doing so would mean
ReplyDeletethat any clients wishing to display
the objects would have to write their
own code to convert the object to
string, but that was answered with
"yes they would".
This is not a question that can be answered in isolation ... you should ask the clients (or the people writing them) what they think of the idea. If I was using a Java library and relying on its toString() overloads for debugging, I'd be rather annoyed if the library's developers decided to purge them.
To be fair, said developer here, but not lead developer in any sense.
ReplyDeleteThe original issue is not necessarily about toString(), but about a second method paramString:
"With all of its string concatenation and null value checking, paramString is a bug magnet."
See http://code.google.com/p/piccolo2d/issues/detail?id=99
I would definitely keep the toString() implementations, particularly for debugging purposes. As a primarily C++ developer, I wish things were as easy in C++ as they are in Java in this respect (operator overloading can be a pain).
ReplyDeleteIf there is a problem with the existing toString() implementations, the developer should fix the problem. Saying the current implementations are all "pure cruft" and removing them is actively doing harm, unless the existing toString() methods are uncommonly poorly written.
ReplyDeleteI would strongly discourage the developer from removing any functioning toString() method.
I always auto-generate toString() methods for all my POJOs, DTOs and/or any object that holds persistent data. For private internal properties good logging practice should do the trick.
ReplyDeleteAlways remember to replace in toString methods passwords and other sesitive info with [Omitted] (or something similar of top secrete nature)
implement always :) As mentioned above, it is invaluable for debugging.
ReplyDeleteIt is good for debugging purposes. But if you want to display given object as a string to the end user you should never use toString() implementation but provide custom method for that.
ReplyDeleteSo regarding
I've said that doing so would mean
that any clients wishing to display
the objects would have to write their
own code to convert the object to
string, but that was answered with
"yes they would".
I agree with your team lead. If you want to display object to any client use custom implementation. If you want to use it for debugging purposes use toString().
We're getting a ConcurrentModificationException thrown out of one of our toString() methods, so there's an occasional drawback. Of course it's our own fault for not making it synchronized.
ReplyDelete