Saturday 6 February 2010

Refactoring #1

I was introduced to a code the other day, that had a bug. It was pretty easy to reproduce, and I could easily create a Selenium test case for the problem. I was told, that the code is a mess and I'm free to do whatever I want.
When you're doing refactoring, you should always know what exactly you are doing. In this case the goal was to eliminate the bug causing the security issues. The ~500LOC method had an enormous cyclometric complexity, and more static calls than I've ever written. At least it was in some sort of a class, but not a single comment were there to guide me through the tough logic that merges security settings from various extensions (modules) and calculates the dependencies and verifies values and draws forms and ... hard-to-test code heaven...

So what to do when you see a
With a client code:
If refactoring the statics is not yet an option, use wrappers!
Then you can easily add the dependencies for your method

And there you go. You have the statics in control.

The client code needs to be modified:

The other thing I've bumped into is using $_POST and $_SERVER in the methods.
Doing this you won't have such a hard time testing, but you'll hopefully won't want to set variables in these arrays by hand to reproduce some scenarios.

What's wrong with globals?

Accessing global state statically doesn’t clarify those shared dependencies to readers of the constructors and methods that use the Global State. Global State and Singletons make APIs lie about their true dependencies. To really understand the dependencies, developers must read every line of code. It causes Spooky Action at a Distance: when running test suites, global state mutated in one test can cause a subsequent or parallel test to fail unexpectedly.
Misko Hevery: Flaw: Brittle Global State & Singletons

So what can you do? Give them to your method. Or if they're used widely in your code, pass them in the constructor. (you do have one do you?)
When I realized I need all dependencies throughout the whole class, I've moved them up one level.

Then the client code wil be:

The other thing you should look for in a code is the usage of the variables. For example, when I've searched for "where do I use the $this->_serverVars", I've found the following:

You should note, that the code is only using the 'REQUEST_METHOD' element, nothing else.

Tell, Don't Ask

The "Tell, Don't Ask" principle suggests that you should tell objects what to do rather than ask for their state (i.e., their data), make a decision, and then tell them what to do.

The guiding idea behind this is that an object should avoid exposing it's internal state to another object. Consider a quote from The Art of Enbugging which discusses the story of "The Paperboy and the Wallet".
Suppose the paperboy comes to your door, demanding payment for the week. You turn around, and the paperboy pulls your wallet out of your back pocket, takes the two bucks, and puts the wallet back. Instead of asking for the wallet, the paperboy should tell the customer to pay the $2.00.

from & more @

So I've modified my code:

Then the client code wil be:

And there you have a nice, testable, mockable code. Here's the test file:

Of course testing is only easy when your base objects' responsibilities are clear and simple. In my case, I still haven't found the cause of the bug, since the whole code is more complex and still has a lot of work to do. In the next chapter of hunting down the strange error I'll fill you in with my experiences of finding out what and how the original code does, before you can carry on with refactoring.

No comments :

Post a Comment

Note: only a member of this blog may post a comment.