This post originated from an RSS feed registered with Agile Buzz
by Keith Ray.
Original Post: Design, Test-Driven-Design, and Code Smells
Feed Title: MemoRanda
Feed URL: http://homepage.mac.com/1/homepage404ErrorPage.html
Feed Description: Keith Ray's notes to be remembered on agile software development, project management, oo programming, and other topics.
In retrospective, a good design looks simple and obvious, but getting there isn't always straight-forward. Once you've done Test-Driven-Development/Design for a while, "testable" becomes part of your definition of "good design", so that designs you once thought were good become less so when you consider their lack of testability. TDD helps you in getting to good design, but only if you keep you mind open to pains or code-smells that come up while you go through the repeated sequences of test-code-refactor. To quote an excellent (but mostly unrelated) essay by Paul Graham:
The way to get a big idea to appear in your head is not to hunt for big ideas, but to put in a lot of time on work that interests you, and in the process keep your mind open enough that a big idea can take roost. Einstein, Ford, and Beckenbauer all used this recipe. They all knew their work like a piano player knows the keys. So when something seemed amiss to them, they had the confidence to notice it.
One of the worst things you can do when doing Test-Driven-Development is to stubbornly bull your through to implementing the design you conceived before you started, ignoring code-duplication and any other code-smells that come up. TDD forces you to create testable designs, but only encourages you to come up with good designs. Pair-programming (particularly with someone more experienced with TDD) will detect code smells when they come up, and help suggest refactorings to do to clean them up.
By the way, if you have not heard the term before, a code-smell is a hint that the design of the code may need improvement. Sometimes the design can't or shouldn't be changed, but more often addressing the problem detected by a code smell improves the design.
To try to demonstrate some code smells and possible design improvements in a piece of real software, I've picked a class at random from the Firefox/Mozilla sources. These smells I mention here do not necessarily mean that the code should be changed, but point out places to look at for possible improvements.
nsTransactionItem.h (follow the link to see the source code). Starting with the classes being referenced. Well, I can't find the nsITransaction class at all. Looking at other source, it appears that it should be declared in the header file "nsITransaction.h", but that file does not appear in the code I downloaded. %ltgrumble%gtHow many tar files do I have to download to get the complete source?!s%lt/grumble%gt. (It seems this is supposed to be code-generated from nsITransaction? I didn't try building the app.
nsTransactionStack, nsTransactionRedoStack, and nsTransactionManager are concrete classes, violating the Dependency Inversion Principle (more of a guideline, really) that says "Depend upon Abstractions. Do not depend upon concretions." Depending on concrete classes generally tends make the code fragile, and hard to substitute objects (one of the powerful features of OO programming. For example, in testing nsTransactionItem's UndoTransaction method, you might want to pass in a "fake" nsTransactionManager object that returns NS_FAILED from its WillUndoNotify method. Now none of the classes I've look at so far had any public non-virtual methods, so subclassing nsTransactionManager to have the desired behavior is possible. I wouldn't go and change these to "interface/abstract" and "concrete" pairs of classes just to satisfy the DIP principle, but if I found a need for more than one concrete subclass, I might do it.
There are six private methods in the nsTransactionItem class. Private methods are hard to test directly, and sometimes indicate that a class has too many responsibilities that could or should be off-loaded to other classes (possibly classes that don't exist yet.)
Delving in nsTransactionItem.cpp and looking at implementation of these methods, I'm on the alert for Feature-Envy, Verb-Subject and Long Method smells. UndoChildren and RedoChildren are the longest methods and have the verb-nouns names, so let's pay special attention to them. Leaving out the error-handling in UndoChildren, and translating to psueodo-code, this method is doing something like:
method nsTransactionItem::UndoChildren(aTxMgr)
index = mUndoStack->GetSize()
while index-- > 0 do
item = mUndoStack->Peek()
transaction = item->GetTransaction
aTxMgr->WillUndoNotify(transaction)
if item->UndoTransaction(aTxMgr)
mUndoStack->Pop(&item)
mRedoStack->Push(item)
endif
aTxMgr->DidUndoNotify(transaction)
end while
end method
In the above pseudo-code, I renamed the original code's one-letter variable "t" to "transaction" to make more sense. (Write for the reader, since many more will be reading the code than writing it. Use good names.) It MIGHT be possible to push most of this method into UndoStack (move-method). There also could be a violation of Law of Demeter as well. Let's see what that might look like:
method nsTransactionItem::UndoChildren(aTxMgr)
mUndoStack->UndoAll(aTxMgr,mRedoStack)
end method
...
method nsTransactionStack::UndoAll(aTxMgr,aRedoStack)
index = this->GetSize()
while index-- > 0 do
item = this->Peek()
transaction = item->GetTransaction
aTxMgr->WillUndoNotify(transaction)
if item->UndoTransaction(aTxMgr)
this->Pop(&item)
aRedoStack->Push(item)
endif
aTxMgr->DidUndoNotify(transaction)
end while
end method
That refactored code could be considered slightly better because the new method is referring to its own methods and data more than the original method did. If this eliminated duplication from elsewhere in the project, I would certainly do this. Otherwise, maybe not.
The pair of WillUndoNotify and DidUndoNotify method is an opportunity to use the C++ idiom Resource Acquisition Is Initialization. (the C2 wiki has a few other pages with similar names, and none of them is exactly conveying the idiom I have in mind.
method nsTransactionStack::UndoAll(aTxMgr,aRedoStack)
index = this->GetSize()
while index-- > 0 do
item = this->Peek()
transaction = item->GetTransaction
UndoNotifier notifier(aTxMgr,transaction)
// notifier's constructor calls aTxMgr->WillUndoNotify(transaction)
if item->UndoTransaction(aTxMgr)
this->Pop(&item)
aRedoStack->Push(item)
endif
// notifier's destructor calls aTxMgr->DidUndoNotify(transaction)
end while
end method
Generally, there's not at lot to do that will pragmatically improve this particular class, even keeping a sharp lookout for code smells, but there is one elephant in the living room: the error-handling that clutters up every method. The Mozilla project decided not to allow exceptions apparently because some (really antique?) platforms that they wanted support didn't support them. Also, I suppose they were motivated by the (valid) ideas that exception-safe C++ code is harder to write, and generally requires "near-expert" C++ programmers. There would have to be standards so that the catch-clauses don't get out of hand. Let's look at the costs of that decision by just examining a simple "getter":
Because the return value is dedicated to error-handling, this function has to use the awkward idiom of a pointer-to-a-pointer parameter, and has to check that the pointer-to-a-pointer is not null, so a one-line method is now four lines long. It has to return an error code even though its own behavior (return an object) could not fail if it had been coded in the more normal idiom. (And where's the "const"? Most getters are const.) Other methods are even longer and tend to be unclear because of the interlacing of error-checking and algorithm.
MOST of the places that call this getter check the result and return it if it's an error, adding two lines of code to those callers (and multiple returns to methods, which can be confusing for long methods). However, several callers do NOT check the result, which is inconsistent and possibly fatal if there was a real problem being conveyed in the result.
Another thought. This class Releases a transaction object in its destructor, and deletes two others. Using smart pointer template classes (something like boost::shared_ptr) would automatically handle these deletions so there is just a little less code to write or forget to write. The implementations of Release that I browsed were ref-counting, which adds a responsibility to those classes that a smart-pointer template class would keep separate.
Previous Topic
