I’ve started refactoring the actual SnmpCollector code in an attempt to simplify a few things as a prepare to reorganize. I figured I’d start at the beginning. WHOA! Have you seen the initialize method in there. Check it out in 1.2! This one methods was hundreds of lines longs generated and/or manipulated at least 3 Maps, piles of Lists, and on top of that dove into the database for a number of different things. In one case it even made database calls inside a loop processing a result set! Needless to say when I look at it I was completely overwhelmed!
Fortunately for me I had just been discussing refactoring with Dave and Mike Huot. I had been going through the examples from the refactoring book and had been showing Dave some of the work I had done on Collectd. I had noticed a comment that said [paraphrased], “If you run across a very complex method with large number of local variables, turn it into a method object and then refactor.” I had been a little skeptical with respect to how this would really help but when I came across this method I decided I would give it a try.
I created an object called Initializer. The purpose of this object was just to have one public method called execute that did all the same things as the ‘initialize method did. I created an instance of the object and moved the method into the Initializer class using Eclipse. Next I went through the object and, again using Eclipse, I made all the locals into fields. This at first doesn’t really do anything to help the complexity of the code but it makes the next steps possible which make things MUCH easier.
Once all of the locals are fields, Then it is a simple matter to go through the code and extract methods that perform various useful functions. By simple doing this I was able to turn the initialize into the following:
initialize();
/*
* All database calls wrapped in try/finally block so we make certain
* that the connection will be closed when we are finished.
*/
allocateDBConn();
try {
getPrimarySnmpInfo();
getSysObjId();
createNodeInfo();
determineSnmpVersion();
getSnmpInfoForInterfaces();
verifyCollectionIsNecessary();
} finally {
closeDBConn();
}
logCompletion();
The code in these methods is still quite ugly but now I have a number of methods with a much more easily defined purpose. I am also able to do the same thing to those method as I need to. The ultimate goal of course is to turn most of this function into operations on the appropriate objects as this makes things MUCH simpler. I’m not finished with this object yet but I suspect by the time I finish this object will ‘appear’ to do nothing more than validate the parameters to the initialize method.
This refactoring thing just keeps getting better and better. But knowing refactoring and actually doing refactoring sure are very different and real mastery comes with practice. I sure wish I had this rectoring skilll when I worked on the poller!
Well, happy refactoring!
Matt