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;
 }
}

the dictionary is mightier than the associative array

Developers tend to overlook (or not know about) Dictionaries. In AS 3.0, a Dictionary is similar to an associative array, except it takes objects as keys instead of Strings. Compare this use of an associative array ...

var pointsForHittingEnemies : Array = new Array();
pointsForHittingEnemies[ "dracula" ] = 100; 
pointsForHittingEnemies[ "mummy" ] = 50; 
pointsForHittingEnemies[ "frankenstein" ] = 25; 

... with this use of a Dictionary ...

import flash.utils.Dictionary;

var dracula : Monster = new Monster();
var mummy : Monster = new Monster();
var frankenstein : Monster = new Monster();

var pointsForHittingEnemies : Dictionary = new Dictionary();
pointsForHittingEnemies[ dracula ] = 100; 
pointsForHittingEnemies[ mummy ] = 50; 
pointsForHittingEnemies[ frankenstein ] = 25; 

In the second example, note the lack of quotation marks around dracula, mummy and frankenstein. The indices to the Dictionary version of pointForHittingEnemies are objects, not Strings as in the Array version.

Dictionaries are really useful when you need to associate random bits of data with objects. The way this often rears its head for me is when I want to associate random data with a Sprite.

Let's say I am making a playlist of videos. I want a button for each video on the list. The buttons will be Sprites, but how do I associate a Sprite with a particular video, so when the user clicks that Sprite, the associated video will play?

Here's one solution:

import flash.utils.Dictionary;
import flash.display.Sprite;
import flash.events.MouseEvent;

var videos : Array = ["TheGodfather.flv", "2001.flv","GoneWithTheWind.flv"];
var buttonList : Dictionary;

init();

function init() : void
{ 
 var buttonList = new Dictionary();
 var totalVideos : int = videos.length;
 for ( var i : int = 0; i < totalVideos; i++ ) makeButton( i );
}

function makeButtons( index : int ) : void
{
 var button : Sprite = new Sprite();
 button.buttonMode = true;
 button.x = index * 100; //to space buttons out across stage
 
 //add the button to the Dictionary
 //as a key to the video's index
 buttonList[ button ] = index;
 
 //draws stuff on the button (function definition not shown)
 renderButton( button );
 
 addChild( button );
 
 //take a look at the callback function
 //to see how I use the Dictionary
 button.addEventListener( MouseEvent.CLICK, buttonClickCallback );
}

function buttonClickCallback( event : MouseEvent ) : void
{
 var button : Sprite = event.currentTarget as Sprite;
 
 //look up button in the Dictionary and get the
 //appropriate video index
 var index : int = buttonList[ button ];
 
 var videoUrl : String = videos[ index ];
 
 //call to a function that plays a video based on its URL.
 //function definition not shown
 playVideo( videoUrl ); 
}

Notes: instead of storing indices in my Dictionary, I could have stored the actual URLs of the videos:

buttonList[ button ] = videos[ index ];

In this case, I would have written the callback function as follows:

function buttonClickCallback( event : MouseEvent ) : void
{
 var button : Sprite = event.currentTarget as Sprite;
 
 //look up button in the Dictionary and get the
 //appropriate video url
 var videoUrl : String = buttonList[ videoUrl ];
 
 //call to a function that plays a video based on its URL.
 //function definition not shown
 playVideo( videoUrl ); 
}

I like Dictionaries, because they're simple and powerful. However, I only use them for really quick-fix chores, usually when I'm prototyping something on the Timeline. A more robust solution for associating data with objects is the Class structure:

class VideoButton extends Sprite
{
 private var _videoUrl : String;
 
 public function get videoUrl() : String { return _videoUrl }
 public function set videoUrl( value : String ) : void
 {
  _videoUrl = value;
 }
}

Now, in my main document, import the VideoButton class and rewrite the makeButtons function as follows:

function makeButtons( index : int ) : void
{
 var button : VideoButton = new VideoButton();
 button.buttonMode = true;
 button.x = index * 100; //to space buttons out across stage
 
 //store the video's url right in the object itself
 button.videoUrl = videos[ index ];
 
 //draws stuff on the button (function definition not shown)
 renderButton( button );
 
 addChild( button );
 
 //take a look at the callback function
 //to see how I use the Dictionary
 button.addEventListener( MouseEvent.CLICK, buttonClickCallback );
}

And the callback will now look like this:

function buttonClickCallback( event : MouseEvent ) : void
{
 var button : VideoButton = event.currentTarget as VideoButton;
 
 //call to a function that plays a video based on its URL.
 //function definition not shown
 playVideo( button.videoUrl ); 
}

parameters suck

I'm learning Objective-C, and I've come to love the way you make method calls in that language. If you'd type this in Actionscript...

shapes.arrow( 0xFF0000, 0x00FF00 )

... you'd type something like this in Objective-C:

[shapes arrowWithStrokeColor: 0xFF0000 andFillColor: 0x00FF00 ]

Though they are more verbose, Objective-C method calls make the purpose of their arguments clear.

With code, it's more important to cater to the reader than to the writer (who may wish to save himself some keystrokes.) Code gets read many more times than it gets written. If verbosity is the price we have to pay for readable code, it's time to start digging into our wallets.

I don't want to read shapes.arrow( 0xFF0000, 0x00FF00 ). I can guess that the two hex values are colors, but I don't know which is the fill color and which is the stroke color. To find out, I have to search for the method definition, which is probably in a different file from the one I'm looking at. That slows me down and makes me think about file-navigation in my IDE, rather than about the meaning of the code.

Unfortunately, Actionscript doesn't give you an elegant solution to this problem, so most developers don't bother trying to solve it. But it's a serious problem, and there are ways to deal with it:

1) via comments:

// parameters: strokeColor, fillColor
shapes.arrow( 0xFF0000, 0x00FF00 )

This is a bad solution, because comments and the code they refer to can easily get out of sync. Say that you (or someone else) changes the definition of shapes.arrow() to...

public function arrow( fillColor : uint, strokeColor: uint ) : void { ... }

... but forgets to update the comment. It will now be an incorrect and misleading comment. This is an easy mistake to make, because the comment and the method definition are in different locations. You might make the update in one place and forget to make it in the other.

In general, I'm not a fan of comments. It's too easy to use them to explain messy code. It's much better to clean the code.

2) via descriptive-method names:

shapes.arrowWithStrokAndFillColor( 0xFF0000, 0x00FF00 )

This tends to stretch method names to the point that they get hard to read, and it get absurd if there are more than a couple of parameters:

shapes.arrowWithStrokeWidthAndStrokeColorAndFillColor( 1, 0xFF0000, 0x00FF00 );

However, in certain cases, when you refer to parameters in method names, it makes the code much easier to read. Compare...

find( name, names )

... with ...

findItemInList( name, names )

Of course, if you're using someone else's API, you don't have the luxury of naming their methods. In that case, I recommend...

3) via variables:

compare ...

shapes.arrow( 0xFF0000, 0x00FF00 )

... with ...

var strokeColor : uint = 0xFF0000;
var fillColor : uint = 0x00FF00;

shapes.arrow( fillCollor, strokeColor );

This is my favorite solution, because it uses code as commentary. Self-commenting code is always better than obscure code with comments.

Bottom line: when you find yourself typing something like move( object, 1, 5, false, [10,10] ), think about what you can do to make your code readable. You may not have written the move() function, but you did write the call to it. It's your responsibility to use whatever structures exist in your language to make that call clear.

to block or not to block

"Best practices" documents tell me to always follow an if, while or for statements with a block of code. They suggest I should type this...

if ( missileHitAlienShip() )
{
 score += 100;
}

...and never this...

if ( missileHitAlienShip() ) score += 100;

I disagree. I find the latter version much easier to read. If you translate the two versions into English, you get "if a missile hits the alien ship, do the following: raise the player's score by 100" and "if a missile hits the alien ship, raise the player's score by 100." I prefer the second version.

When I present them by themselves, the two versions don't look that different, and the block version isn't all that difficult to read. But when a bunch of blocks are nested inside one another, The code quickly gets difficult to read. Conditional logic gets all mixed up with nitty-gritty implementation details. Compare...

function addToScores() : void
{
 if ( levelNumber > 10 )
 {
  bonus += 500; 
 }
 
 for ( var i : int = 0; i < numPlayers; i++ )
 {
  player[ i ].score += bonus;
 }
}

... with ...

function addToScores() : void
{
 if ( levelNumber > 10 ) bonus += 500;
 
 for ( var i : int = 0; i < numPlayers; i++ )
   player[ i ].score += bonus;
}

The "best practices" docs say that you should use blocks because they allow you to easily switch from a single statement to multiple statements. For instance, if I start with something like this...

function addNewAlienShip() : void
{
 var ship : AlienShip = new AllienShip();
 
 if ( shipsHaveWarpDrive ) ship.x = Math.random() * universe.width;
 else ship.x = alienBase.x; 
}

... and later realize I need to also set y-coordinates, I'll need to change my code to something like this:

function addNewAlienShip() : void
{
 var ship : AlienShip = new AllienShip();
 
 if ( shipsHaveWarpDrive ) 
 {
  ship.x = Math.random() * universe.width;
  ship.y = Math.random() * universe.height;
 }
 else 
 {
  ship.x = alienBase.x;
  ship.y = alienBase.y;
 }
}

Since I never know how many statements I'll have to add after an if, I might as well start with a block -- as a sort of template -- even if I initially only intend to follow the if with one statement. Or so the "best practices" logic goes.

I disagree. I'm not a fan of blocks, because they encourage multiple levels of nesting and make methods overly long and hard to read. Given the example above, which may be my initial draft, I'd refactor it as...

function addNewAlienShip() : void
{
 var ship : AlienShip = new AllienShip();
 
 if ( shipsHaveWarpDrive ) putInRandomLocation( ship );
 else putInBase( ship );
} 

function putInRandomLocation( ship : AlienShip ) : void
{
 ship.x = Math.random() * universe.width;
 ship.y = Math.random() * universe.height;
}

function putInBase( ship: AlienShip ) : void
{
 ship.x = alienBase.x;
 ship.y = alienBase.y;
}

This is easier to read and modular. If I ever want to change the way warp drives place ships, I'll have an easy time doing so without searching through levels of nesting.

I am not dogmatic about forbidding blocks. If a block seems like a simple and clean solution, I use it. But I initially follow my ifs, whiles and fors with a single statement. If I later realize that I need to add more statements, I make a decision. I sometimes replace the single statement with a block. More often, I turn the single statement into a function call, as I did in the example above.

A couple of years ago, I turned a major corner in my life as a coder: I started taking three maxims seriously:

1) each function should do one thing.
2) functions should be as short as possible.
3) nested block are evil.

Previously, I'd given lip-service to these ideas, but when I started following them religiously, my code got way easier to read and maintain.

I have found simplicity on the method level to be more important than simplicity on the class level. Since OOP texts mostly focus on classes, many programmers pay more attention to them than to methods. That's like paying more attention to chapters than to sentences. Good writers know that the sentence is where the real work happens. A well organized table of contents is worthless if each chapter is full of non-sensical sentences.

Again, I am not dogmatic about my three rules, and I often can't follow them when making an initial draft. But when I refactor, I take a close look at any method that is longer than five or six statements. And I take an even closer look at blocks. My goal is to write a group of simple, short functions that each works as one little cog in my big machine.

the zen of curly braces

As one of the unwashed millions who hopes to make his fame and fortune as an iPhone developer, I've been learning Objective-C. My main teacher has been "Programming in Objective-C 2.0" by Stephen Kochan, a book which deserves its accolades.

At some point, Kochan says something like, "In general, you can substitute a block of code for a single statement." I thought about that for a second and realized that, simple as that sounds, I'd never really thought about it before.

Of course, I know that you can -- in all the c-family languages -- write...

if ( condition ) statement;

or

if ( condition )
{
 block;
 of;
 statements;
}

... but I hadn't considered statement/block substitution as a general rule.

To test things out, I tried this timeline code in an AS3.0 fla:

{
 var a : int = 3;
 trace( a );
}

{
 for ( var i : int = 0; i < 10; i++ ) trace( i );
}

{}

And it worked!

Now, it's not that I think this is useful. I don't. But I love it when I can generalize some specific behaviors into a principle. When this code worked, I felt some contents drifting together. I suddenly understood a bit of Actionscript grammar that I'd never really considered before.

Actually, I can think of a use for this: refactoring. Say that as a first draft, you write a method like this:

public function init() : void
{
 drawBackground();
 drawPlayer();
 drawEnemies();
 setPlayerLives();
 setPlayerStartingPosition();
 drawPlayerShip();
 setEnemyStaringPositions();
 setEnemyStrength();
 setPlayerStrength();
 startGame();
}

Your first step in refactoring could be to group statements into blocks:

public function init() : void
{
 {
  drawBackground();
  drawPlayer();
  drawEnemies();
 }
 
 {
  setPlayerLives();
  setPlayerStartingPosition();
 }
 
 {
  drawPlayerShip();
 }
 
 {
  setEnemyStaringPositions();
 }
 
 {
  setEnemyStrength();
  setPlayerStrength();
 }
 
 startGame();
}

I like this, because it suggests groupings without affecting functionality. init should run exactly the way it did before you added the blocks.

Having added them, you can see some better ways of organizing the statements:

public function init() : void
{
 {
  drawBackground();
  drawPlayer();
  drawEnemies();
  drawPlayerShip();
 }
 
 {
  setPlayerLives();
  setPlayerStartingPosition();
  setPlayerStrength();
 }
 
 {
  setEnemyStaringPositions();
  setEnemyStrength();
 }
 
 startGame();
}

This, with a little cutting and pasting, leads naturally to...

public function init() : void
{
 renderGamePieces();
 initPlayer();
 initEnemies();
 startGame();
}

private function renderGamePieces() : void 
{
 drawBackground();
 drawPlayer();
 drawEnemies();
 drawPlayerShip();
}

private function initPlayer() : void
{
 setPlayerLives();
 setPlayerStartingPosition();
 setPlayerStrength();
}

private function initEnemies() : void
{
 setEnemyStaringPositions();
 setEnemyStrength();
}

incidentally, going back to my original example...

{
 var a : int = 3;
 trace( a );
}

{
 for ( var i : int = 0; i < 10; i++ ) trace( i );
}

{}

... I found that double nesting works, too:

{
 {
  var a : int = 3;
  trace( a );
 }

 {
  for ( var i : int = 0; i < 10; i++ ) trace( i );
 }

 {}
}

As does triple nesting:

{
 {
  {
   var a : int = 3;
   trace( a );
  }

  {
   for ( var i : int = 0; i < 10; i++ ) trace( i );
  }

  {}
 }
}

And so on.

Strangely, this doesn't work:

{
 var a : int = 3;
 trace( a );
}

{
 for ( var i : int = 0; i < 10; i++ ) trace( i );
}

{};

The problem here is the semi-colon after the final close-brace. It throws this error:

Syntax error: extra characters found after end of program.

I got the same error when I added an empty block to the start of the frame code:

{}

{
 var a : int = 3;
 trace( a );
}

{
 for ( var i : int = 0; i < 10; i++ ) trace( i );
}

And I also got it when I added an empty block in the middle of the code:

{
 var a : int = 3;
 trace( a );
}

{}

{
 for ( var i : int = 0; i < 10; i++ ) trace( i );
}

For some reason (that I don't understand), the AS interpreter assumes it's reached the end of your code when it finds an empty set of braces. The empty set doesn't cause an error on its own, but you will get an error if there are any characters following the the close-brace. This isn't a problem, since I can't think of a reason to ever include an empty block. But I am curious about what's going on.

Dumb mistakes.

Here are a couple of mistakes I make after a long day of coding. Can you spot the errors?

1. Stack Overflow

public class Person
{
 private var _name : String;
 
 public function get name() : String
 {
  if ( name == null ) name = "anonymous";
  return _name;
 }
 
 public function set name( value : String ) { _name = value };
}

The problem: run this code and you'll get an output panel full of "stack overflow" errors. Why? Well, take a closer look at the getter method. It's supposed to return the value of private member "_name". But the first statement in the method refers to "name," not "_name" (note the underscore). _name is the private member; name is the getter method.

When the first statement executes, it tests to see if THE OUTPUT OF THE GETTER METHOD ITSELF is equal to null. Remember, when you type the name of a getter method by itself, you're really just using a syntactic sugar. "name" is really "name()." So what I'm doing in the first statement is similar to this:

 function foo() : Number
 {
  if ( foo() == null ) trace( "x is equal to null" );
 }

The call foo() in the first statement calls the function foo, which has a call to foo in the first statement, which calls foo... and so on. An infinite recursive loop.

In the previous example, I can remove the loop by simply adding underscores to "name":

bad: if ( name == null ) name = "anonymous";
good: if ( _name == null ) _name = "anonymous";

Now name is not calling itself; it's referring to a private member called _name.


2. Null Envy

var herName : String;
var flower : Sprite;
var sheLovesMe : Boolean;
var herAge : Number;
var kisses : int;

if ( herName == null ) herName = "Lisa"; //works like a charm

if ( flower == null ) flower = new Sprite(); //works like a charm

if ( sheLovesMe == null ) sheLovesMe = false; //error

if ( herAge == null ) herAge = 18; //error

if ( kisses == null ) kisses = 0; //error

The problem here is that Booleans and numbers types can NEVER be set to null. They don't accept null as a value; nor can they be compared to null.

Once you define a Boolean, it is automatically set to false. (You can later change its value to true.) There's no such thing as a Boolean with no value, though you can set a Boolean to undefined.

var iLikeCheese : Boolean; //note: not explicitly set to a value
trace( iLikeCheese ); //false

You also can't set or compare a Number to null. Unset numbers are set by default to NaN, which is short for "not a number." However, you can't do this:

if ( herAge == NaN ) herAge = 18; //error

Instead, you have to do this:

if ( isNaN( herAge) ) herAge = 18; //works like a charm

ints and uints default to zero. If I'm using an int as a uint -- in other words, if my int should never be set to a negative number -- I sometimes set it to negative one by default. For instance, you can't give a girl a negative kiss, so...

public static const NOT_SET : int = -1;

var kisses : int = NOT_SET;

if ( kisses == NOT_SET ) kisses = 10;

For this reason, I often use ints to store hex-color values. How else should I indicated no color? After all, zero is black. I suppose I could use a uint and define something like this:

public const NO_COLOR : uint = uint.MAX_VALUE; //MAX_VALUE is a built-in constant

var favoriteColor : uint = NO_COLOR; 

if ( favoriteColor == NO_COLOR ) favoriteColor = 0xFF0000;

This solution is probably good enough for most simple circumstances, but if I need something more robust and scalable, I create a class:

public class Color
{
 public static const NO_COLOR : uint = uint.MAX_VALUE;
 private var _color : uint = Color.NO_COLOR;
 
 public function get color() : uint { return _color; }
 public function set color( value : uint ) { _color = value; }
 public function isSet() : Boolean { return _color == Color.NO_COLOR; }
}

looping through a class's static members

How do you loop through all static members of a class?

First of all, why would you want to do that? Well, let's say you're writing a game in which the player can move in the four classic compass directions:

public class Directions
{
 public static const NORTH : String = "north";
 public static const SOUTH : String = "south";
 public static const EAST : String = "east";
 public static const WEST : String = "west";
}

Somewhere in your game, you have a moveInDirection() method that accepts a direction. But you want to make sure it doesn't try to move the player in illegal directions, e.g. moveInDirection("up"); The IDE should prevent this, because if you type moveInDirection(Directions.UP) you'll get an error (because there's no UP constant).

However, I'd like to make moveInDirection a bit more idiot proof than having it rely on the IDE. Also, it's possible that directions may be chosen at run time. For instance, a player might type a direction in a textfield, as in a text-adventure game. What if the player types "up"?

So I'd like moveInDirection() to check if the player is trying to make a legal move...

function movieInDirection( direction : String ) : void
{
 if ( isLegal( direction ) )
 {
  ...
 }
}

The question is, how should isLegal() work? The last thing I want to do is to use a bunch of conditionals or an Array...

function isLegal( direction : String ) : Boolean
{
 var legal : Array = [ "north", "south", "east", "west" ];
 
 for each ( var legalDirection : String in legal )
 {
   if ( direction == legalDirection ) return true;
 }
 
 return false;
}

That's a terrible solution, because it couples the class Directions to the Array in my isLegal function. Scaling will be dangerous. For instance, what if I add a new constant to Directions...

public static const INSIDE : String = "inside";

... but forget to add it to the Array?

var legal : Array = [ "north", "south", "east", "west" ]; //Oops! Forgot to add "inside"

A better solution is to loop through all the constants in Directions and check if the player's attempted direction exists. But how do you loop through all the static members of a class? After playing around with arcane solutions involving prototype chains, I settled on using the little-known describeType() function. It's in the flash.utils class, and you import it like this:

import flash.utils.describeType;

If you feed describleType() the Directions class -- trace( describeType( Directions) ) -- it outputs an XML description of the class:

<type name="Directions" base="Class" isDynamic="true" isFinal="true" isStatic="true">
  <extendsClass type="Class"/>
  <extendsClass type="Object"/>
  <constant name="WEST" type="String"/>
  <constant name="NORTH" type="String"/>
  <constant name="EAST" type="String"/>
  <constant name="SOUTH" type="String"/>
  <accessor name="prototype" access="readonly" type="*" declaredBy="Class"/>
  <factory type="Directions">
    <extendsClass type="Enum"/>
    <extendsClass type="Object"/>
  </factory>
</type>

It's then a fairly easy matter to parse the xml, extract all the constants, and test them against the player's choice. I packaged the solution in a utility class:

package com.grumblebee.constants 
{

import flash.utils.describeType;    

  public class Utilities 
  {
     public static function isLegal( enumClass : Class, value : String ) : Boolean
     {
        var xml : XML = describeType( enumClass );
        var xmlList : XMLList = xml.child( "constant" );
        var enumName : String;
      
        for each ( var child : XML in xmlList )
        {
          enumName = child.attribute( "name" ).toString();
          if ( enumClass[ enumName ] == value ) return true;
        }
   
        return false;
     }
     
  }
}

I can't shake the feeling that there's a better way to do this. I'd love to lose the XML parsing. But this is the best I can come up with for the moment. One thing I could do is transform the player's string to upper-case and test it directly to see if there's such a constant in Directions:

if ( Directions[ playerString.toUpperCase() ] != undefined ) ...

But that only works if I stick to the convention of NORTH = "north". It fails if Directions looks like this:

public class Directions
{
 public static const NORTH : String = "n";
 public static const SOUTH : String = "s";
 public static const EAST : String = "e";
 public static const WEST : String = "w";
}

it would also fail in the case of...

public static const UNDER_THE_BRIDGE : String = "under the bridge";

At one point, I thought about adding the isLegal() function to Directions. You can get a class to refer to its own static constants this way:

trace( prototype.constructor[ "NORTH" ] ); //north

Sadly, though prototype.constructor allows you to access individual class variables and constants, for some reason, you can't loop through its members:

for each ( var s: String in prototype.constant ) trace( prototype.constant[ s ] ); //no output

It also doesn't work if you explicitly name the class:

trace( Directions[ "NORTH"] ); //north
for each ( var s : String in Directions ) trace( Directions[ s ] ); //no output.

So until something better comes along, I'm stuck with XML parsing.