In an Extreme Programming environment, continuous, merciless refactoring is a way of life. Though the concept of refactoring isn't woven so tightly and explicitly into the workflow of most software developers, knowing when to stop what you're about to do and take an extra hour to rewrite a few methods or a few classes instead of adding yet another condition to that logic block could be the difference between 6 hours of head scratching and a simple in-and-out update when you see this code again 6 months from now.
If you're already working with a well written project it's frequently completely appropriate to add a few lines of code to a method or two, to pass in a new parameter, or run out and grab some additional data, and be done with it. Sometimes a bug is really just a bug: a loop is off by one, a boolean expression is incorrectly nested, some little bit of business logic was overlooked, whatever.
Sometimes though, a bug (or the process of fixing the bug) is an indication of a bigger problem. Here are 5½ signs that the patch you're about to make will cause you bigger headaches down the road if you haven't already considered and dismissed refactoring:
1: A new component doesn't seem to fit.
When you're holding a square peg and looking at a round hole, you should be asking yourself why the hole is round, instead of looking for a sledgehammer. Forcing a component into an archicture where it doesn't belong is going to cause problems now, while you try to trick the current architecture into supporting the new functionality, and it's not going to make it any easier in the future when you revisit the new component or add more new components.
Instead, think about why the new component doesn't fit and refactor your architecture appropriately. If you're having trouble hooking something into an application's initial loading sequence, consider how the loading sequence could be managed or initiated better. If you have to pass messages through four different objects to get a particular component access to some data, work out how you can simplify access with an interface in one of those classes.
Perhaps most importantly, though, if you discover that the hole is round for a reason, ask yourself why your peg is square, and make sure there aren't already some square holes somewhere else.
2: You're asking "Now what does that variable/method/class do again?"
Frequently jumping around code trying to remember what each piece does indicates that it could use some cleanup. Variable, method and class names should clearly indicate their function. A useful corollary to this rule is that in order to have good names, they must have clearly defined functions.
If the names are poor now, but haven't always been poor (i.e. they're poor "for historical reasons"), chances are that adding new, perhaps unexpected, features have caused some units of code to take on additional responsibilities that are best left to a separate unit. If these responsibilities aren't left to the unit best suited to handle them, you're less likely to remember the location of, and reuse that code later.
Note: Using comments as a reminder of the function of a piece of code doesn't count. Comments shouldn't explain what something does. They should explain why.
3: It's fixed, but you don't know why.
You twiddle an AND to an OR in a condition somewhere et voilà! it works. Congratulations, you're one step closer to having a clear bug list. However, you're one step closer to a nervous breakdown next time a build gets a thorough testing. Why? Because you just fixed one use case, but broke 4 others. Sure, if you have unit tests you might find out before the QA team does, but chances are you don't have unit tests, and if you do their coverage isn't so great.
What to do, then? Easy: figure out why it seems to work. Make a flow chart or logic table if you have to, but don't just use it to determine which complicated logic statement to tack on to the end of each of the statements in your giant if-block and move on. Instead, use it to break down the process into a number of more well-defined methods (see #2), to reorder the tests to make what you're really looking to weed out at each level more clear, or to move decisions that shouldn't be made here into another object (hint: see if you're performing the same tests in a lot of different places in the application).
If all that logic really is that necessary, all of those factors really apply to the decision to be made at that precise moment in the code, and none of the factors could (or should) be calculated somewhere else, at least simplify the logic statements and save yourself this trouble next time you come here. If you haven't visited your friend De Morgan in a while, now might be a good opportunity to catch up on old times.
4: You're about to change a private method or variable to public.
If you made a member private when you first wrote the class, chances are pretty good that it was a good choice. At best, changing its visiblity to public will introduce inconsistent usage of the class. If you're not so lucky you could end up with inconsistent data in the class, or an even bigger refactoring job down the line when you change the implementation of that variable or method which was previously safely hidden from view.
Think about why that object isn't allowing other objects the access you're about to give them; think about why you "need" access to that variable or method directly; and think about what that kind of direct access could mean for future changes. If you still think it's a good idea, then go right ahead and change it, just know what you're doing and why before you do it.
5: You're about to Paste a chunk of code.
You've already done the important work and recognized that the functionality you're trying to add depends, in part, on some piece of logic that already exists in the system. You've already done the hard work and located that piece of logic. Now you're about to destroy all of the good you've done by creating two separate places to monitor and maintain that logic.
True, some pieces of logic are deceptively similiar, but really do different things and belong in separate classes in distant and disparate parts of the system. I expect that by the time you've hit CTRL+C, this is the exception, though, and not the rule, so think long and hard about where your cursor is before you press CTRL+V, and you'll be glad you did.
5½: You're working again on a consistently buggy bit of code.
Frequent problems is a sign that your car needs a tune up; frequent problems is also a sign that your code needs a tune up. This only gets half a mark because if it seems like you're looking at a particular method or component every time QA takes a look at a new build, there's a good chance that you've been ignoring the other signs in this list long enough. It's time to bite the bullet.
Don't go easy on it at this point. Remember: this section of code has caused you hours of headaches in the past, and this is your chance to make sure that it will never get that opportunity again. The whole of the buggy piece of code, the object its in, and the architecture around it should be in question. Be ruthless. You'll be glad you did.
Submitted by Anonymous (not verified) on April 19, 2007 - 5:10am.
"Comments shouldn't explain what something does. They should explain why."
Whenever I see this line quoted from a well-known textbook I wonder how experienced the quoter really is.
Comments should convey the intent of the code, something that is not obvious by reading the code alone, and they should speed up the understanding of the next reader (which could be you). You don't want to know why the code does what it does ("... because of change request X", "... because we want to know the distance between p1 and p2", "... because the customer may be out of credit"); indeed, these examples and all comments are silly, if answered from a "why" perspective. It is far better to document the meta-information on the human language level to allow the reader to understand the connection between requirements and code. He'll be able to figure out the why himself.
Examples:
// calculate distance of the two points in 2Dd = sqrt((p2.x - p1.x) * (p2.x - p1.x) + (p2.y - p1.y) * (p2.y - p1.y))
// check whether customer still has credit
if (customer.getBalance() <= bag.getTotal()) ...
These comments say what is being done, not why.
Otherwise, a good article.
Submitted by Brandon on April 24, 2007 - 1:21pm.
Thanks for reading and commenting, Anonymous.
While I agree that comments should speed up understanding for the next reader, I disagree that they should do that by conveying the intent of the code. I maintain that the code should convey the intent of the code.
At this point comments conveying the intent of the code become redundant and useless. To use your example:
Obviously you don't need comments for "according to item 3 on page 4 of the version 10 of the spec ..." and anything changed "because of change request y" will be commited to source control with a comment that says "Completed Change Request Y" but how which of these two things would you rather see?
// customers that still have credit get foo'd if (customer.getBalance() < = bag.getTotal()) {foo(customer)} // customers with credit need to get foo'd before bar happens so baz updates properly if (customer.getBalance() < = bag.getTotal()) {foo(customer)}Now when I come back three months from now and try to refactor and foo customers with credit in a different method, I know to make sure that baz updates properly.
Submitted by Justin Bozonier (not verified) on June 20, 2007 - 1:35pm.
@Anonymous:
I agree with the author over you. The *what* can be defined by doing this:
d = DistanceBetweenPoints(point1, point2);
where point1/2 are point objects
or even
d = DistanceBetweenPoints(x1, y1, x2, y2);
and then define your method to implement this.
Instead of:
// calculate distance of the two points in 2D
d = sqrt((p2.x - p1.x) * (p2.x - p1.x) + (p2.y - p1.y) * (p2.y - p1.y))
Now the *what* is the method name and the why is the comment (though even that could be a method name as well).
Post new comment