I’ve decided to start a new category: The Code Pranger. Maybe it’s a bit redundant, because most of it will be some form of rambling, and The Daily WTF probably does a much better and much more public job at this, but this pranger will be under my control.
To clarify, a pranger is a medieval device for torture and public humiliation, similar to stocks. Someone who had done something shameful was locked up in a pranger, usually by restraining the neck and both hands between two pieces of metal or wood, and exposed to the public. I am not going to expose the writers of bad code here (even though sometimes I might want to; let this be a reminder to myself: don’t!), I’ll just put pieces of code, which I find ugly, on the pranger.
The purpose, of course, is to provide anti-patterns, perhaps create some discussion, and increase the overall quality of code being written. I realize that I will probably won’t achieve the last of those goals. I also realize that I probably won’t even fix the examples of bad code, because I believe in the maxim of “don’t fix what’s not broken”, where broken and ugly aren’t the same thing.
Anyway, here we go. I absolutely hate this piece of code summarized below:
public class ProjectPropertiesFrame extends JFrame {
// ...
private DirectorySelectorComponent _workDirSelector;
// ...
public JPanel _workDirectoryPanel() {
_workDirSelector = new DirectorySelectorComponent(/* ... */);
// ...
return _workDirSelector;
}
private void _setupPanel(JPanel panel) {
// ...
JPanel wdPanel = _workDirectoryPanel();
gridbag.setConstraints(wdPanel, c);
panel.add(wdPanel);
// ...
}
}
The method \_workDirectoryPanel() mutates the _workDirSelector field and returns the new value of the field at the same time. At the call site, the return value is used for further processing.
Why mutate and return? Either mutate the field, let the method be void, and use the field. It’s been mutated, so it’s ready to go. Or don’t touch the field, don’t mutate, return the new value to the caller, and let the caller decide what to do: Mutating the field, for example. If you really want to do both, at least return the old value of the field before it was mutated. That makes at least sense in some situations.
But mutating and returning the value of the field? That’s just a function with an ugly, perverse side effect. I really don’t see the use at all. Is it convenience? No, it can’t be, it would be more convenient just to mutate and not create the local variable wdPanel. Why would anyone write this?
I think mutation should be avoided whenever possible. Of course, in most production programming languages, that’s not possible. So make a void function and be explicit about the mutation you’re doing. There’s a programming style that really dislikes void methods; I sometimes subscribe to it. If a method is void, you can’t chain method calls together. If it returns something, you can. So if, for example, you have a List class and the add(T element) method mutates the internal state of the list, then you could let the method return this:
public class List
// ...
public List
// ...
return this;
}
}
That way, you can quite succinctly write something like myList.add(1).add(2).add(3);. But that’s a completely different situation.
Can anyone find an explanation why a method should mutate a field and return the new value of the field? I can’t.
