The best of the worst: Standard patterns of horrible code from Java and .Net

There are some standard worst practices in code which make my teeth ache when I see them. They can appear at any time in a code base and look very innocent but will turn everything around them into spaghetti in short order. Someone somewhere near you is typing out one of these insidious bits of code as you read this. You have been warned.
I've already written about the one where people create namespaces and constants to handle types. Something like

public interface CarType{
public static final String AUDI = "audi";
public static final String MARUTI = "maruti";
The constants are usually ints, or even worse, Strings. The latter start popping up everywhere, including (in one bad case) eventually showing up as a piece of text on a UI. But enough of that, let's move on to today's rant.

Today, recruitment had bribed some of us developers into a code review session (our interview process requires candidates to submit code) over lunch by ordering from Subway. Some of the code I reviewed brought back two of the most annoying, yet widespread development worst practices. Here they are, one from Java and the other from .Net

.Net: Everything is a DataSet
Aargh. Why of why is there this obsession with mapping everything to the rows and columns of a DataSet? Everything is not a DataSet. They're People and Employees and Magnolias. I've seen code where there wasn't a single developer created class! And this from people with several years of experience, not freshers. The flow inevitably looks something like Create DataSet -> Read from xml file/webservice -> Optionally pipe it around a bit using a few web services -> Pump into DataSet -> Perform operations, creating and manipulating DataTables, DataColumns and DataRows by the dozen with no classes and tons of procedural logic -> Bind these to UI objects. If you're really unlucky, that 'tons of procedural code' I mentioned will be in a class which inherits from Form. A more thorough, consistent and violent violation of OO principles I am yet to come across.

I would suggest that XStream .Net and NHibernate be used instead to achieve the convenience that DataSets offer in the short term, but over a longer period and while maintaining code quality.

Java: Every field must have a getter and a setter
That strange and wonderful idea, the bean, has made a whole generation of Java developers completely inured to the idea of maintaining encapsulation. The bean was created to solve a specific category of problem, but the structure of the bean has spread like a virus, destroying the integrity of the most innocent of domain objects. When doctors wish to test the reflexes of such developers, the preferred method is no longer a mallet applied smartly just below the kneecap. No, you need just give them a class in an editor, ask them to add a field and check if they automatically add a getter and a setter for it.
The only time using a setter is acceptable is
  • if the state of the class cannot be corrupted by using it (calling setText on a TextBox can change its state, but does not corrupt it)
  • if the framework demands it
Getters aren't as bad as setters, but over time on a large code base they encourage the processing of data outside of the class to which it belongs. From there, it's a short and slippery slope to code duplication and other evils. In combination, getters and setters can reduce once healthy domain objects to pale DTOs.

I would suggest simply avoiding setters unless a change of state for that field does not corrupt the object. A lot of frameworks now understand this and help developers do away with setters. Hibernate, for example, allows you to configure field level access (Hibernate will populate your domain object fields directly using reflection, even if they are private), thus removing the need for setters.
For getters, use them only when the data read using the getter will never be processed - in other words, only in situations like binding to a UI or persisting to a database.


Kunal said...

public static final int MARUTI = "maruti";

I don't think you can assign a *string* to an *int*!

Unknown said...

Darn. I always miss something... thanks, it's fixed now.

kodeninja said...

I think part of the problem can be addressed if you can apply the Immutable pattern to the class being designed. Think and think hard if the class you are designing really needs to allow modifications.

And, thread-safety is a nice added bonus :)

It's surprising how many cases don't actually require mutability!!!

Anonymous said...

Yes, the getters and setters have taken over the world. I'm beginning to realize why they say Getter and Setters Are Evil.

Tony Morris said...

It's surprising how many cases don't actually require mutability!!!

No class needs "mutability". Use a real language such as a pure functional one, then think again.

Anonymous said...

Damn it! I have spent several interviews trying to elicit from developers why getters and setters are bad. Often I crack and just end up ranting at them. What I *really* want to convey is how they break encapsulation - but I find the concept of encapsulation so hard to set out that I usually wimp out and just end up with an enumeration of the benefits of immutable classes, which are immediately clear.

Anonymous said...