S
Steve Howell
In my rewritten code, here is the smell:dispatches = {
'dict': _dict,
'list': _list,
'attr': _attr,
'key': _key,
'as': _as,
'call': _call,
'recurse': _recurse,
}
if kind in dispatches:
return dispatches[kind](ast)
else:
raise Exception('unknown kind!')There is nothing wrong with the code taken out of context, but one of
the first things that lexical duplication should alert you to is the
fact that you are creating code that is brittle to extension.
Really? It is very simple to extend it by adding another key:item to the
dict.
Easy for me to extend, of course, since I wrote the code. It's the
maintainers I am worried about. I can make their lives easier, of
course, by changing the text of the exception to say something like
this:
"Silly maintenance programmer, you now have to update the dispatch
table!"
I have no problem with that.
It gets worse -- the reference to the in operator is in a completely
different module to the implementation of the in operator, which may be
in a completely different language! (C in this case.)
I'm being sarcastic, of course, but I do have a serious point. I'm not
impressed by complaints that the definition of the function is oh-so-very-
far-away from where you use it. Well duh, that's one of the reasons we
have functions, so they can be used anywhere, not just where they're
defined.
The slippery slope here is that every block of code that you ever
write should be extracted out into a method. Now I am all in favor of
tiny methods, but the whole point of if/elif/elif/elif/elif/end code
is that you are actually calling out to the maintenance programmer
that this code is only applicable in certain conditions. That's why
"if" statements are called "conditionals"! You are actually telling
the maintenance programmer that is only sensible to use these
statements under these conditions. It is actually extremely explicit.
Now I'm being sarcastic too, but I also have a serious point. Nine
times out of ten if/elif/elif/end calls out the smell of spaghetti
code. But it can also occasionally be a false negative. It can be a
case of avoiding premature abstraction and indirection.