Refactoring code smells in ASP.NET Web Forms Code Behind Files: 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.