Thursday, 19 February 2009

Avoid not using positive method names

I'm integrating with Spring Security at the moment. It seems to be a well-designed framework, but I got myself confused this morning trying to implement some methods of the UserDetails interface:

boolean isAccountNonExpired()
    Indicates whether the user's account has expired.
boolean isAccountNonLocked()
    Indicates whether the user is locked or unlocked.
boolean isCredentialsNonExpired()
    Indicates whether the user's credentials (password) has expired.
boolean isEnabled()
    Indicates whether the user is enabled or disabled.

I imagine the designers wanted all the answers to be true for the normal "happy" situation. But my brain couldn't cope with all the negatives, so I ended up writing a second set of positively-named methods to call from the negative ones. :-)

public boolean isAccountNonExpired() {
return !isAccountExpired();
}
... etc.

5 comments:

Andy said...

In the words of Bill and Ted, this is a most non-non-non-non-non-heinous article :-)

I have issues with negative method names too :-)

David Peterson said...

Thanks (I think).

.MOz said...

Also the codebase we "inherited" has several instances of non positive (negative?) method names; the definition of a "negative" name itself looks like a smell (smells like a smell?).
Even weirder, we have both positive and negative method names declared as public, so they are both shattered through the codebase, and it is not clear whether sometimes one method is preferred over the other; we started refactoring everything to eventually delete all references to negative names.
Negative names only clutter code: that's bad, because we want our code to clearly communicate its intents, and this negation is a small but disturbing impedance between brain and code.
"Keep your code clean" is always a good advice :-)

Duncan Pierce said...

This really confuses me too. It gets awfully tangled when client code starts calling negative-sense method names with a ! operator and then doing most of the work in the 'else' branch of an 'if'.

Rather than inverting the sense of these methods (which, like David, I prefer) you can sometimes make things more comprehensible by dropping the slavish devotion to the 'is' prefix and using ordinary English words like 'unlocked' over constructions like 'nonLocked'.

If the docs for 'isAccountNonExpired' say "true if the user's account is valid..." why not call the method 'valid'? Or 'isValid'?

If you want to preserve the distinction between credentials vs accounts expiring (and I would be interested to hear why you'd want to), you could make it 'accountIsValid' or 'accountIsUnexpired'.

BTW 'isCredentialsNonExpired' is a weird - and probably incorrect - use of grammar. It forces even more cognitive load on the reader.

David Peterson said...

Yeah, or maybe use a "tell, don't ask" approach and, instead of asking UserDetails all those little "getter" questions, tell it to assertIsValid().