Wednesday, February 3, 2010

A bad name will kill

Javadoc is quite helpful if you have to deal with a new API. It explains what methods are doing, and the role each class is playing. But let's be honest: How many times have you ever read the javadoc upfront? After studying some basic examples I usually choose classes and methods by name: Hey, this sounds like the functionality I need. Then I check the javadoc to make sure that it actually is what I want. But this requires that names are well chosen...

Bad names do not communicate a method's purpose or, even worse, they communicate the wrong intention. So what are the reasons for poor naming? Well, one reason is for sure careless naming. The one who designs an API knows exactly what it does, so, with a certain lack of sense, he won't even recognize that it's badly named. Another reason is refactoring or redesign that changed the intention of a method. Means: the name was wisely choosen in the first place, but after changing the methods semantics, the name has not been adapted in order to reflect its new meaning :-|

You will find poorly named methods all over the place, so let's take a look at a prominent example: Thread.interrupted(). So what does this method do? It queries the thread's interrupted state, doesn't it? No, that's what method Thread.isInterrupted() does. Yeah no, is clear... so what the heck is interrupted() doing? Have a look at the javadoc:
public static boolean interrupted()
Tests whether the current thread has been interrupted. The interrupted status of the thread is cleared by this method. In other words, if this method were to be called twice in succession, the second call would return false (unless the current thread were interrupted again, after the first call had cleared its interrupted status and before the second call had examined it).
A thread interruption ignored because a thread was not alive at the time of the interrupt will be reflected by this method returning false.
So basically it resets the threads interrupted state, and returns the value it had before reset. This kind of indivisible operation is used in operating systems (e.g. to implement mutual exclusion) and is usually called testAndSet(). Java's AtomicX classes also have such a method called getAndSet().

Our interrupted() method is a derivate which does not set but clear the state. This operation is usually called testAndClear(). Now that we are aware of what the method really does, we can give it a better name. Intuitionally one would call it testAndClearInterrupted(), but in analogy to AtomicX.getAndSet() we better call it

  public static boolean getAndClearInterrupted()

Now the method communicates what it does. No misunderstanding, no need to look at the javadoc.

So here is my recipe on naming methods: If a method's name is not crystal clear...
  1. give the method any (foolish) name, e.g. foo or yetToName
  2. write javadoc for the method and describe what it does
  3. read your own doc and find a name (if it's still not clear, go back to 2.)
  4. rename your method
I can hear someone whine: My API is already widely used, changing the name will break existing code. That's a lame excuse. Keep your old bad named method, make it use your new one and mark it deprecated, e.g.

* @deprecated use {@link #getAndClearInterrupted() getAndClearInterrupted()} instead
public static boolean interrupted() {
return getAndClearInterrupted();

Didn't hurt that much, did it? So, there is no reason to live with bad named methods.

Best regards

A bad wound may heal, but a bad name will kill.
-- Scottish Proverb