Friday, July 24, 2009

a little lesson in refactoring

Here's a secret you already know: great writers revise. I learned more about writing by looking at an early draft of George Orwell's "1984" than I did in any writing class. Orwell's prose seems so effortless. Yet its simplicity belies all the sweat he poured into it. Every page of his draft is riddled with multiple edits, cross-outs and revisions.



Here's a secret you may not know: good programmers revise. When they get their program working, they know they've only begun their labor. The program is finished when it's easy for humans to read.

Since I learned so much from looking over Orwell's shoulder, I wish I could do the same with lots of programmers, watching them refactor their work to make it clear.

Unfortunately, most programming books omit this step. They either show you rough code that cries out for refactoring. Or they show you clean code that looks like it sprung into existence that way.

When I first started programming, I wondered why my code never looked as good as examples in books. What I didn't realize is that the book examples were the end result of a hidden process, one that involved many drafts and revisions.

So that you don't suffer the same confusion I did, I'm going to show you my revision process, starting with some very rough code I actually wrote. If you enjoy this process and want to learn more, I recommend the following books:

- "Clean Code" by Robert C. Martin.

- "Refactoring" by Martin Fowler and others.

- "Code Complete" by Steve McConnell.

I'm not going to demo a complex piece of code. I'm going to show you the sort of thing you've probably written a hundred times: code to draw a rectangle.

Going into my project, I knew I'd have to draw a bunch of rectangles for various background elements and buttons. I knew that some of them would have both fills and strokes, others would have just fills, and still others would have just strokes. The ones with strokes would have various stroke widths.

I decided to make a CustomRectangle class that allowed for all of these variations. Here's my first attempt at the class header and its constructor:


import flash.display.Sprite;
import com.grumblebee.enums.Colors;

public class CustomRectangle extends Sprite
{
private var _displayWidth : Number;
private var _displayHeight : Number;
private var _fillColor : uint;
private var _strokeColor : uint;
private var _strokeWidth : Number;

public function CustomRectangle( displayWidth : Number = 0,
displayHeight : Number = 0,
fillColor : uint = Colors.NO_COLOR,
strokeColor : uint = Colors.NO_COLOR
strokeWidth : Number = 1 )
{
_displayWidth = displayWidth;
_displayHeight = displayHeight;
_fillColor = fillColor;
_strokeColor = strokeColor;
_strokeWidth = strokeWidth;

render();
}

private function render() { ... }
}



You may be wondering about Colors.NO_COLOR. Since I'm using uints to store colors, I need a way to un-set a color value. ints and uints default to zero, and in the hex-color system, zero is black (0x000000). ints and uints can't be set to undefined or null, so where does that leave me? How can I say "there's no fill color?"

It leaves me with uint.MAX_VALUE. uint.MAX_VALUE is a built-in constant that holds the largest number your system can store in a unit (int has both MAX_VALUE and MIN_VALUE constants).

MAX_VALUE might resolve to a different number on different OSes. Out of curiosity, I decided to trace( uint.MAX_VALUE ) on my Mac. As it happens, my Mac's MAX_VALUE for uints is 4,294,967,295. That's way larger than the largest color-value, which is 0xFFFFFF or 16,777,215. (Though it's NOT larger than 0xFFFFFFFF, so if you're using alpha+color hex numbers, you need a different solution!)

So I decided that a "color value" of uint.MAX_VALUE equals no color. To make my code a bit more readable, I created an enum class for colors:


public class Colors
{
public static const NO_COLOR : uint = uint.MAX_VALUE;
}


Getting back to my CustomRectangle class, I now needed to define the render method:


private function render() : void
{
if ( _strokeColor != Colors.NO_COLOR && _fillColor != Colors.NO_COLOR )
{
if ( _strokeColor != Colors.NO_COLOR )
graphics.lineStyle( _strokeWidth, _strokeColor );

if ( _fillColor != Colors.NO_COLOR )
graphics.beginFill( _fillColor );

graphics.drawRect( 0, 0, _displayWidth, _displayHeight );
graphics.endFill();
}
}


That's not terrible code by any means, but it doesn't pass the Headache Test (HT). To give your code the HT, unfocus your eyes slightly and look at it. If you see a mess of stuff in parenthesis and multiple levels of nesting, your code fails the HT. Don't feel bad. Most first-draft code fails.

My code is not all that hard to figure out. But a future reader (e.g. me next week) doesn't want to waste his time trying to figure it out. He wants to just look at it and GET it! So let's help him out.

First, let's rector the outer-most conditional. Any time you see && or ||, a refactoring bell should go off in your head. Why? Because multiple conditions are headache inducing. They are even that way in English. Compare...

"I'll take you to the zoo if it's not raining tomorrow and if the temperature is above 50 degrees."

... with ...

"I'll take you to the zoo tomorrow if it's a nice day."

Along these lines, we'll refactor the code as follows:


private function render() : void
{
if ( isRenderable() )
{
...
}
}


And we'll move the ugly conditional into it's own helper method:


private function isRenderable() : Boolean
{
if ( _strokeColor != Colors.NO_COLOR && _fillColor != Colors.NO_COLOR )
return true;
return false;
}


In case you didn't know, you don't have to wrap the second return statement in an else clause. Return statements always cause their methods to end immediately, so if the "return true" executes, the "return false" will never run.

You might argue that we didn't really simplify the compound-if statement. We just moved it. I agree, and sometimes that's the best you can do. At least we've burried the ugly code. Readers who don't want a headache can just read as far as the call to isRenderable().

But we can do better: let's refactor the two clauses of the compound if into two separate methods:


private function isRenderable() : Boolean
{
if ( hasStroke() || hasFill() ) return true;
return false;
}

private function hasStroke() : Boolean
{
if ( _strokeColor != Colors.NO_COLOR ) return true;
return false;
}

private function hasFill() : Boolean
{
if ( _fillColor != Colors.NO_COLOR ) return true;
return false;
}


Though we just wrote hasFill() and hasStroke() to simplify the isRenderable() method, it now looks like they can do double-duty and also help us simplify render(). Let's change this...


private function render() : void
{
if ( isRenderable() )
{
if ( _strokeColor != Colors.NO_COLOR )
graphics.lineStyle( _strokeWidth, _strokeColor );

if ( _fillColor != Colors.NO_COLOR )
graphics.beginFill( _fillColor );

graphics.drawRect( 0, 0, _displayWidth, _displayHeight );
graphics.endFill();
}
}


... to this ...


private function render() : void
{
if ( isRenderable() )
{
if ( hasStroke() )
graphics.lineStyle( _strokeWidth, _strokeColor );

if ( hasFill() )
graphics.beginFill( _fillColor );

graphics.drawRect( 0, 0, _displayWidth, _displayHeight );
graphics.endFill();
}
}


... which I hope you'll agree is less of a headache to read. But I still think we can do better. Let's define two more helper methods:


private function setFill() : void
{
graphics.beginFill( _fillColor );
}

private function setStroke() : void
{
graphics.lineStyle( _strokeWidth, _strokeColor );
}


With these puppies available, we can refactor is render to this:


private function render() : void
{
if ( isRenderable() )
{
if ( hasStroke() ) setStroke();

if ( hasFill() ) setFill();

graphics.drawRect( 0, 0, _displayWidth, _displayHeight );
graphics.endFill();
}
}


One last migrane: the nested ifs. Is there any way we can make this method have only one level of nesting? Sure there is!


private function render() : void
{
if ( ! isRenderable() ) return;

if ( hasStroke() ) setStroke();

if ( hasFill() ) setFill();

graphics.drawRect( 0, 0, _displayWidth, _displayHeight );
graphics.endFill();
}


Now THAT passes the Headache Test!

Remember, a return instantly cause the method to end. So if the rectangle is not renderable, the code after the first if won't even run.

Which leaves us with one last headache: the constructor.


public function CustomRectangle( displayWidth : Number = 0,
displayHeight : Number = 0,
fillColor : uint = Colors.NO_COLOR,
strokeColor : uint = Colors.NO_COLOR
strokeWidth : Number = 1 )
{
_displayWidth = displayWidth;
_displayHeight = displayHeight;
_fillColor = fillColor;
_strokeColor = strokeColor;
_strokeWidth = strokeWidth;

render();
}


My problem with this is that it has a big parameter list. Looking at that list makes my head spin a little. I'd like the reader to know that that method expects render data and leave it at that. If the reader wants to know more, he can drill down farther. If he doesn't care, he can ignore the details.

One way to deal with classes that need a lot of set-up data is to make a bunch of little setter methods:


public function CustomRectangle() { }
public function set displayWidth ( value : Number ) { _displayWidth = value; }
public function set displayHeight ( value : Number ) { _displayHeight = value; }

...

public function render() : void { ... }


My problem with this is that it's really hard to tell what you need to do to set up the class and what you is optional. Is it okay to do this?


var rect : CustomRectangle = new CustomRectangle();
rect.displayWidth = 100;
render();


Will this class blow up because I didn't set displayHeight or any of the other properties? Do I have to set them all? Does it matter what order I set them in? The nice thing about variables that get set via constructor calls is that there's no ambiguity. If you forget one, the compiler will yell at you. But this can lead to parameter bloat, as we saw above.

The solution is to create a new object, a Value Object (a.k.a Data Transfer Object) and pass that to the constructor:


public class RenderData
{
public var displayWidth = 0;
public var displayHeight = 0;
public var fillColor : uint = Colors.NO_COLOR;
public var strokeColor : uint = Colors.NO_COLOR;
public var strokeWidth : Number = 1;
}


We can now refactor our constructor to this:


public function CustomRectangle( renderData : RenderData )
{
_displayWidth = renderData.displayWidth;
_displayHeight = renderData.displayHeight;
_fillColor = renderData.fillColor;
_strokeColor = renderData.strokeColor;
_strokeWidth = renderData.strokeWidth;

render();
}


Or, if we want, we can organize this way:


public function CustomRectangle( renderData : RenderData )
{
initWith( renderData );

render();
}

private function initWith( renderData : RenderData ) : void
{
_displayWidth = renderData.displayWidth;
_displayHeight = renderData.displayHeight;
_fillColor = renderData.fillColor;
_strokeColor = renderData.strokeColor;
_strokeWidth = renderData.strokeWidth;
}


Ah, better than Rolaids and Bufferin combined!

Here's our final draft. Take a look at this in a month. I bet it will still be instantly clear to you how it works.


class Custom Rectangle extends Sprite
{
public function CustomRectangle( renderData : RenderData )
{
initWith( renderData );

render();
}

private function initWith( renderData : RenderData ) : void
{
_displayWidth = renderData.displayWidth;
_displayHeight = renderData.displayHeight;
_fillColor = renderData.fillColor;
_strokeColor = renderData.strokeColor;
_strokeWidth = renderData.strokeWidth;
}

private function render() : void
{
if ( ! isRenderable() ) return;

if ( hasStroke() ) setStroke();

if ( hasFill() ) setFill();

graphics.drawRect( 0, 0, _displayWidth, _displayHeight );
graphics.endFill();
}

private function setFill() : void
{
graphics.beginFill( _fillColor );
}

private function setStroke() : void
{
graphics.lineStyle( _strokeWidth, _strokeColor );
}

private function isRenderable() : Boolean
{
if ( hasStroke() || hasFill() ) return true;
return false;
}

private function hasStroke() : Boolean
{
if ( _strokeColor != Colors.NO_COLOR ) return true;
return false;
}

private function hasFill() : Boolean
{
if ( _fillColor != Colors.NO_COLOR ) return true;
return false;
}
}

No comments:

Post a Comment