Refactoring code smells: Is it risky? Is it wise? Is it a waste of time?

Question

Given a long method with Pac-Man ifs, would it be wise to:

1. Break down code blocks into regions.
2. Then refactor code blocks into methods.

Or would it be best to leave it alone?

Would it be risky?
Would it be a waste of time?

In the absence of automated unit-tests, I'm trying to understand the risk-reward relationship associated with this manuever.

Answer 1

Your plan, or clean up the logic on the ifs (in cases where the pacman structure is an inefficient order or view of the conditions) is usually the place to start IMHO.

As to if it is worth the effort:

• do you expect to be changing this page much?
• do you have trouble understanding what is going on in there to the point you are wary of modifying it?
• do you think it is causing a performance or stability problem?
• is there no real reason it was done this way in the first place?
• would it be trivial to manually validate the correctness of the re-factor?

IMHO unless you have some strong "yes" answers to those questions re-factoring it is a questionable exercise when compared to the risk of botching it and the fact that you could be adding new value elsewhere with your time.

Answer 2

It's risky in the absence of automated tests. Of course you'll test as you go along, but that's tedious, and error-prone. Ideally you'd write unit tests capturing the current behaviour, but that's not always feasible.

Having been in this position before (with PHP), if the code really stinks, refactor it very slowly and very carefully.

I think your plan's sound - take the long method, break it up with whitespace into paragraphs, and try Extract Method. Quite how you do that depends on how many variables are in play, and how they interact. Try trace the dependencies of a paragraph before you start.

Is it worth it? If the code's really awful, and its awfulness is starting to spread, then it's time to get stuck in there. If it's been running fine for ages, then you risk introducing bugs into something that for now Just Works.

Answer 3

0 . Work out what the logic of the if statements is.

Quite often when I'm looking at a set of pac-man ifs I find that if I just draw out something like a truth table of all the conditions involved I can work out a much better route to resolving the problem.

That way you can also assess whether there is a better method, how you might break it down further and ( and this is a big one with this kind of code ) whether there are any holes in the logic.

Having done that you can probably break it down into a couple of switch statements and a couple of methods and save the next poor mook who has to go through the code a whole lot of problems.

Answer 4

The code you linked to is not editable. If you don’t have to touch it, leave it. If you have to touch it, having unit tests would be nice. So write unit tests, and make sure that if you change any single condition, some unit test breaks.

Now the linked code is not difficult to change but difficult to change without making any mistakes. One way that helps is two people making the changes individually and then you check that you have the exact same code twice. It improves your chances.

Now a systematic way to fix it in this case of massively nested ifs and else’s: Add a variable “bool done = false”. Then you change from “if(cond1) { things1; if (cond2) { things2; if (cond3) { things3 } } } to

If (! Cond1) done = true;  
If (! Done) thing1;  
If (! Done && ! Cond2) done = true;
If (!done) thing2;
If (! Done && ! Cond3) done = true;
If (! Done) thing3;

And you can do that for miles.

What you don’t do: Change this total mess of code without refactoring. Or write code that you think is correct: During refactoring you write code that has exactly the same behaviour, whether it’s right or wrong. If anything in the end result looks strange, then either you have found a hidden bug, or you have found that the correct behaviour isn’t what you thought is correct. Either way you want to know.

That’s the rule with refactoring: Don’t even try to figure out what the code does, but change it purely mechanical so the semantics stays identical while the code becomes readable. At that point it should be editable in case you need to change it, and what it does should become obvious, whether it is correct or not.

Answer 5

I think your time would be better spent (assuming you have the time) writing unit tests for the method as-is.

From there, you can be much more confident in your refactor if you decide to do it.

Now when deciding to do the refactor, there are a lot of things to consider:

• what are my other priorities?
• will a refactor benefit anything? Performance? Readability? Reliability? Developer experience?
• do I have the resources or capacity to test this change? Yes, unit tests are great, but usually you’d want other layers of testing as well.
• is this code going to be sticking around for awhile or is it an aging feature that offers little value to my end users and may be sunset soon?

Answer 6

Are you "taskless"? The wingspan of intended improvements might be considerable and unless quality component¹ is the sole concern then don't bother.

It can be so risky you make it be. The absence of unit tests is a down side, the up side is that the unit tests could be written before refactoring since the intent is to preserve existing behaviour rather than unveil deviations from the requirements. Another side without unit tests is to verify through an automated tool² the results of external calls that have to preserve after the refactoring.

The answer to all questions from the description are with you. You just have to understand your context to proceed with what it is comfortable.

---

¹ Quality component of project management triangle (scope, time, quality)
² In software testing, test automation is the use of software separate from the software being tested to control the execution of tests and the comparison of actual outcomes with predicted outcomes. (from test automation wikipedia page)