(Not So) Stupid Questions 17: Should Code be Clean or D.R.Y.?
07/12/2007
Editor's note: Sometimes the most interesting discussions begin when someone says, "This may be a stupid question, but ...." If the person asking the question has taken the time to think about the problem before asking, the question is often not stupid at all. The uncertainty points out an ambiguity in the specs, holes in the docs, or a search for how more experienced programmers might address a particular problem. From time to time, we will print one of the "(Not So) Stupid Questions" we receive and invite our readers to answer the question in the feedback section.
Remember that new people are joining the Java community all the time and may be looking for help from those with more experience. Also, those who began with Java as their first language can benefit from those coming to the community with experience in other languages. As always, answer the questions with kindness. You are also welcome to submit your questions to
This may be a stupid question, but ... "Should you prefer 'clean' code (e.g., sound object-oriented design) or D.R.Y. code?"
First Thoughts
Say we have two unrelated base classes, Fruit and Vehicle. Both these classes have their own trees of subtypes (Apple, Banana, Car, Truck) and both also have fancy toString() methods that use reflection to iterate over all their fields and build their toString()s. The toString() code is generic and useful for their subtypes, but is also identical in both Fruit and Vehicle.
Should you prefer:
Leaving the few (say, dozen) lines of code duplicated in both Fruit and Vehicle?
Factoring the code into an artificial ObjectWithGenericToString base class (i.e., breaking the logic of inheritance, but maintaining encapsulation)?
Factoring the code into a static ToStringUtils class (i.e., breaking encapsulation, but maintaining the logic of inheritance)?
Factoring the code into a "uses" relationship and build a ToStringManager subsystem (i.e., maintaining encapsulation, but complicating the architecture just for a few lines)? Let us assume no third-party systems already exist for this (such as Commons Lang).
In general, I think c) is the right answer, but (assuming package-private is not an option, because Fruit and Vehicle are in different packages) this leads to general purpose "util" packages that border on functional programming.
Spring does it right
2007-08-06 04:05:32 jon077
[Reply | View]
Spring does it right. You encapsulate the styling in a ToStringStyler and to create the toString() methods, you instantiate a ToStringCreator, pass it the object and your styler, then append the properties you want to have printed.
When you are done, call toString() on the ToStringCreator and it prints a consistent style that will recursively call toString() on all the properties in the object tree.
As for the DRY debate, if you are breaking DRY, think about changing your design, but as with all programming, weigh the options and make the best decision for your application.
The correct answer would be E :)
2007-07-22 06:08:39 deniskulik
[Reply | View]
IMHO, the correct answer would be E, which is none of the given above:)
Consider the following:
1. The purpose of the toString() method is to provide the textual representation of the object in the most informative but reasonably compact way. This is typically completely different task than simply printing out values of all fields of the object.
2. The other thing is that most of the time we would use the toString() method to expose values of the fields which are publicly available anyway. This again may differ from the requirement to print out values of all fields, as such fields may exist for private use only and should not be exposed to the outside world.
3. Bearing in mind the above points we may think that in order to solve our problem correctly it would be better not to use the toString() method for printing out values of all fields, but to provide some other method instead.
4. Now, let us look at the requirement of printing out values of all fields a bit deeper. First, it is more likely that we would need to adhere to some format which will have to be the same regardless of the class of the object. Second, the logic of performing our task will be the same regardless of the class of the object. Hence, implementation of our new method in each of the classes will result in duplication of code which itself will inevitably cause a new problem of maintaining the code should we wish to modify the code or change the format of the output string.
5. The other thing we need to bear in mind is that although we want to print out values of all fields now, we may have some exceptions to this requirement in the future. It therefore would be wise to provide some mechanism which would allow us to work around such exceptional cases.
With the above points in mind, we can now solve the problem like this. First, let us create an interface and call it Printable as follows:
public interface Printable
{
public Map getPrintableFields()
}
As you may have guessed the returned Map contains the names of the fields as keys and the values of the fields as values.
Second, let us create the class called Printer as follows:
public class Printer
{
public static String print(Printable printable)
}
Again, as you may have guessed this class will contain the logic of iterating through the Map returned by the specified Printable object and printing out the contents of the Map in a particular format.
There are a number of benefits associated with this approach:
1. There is no code duplication and the whole system is much cleaner and easier to maintain.
2. Using the Printable interface allows us to use any class in our hierarchy without using inheritance.
3. This approach implies that it is the responsibility of the implementer of the Printable interface to decide which of its fields should be exposed, which allows us to accommodate the exceptional cases which we discussed in item 5.
4. Unlike reflection, interfaces reinforce requirements to their implementers, so we dont have to rely on our class to provide a method for exposing its fields, we know the method is there.
5. Note that we don't have to deal with all those checked exceptions that may be thrown as a result of using reflection.
6. Its now very easy to enhance the Printer class to produce the string output in different formats and to choose a particular output format at run time.
As a final note, duplication of code is always a bad thing and should be treated as a signal to reconsider your design. Not only duplication of code requires double (at the very least) effort to write the code in the first place, it requires duplicate testing and duplicate maintenance in the future.
The question is do the two classes share the same functionality because they are joined by some kind of relationship, or is it just co-incidental?
If they share functionality via some relationship, even if it's a remote relationship, then use the class hierarchy to model that relationship.
If they share functionality merely by co-incidence, then duplicate the code. This allows both implementations to be independent, and change in future without impacting on each other. In other words, don't impose a connection which isn't implicitly there.
There is a third option... toString() is a utility method, and sometimes broad-appealing, generic, functionality is best placed in a separate utility library, and referenced from there. As pretty much all software can be segmented into those parts which directly model the problem (typically the underlying data structures and their associated code) and supporting utility code outside the direct scope of the problem, but needed to "make the program function" (typically I/O, user interface, etc.) then there's no shame in this two-headed approach.
toString() is probably a bad example, as it is very clearly a utility method, so employing a supporting utility library is a natural step. Other examples may require a different solution. It depends on the context of the duplication.
Well I've used all of the aforementioned approaches, but the one that I tend to lean towards is the Util method approach. I'm not a fan of overriding the #toString() anyway, you really should be using a java.text.Format or a renderer of sorts (don't worry, I'm not missing the point, I just want my two cents). I think #toString() was a poor example as it should rarely be touched.
Though I prefer the Util method approach, I can think of examples where all of them can be used (sorry, fence-sitter!). I will always try to use the least amount of code that is "reasonably" required. Above all, to me it's performance that matters and good clean code goes hand-in-hand with that!
More work but more reusable
2007-07-16 10:17:50 marcossofting
[Reply | View]
I would stick with d. Seems to be more work but it's more usable along the time.
I vote for "c"
2007-07-16 00:36:15 imichtch
[Reply | View]
"a" is acceptable if non of other are allowed
"b" would be a typical error when implementation affects interface. What should users think about this artificial class? Would this class make sense for them?
"c" is a "standard" way of maintaining usefull (library) code and avoid duplication.
From my point of view "d" is a variation of "c".
toString should be a debugging method
2007-07-15 21:09:14 diegof79
[Reply | View]
To me toString means "the default view as string".
The problem is that the view depends on the context, for example Fruit view is not the same view if you are printing an instance of Fruit in a CSV file or in a label in the UI.
So I think that toString should be used as a debug method, just to print debug logs. (like debugString in Smalltalk)
About the options:
a. Is not that bad, depends on the logic inside toString... ohh wait in Java null is not an object so you have that horrible ifs :(
b. It's the worst!! A really bad use of class hierarchies. Don't do that! (I have to write a lot to explain the reason... just google for "Design Reusable Clases")
c. Why ToStringUtils should be static? Why not a new DebugStringFormatter(this).toString()
If there is a "performance" reason you could change the code later to: create the DebugStringFormatter only one time or... use the static version.
d. Do you mean something like:
toString() { ToStringManager.formatterFor(this).toString(); }
In that case I prefer to use the formatter from the "outside":
anInstanceOfToStringManager.formatterFor(aFruit)
I believe the question that answers this question is: does a change in one class necessarily imply a change in another class? Sure you, have two methods that are functionally equivalent, but do the classes or contracts require that they are equivalent?
Example. I typically use enum and enhance it with utility methods (usually allowing synonyms). If I were allowed, I would create one such abstract enum and extend from it. Now, I copy it to each new enum. Changing functionality of one of them should imply changing in all of them. Subclassing, outsourcing, anything could be used here to assure orthogonality.
But if changing the toString() method of one class would not necessarily imply changing that of the other, then I'd stick with keeping them seperate. You don't want to introduce artificial dependencies because they both happen to quack like a duck.
I would go with B
2007-07-13 03:25:56 phuego
[Reply | View]
Indeed, in the system I work on presently I have gone with B, and its working out fine.
We've introduced an AbstractDTO class that all of our DTOs, etc descend from. It carries a reflection based toString() method, also common error handling etc.
So I know its not ideal from a theory of inheritance point of view; nor is it purist OO, but its pragmatic and simple and done now.
I know my DTOs all have a useful toString(), the code is only in one place, the junior developers don't need to be nagged, my log output is standardised, and maintenance/testing is easiest.
For my two cents, I value pragmatic, DRY, clean, in that order. All are noble goals, but my budget wants it done soonest and cheapest.
I would go with B
2007-07-15 21:28:34 diegof79
[Reply | View]
Well if it worked for you. :)
I had worked in a big Smalltalk system that used something like the "AbstractDTO" that you describe.
The problem with that kind of hierarchies is that if you don't split the responsibilities between your domain model and your "DTO" classes, the systems comes difficult to test (you need a mock db or something like that), and maintain (we have a big headaches trying to fix some cache and transaction management done in that DTOs).
In a new version to replace that legacy system, we choose a different approach where the separation between domain model an "database interaction" was clear and all the debugging, testing and design was much more easier :) (that's why I hate that hierarchies just for code reuse... is not OO purism, is that my experience with that kind of hierarchies was painful)
I would go with B
2007-07-14 20:39:43 bruceg111
[Reply | View]
The issue here is that toString() would need to be in java.lang.Object for this to really work as what about built in types that you may want control over. I use toString() for logging output and wish to have strict control. Having a separate class outside the inheritance structure allows this and best of all I can use the base class to add functionality but also have functionality in the static class which can then choose to use the classes toString() or its own logic (I prefer a type based registry to make such decisions so that the logic is not hard coded but can be altered).
You might be missing the point ...
2007-07-12 20:27:12 zmang12
[Reply | View]
I'd put the code in a utility class (not necessarily static methods), then have Fruit's and Vehicle's toString() methods delegate to the utility class, passing "this" as an argument.
The issue with this approach is that all over your code you have something like "println(objectX.toString())". This causes issue as I now need to differentiate between standard java classes and my own. Since I cannot change standard java (ok I can but choose not to) it means more thinking on all print statements. Better to just say "println(ObjectToString.stringify(objectX))" and let the ObjectToString logic handle the different types. I prefer a registry for type handling here so you can have whatever logic you want and it handles java standard types as well as custom types.
Actually no, all over your code you have println(objectX) if your println method takes an Object, or out.println(objectX) if you have a PrintWriter in the field/variable called "out"
Besides, the questioner was asking how to structure the code so that both fruit and vehicle used the same code for their toString() method, your suggestion says to put in in the caller code. That was not an option and is not what was asked.
First of all println(objectX) is equal to println(objectX.toString()) so I am not sure what the issue was with this. As to the response I am focused on the use of toString() which for me is primarily for logging purposes. As such I want standardized behavior for both built-in java classes and my own classes. As such my solution must deal with both cases. While I realize this is a slight expansion of the question, I thought it made sense as I am actually showing a real world use of such code which seems useful to me. As such I wanted to show how you could unify both java built-in code with your own code.
I am not sure what your disagreement is as we seem to agree with option c) but I just take it slightly farther and show how to a) try to unify the bahavior of built-in classes and b) what the calling would look like. Note that I typically have the toString() methods call a string builder as well but I was trying to focus more on the calling side.
Use GenerateToString. It's an IntelliJ plugin. I know, this is just an example, but maybe it would be better to use a more realistic example, and then there'll be a more realistic discussion.
Seems like annotations might be the answer to this one.
dependencies
2007-07-12 08:37:37 dog
[Reply | View]
Note that by sharing code between 2 classes in different packages you create a common dependency between them. This may or may not be desirable.
In this particular case I would lean to be clean and not DRY.. because toString() tends to be used more for debugging than for anything else. So better just to be redundant than to create dependencies between packages.
There's something to be said about packages that have fewer dependencies.
even better
2007-07-12 08:27:35 anonymousfu
[Reply | View]
Even better, if it's sufficiently generic there's probably already a public library that has what you need. In this case, it's Jakarta Commons Lang ToStringBuilder.
even better
2007-07-12 19:38:35 brucechapman
[Reply | View]
What exactly did you think the question poser meant when they said "Let us assume no third-party systems already exist for this (such as Commons Lang)." ?
static classes work for me
2007-07-12 07:53:07 bruceg111
[Reply | View]
While all approaches could work I almost always uses c), i.e. create a class with a static method such as "buildString" etc. In order to make this somewhat flexible I typically allow registration of the algorithms to perform the toString by type, i.e.
ToStringMaker.register(MyType.class, new MyTypeToStringBuilder());
Such builds implement an interface such as ToStringBuilder. I typically would provide a "standard" build such as DefaultBuilder which in this case just uses reflection. This allows a) the default algorithm b) class specific algorithms which can be registered in either the static class for typical classes in the project or via static sections in classes and c) other less standard implementations (i.e. perhaps one that shows dates as their numeric value in miiliseconds rather than a text version).
Regards,
Bruce
Simple is better
2007-07-12 06:24:14 allaires
[Reply | View]
I'm not sure a big complicated toString() using reflection is a good idea. I think simpler toString() methods in each class or base class would be easier to understand and maintain in the long term.
Simple is better
2007-07-12 16:22:22 sumitkishore
[Reply | View]
Not if, as is often the case, toStrings are used basically to represent simple data objects. I'll argue that except in some unusual cases, this is where Object toStrings are really needed.
Simple is better
2007-07-12 09:07:31 chris_e_brown
[Reply | View]
Reflection might also expose private state in an inappropriate way, or call "get" methods that aren't really representing properties.
As a general solution to this problem, decorator or composite objects are often helpful. The decorator can called from within the class or from outside, accepting obviously more than one type of object, and return the expected output for multiple (possibly unrelated) object types, and composition involves making a new class that embeds two others to get common behaviour without the ambiguous/undesirable side effects that multiple inheritance would lead to.
- Chris
simple is so far not useful
2007-07-14 20:37:08 bruceg111
[Reply | View]
I find your response very confusing. Why would I use a decorator inside the class and if it was outside then it relies on the public methods just like reflection. And how does a composite pattern help at all as it just adds another layer and the point here was to figure out how to handle toString() for a type. You seem to be adding layers to the process and think they solve the problem. I do not mean to sound harsh but unless you have some examples I find your post to only confuse the topic and add little value. The original discussion was very lucid and you have totally thrown away the examples given and have not provided your own to argue with.
could closures bridge this gap?
2007-07-12 05:35:12 adamadas
[Reply | View]
I am not sure of how well closueres could be the solution which would satisfy both styles
could closures bridge this gap?
2007-07-12 09:26:49 dserodio
[Reply | View]
Are closures the new golden hammer?
could closures bridge this gap?
2007-07-12 22:17:10 jwenting
[Reply | View]
yes. And like golden hammers they're pretty much useless. Too soft to use as a hammer, wrong shape to use as a screwdriver, and unless you like hammers they don't even make good wall ornaments.