Consulting Chronicles #6: Refactoring

Introduction

Refactoring is the discipline of applying a multitude of small, low-risk techniques to a code base in order fix and improve it. While corroborated by the software community, employing such techniques in a consulting context can be challenging because people are involved, often in a negative situation.

You need 2 weapons to win the refactoring battle. First, you need to understand what refactoring techniques are at your disposal and how to implement them. Below, I’ve listed the core ones I see needed time and time again. Second, you need to have a plan on how you engage the client company and their employees to allow you to do the first. That’s covered in the next article.

I’ve talked about how you do various parts of these initiatives in previous articles. What I want to talk about today is 1 of the 2 things: The core things you need to be concerned about in refactoring. In the next article we’ll talk about a prioritized plan you should follow when engaging a client where refactoring is needed. These are aimed at being used in the worst situations, but can provide positive benefits in benign or benevolent situations as well.

You should be able to take this blog post and the next, and from them both progressively improve a poor quality code base as well as engage a challenging client amongst challenging circumstances.

Note On Languages

I’ve intermingled JavaScript, ActionScript, and Lua to help provide a variety of examples, show this list is technology agnostic, and provide corroboration of techniques across technologies. That and I miss coding Lua since I’ve been so busy and needed an excuse. This does have a strong focus with front-end languages, namely those which run in a browser.

Nomenclature

Factory/Decoder/Parsing: Taking any kind of data, such as JSON or XML, and converting  into something you need it for. This could be as simple as accessing properties on it to converting it to a completely different type of object, or even creating new objects from it.

Coding Conventions

• Factory Errors vs. Null: Below, I’m using both error and null. Error means if a Factory/function/decoder fails in anyway to understand its data, you throw an error. This helps in unit testing, and expose errors early in the weakest part of a front end application; those that utilize non-strongly typed data. Optionally, when you find a parsing error, you return null instead. Consumers of the Factory must test for null, but you mitigate runtime exceptions and expose faulty consumers. I like using both, but it’s a lot of work to be sure.
• Don’t Create Grenades: Regardless of language, you shouldn’t create grenades. This means don’t use throw/error. Instead, dispatch a custom error event that is opt-in. Consumers shouldn’t have to worry about the code they’re using bubbling up problems to their area of worry unless they’ve opted into those concerns. ActionScript, JavaScript, and Lua also don’t have a strongly typed ways to verify with the compiler that these contracts have been met, unlike Java’s throwable. Thus, don’t throw, inform to those who wish to know. Log regardless.
• Factory vs. VO Self Parsing: Some like Factory’s to parse external data (JSON/XML, etc.) into ValueObjects for an increased likehood of DRY code (shared parsing code that’s easier to unit test). Others like ValueObjects to serialize/deserialize themselves to reduce coding overhead as well as make the API easier to use. I’ve used both below, but I’ve found this is usually a team decision and also depends on how complex your VO’s are.
• Model vs. Command vs. Remote Proxy: From architecture perspective, some people prefer to have a Model handle Application Data, and a Command/Controller/Presenter handle remote Service calls to update that data. Some prefer wrapping both inside of a Remote Proxy (basically a Model that has serivce(s) inside it), and allowing Controllers/Mediators/Presenters/ViewControllers to make calls on those Remote Proxies. I use both Model & Commands below, but no Remote Proxies. It’s up to you and your team which you think is best. If you’ve gotten this far in the discussion, you’re clearly better off than most. The point here is solve for Model and/or Application logic encapsulation.
• try/catch: I use try/catch liberally. Developers concerned about performance, such as game developers or those using complicated parsing algorithms should take note the significant performance cost of try/catch as well as liberal use of ExtractMethod. This article assumes the code is in bad shape and you/your team needs to refactor it over many months to ensure your team can prevent fire drills, meet milestones, and start to build a firm foundation. If the code base has the following problems outlined in this article, your first order of business should be fixing them first before you start optimizing. If you really need performance, just use unit tests instead and pray.
• Singletons: While not a replacement for global variables, and generally frowned upon, well written ones are a good first step if they have access control and utilize strong-typing. (ex Model).

Refactoring Basics

Improving existing code can be done in 3 ways: handling exceptions, using a good architecture, using strong-typing if available, and following refactoring paths. The reason you use refactoring, especially in a consulting/contracting context with existing code bases is that you’re not allowed to rewrite them from scratch. Thus an unwritten bequeathment of responsibility occurs between the code base and you: it now becomes your problem. Its failings become your failings. The insecurity it gives your client now has them project that insecurity, and responsibility to remedy it, onto you.

I’m assuming you’ve already done a code review by this point to ensure you, or your firm, want to even get involved. If you don’t but are still here, it’s probably because they’re paying you a lot of money to wallow in their mess. In that case, welcome to the suck.

The original definition of refactoring, referring to Martin Fowlers here, is making a small, positive change that has low inherent risk. This is done to ensure you don’t break anything. The theory is, over time if you make enough positive changes, you’ll make the whole code base positive.

Life is more complex than that, but the point here is that you use refactoring to affect positive change without breaking the code and looking like an idiot in front of your client and ticking off their customers. You do not make large, sweeping changes to the code base, even if they are needed. Take it slow and steady, 1 little change at a time. Some of the examples below are small (such as using strong-typing in place of Object and *), and some are larger (such as modifying your Model layer) are larger. Sometimes such changes you need help in tackling because it’s sphaghetti code; team refactoring can be invigorating and you get a lot more done, so don’t think you have to do this on your own.

When in doubt, make a small, positive change. If you take more than 3 days to make a change, abort.

Exceptions

Your first priority in refactoring should be handling exceptions or errors. It’s ok if you see how sausage is made in the meat factory, but it’s not ok if your client does. You can turn explosive bedlam into a controlled spectacle. That’s right, things breaking can be made into looking like your in total control. Magic isn’t magic; it’s sleight of hand through distraction.

“Look at this pretty logging window created by our company to handle such situations. All errors are now known to us, when and where they happen. This awesome ability to report said errors amongst your staff, not just QA, but you, your customers… everyone, even while in production! No surprises or fire drills here… ALL at no extra charge.”

But how do we get there? Let’s start at bottom.

A lot of programming languages execute functions or methods in 2 modes: protected or omg-it-might-blow-up-we’re-hardcore mode. JavaScript/CoffeeScript, ActionScript, and Java for example use try/catch. Ruby uses begin & rescue. Python uses try/except. Lua uses pcall. Most languages have various ways to ensure that if code explodes, it’s done so in a controlled fashion. Additionally, some have global exception handling as well.

Exceptions are the first area of uncontrolled chaos that must be contained. No controlled refactoring can take place without tackling these areas first.

Architecture

Architecture comes into play for large code bases, although, using it for small code bases can help you learn (albeit hard to appreciate as it’ll often get in your way of getting r done).

Architecture refers to how an application is built. It’s usually an amalgamation of design patterns used in a common way. For the sake of this article, we’re using architecture in a small sense of the word in helping move parts of the code to be more encapsulated, make things that break easier to identify, test in isolation, that sort of thing. While references to MVC/MVP/MVVM can get really esoteric and subjective, we’re not concerned with that.

What you need to verify with defining “good architecture” for you and your team is how it works, agree on approach, and ensure it helps you test things in isolation. If y’all agree, can be independently productive, and if something breaks you know where… that’s good architecture. Don’t get hung up on the definitions, get hung up on the contract on how things talk to each other, and that your team all agrees on that contract. This’ll come into play later in the next article regarding team synergy.

Strong Typing

Not much to say here. Don’t use Object/*/Dictionary. If you do, wrap it with a strongly typed API or unit tests. Putting the burden on the consumer of those objects leads to lots of code. Lots of code == bad.

Don’t do this:

var person:Object = {};
person.firstName = "Jesse";
person.lastName = "Warden";

Do this:

class PersonVO
{
   public var firstName:String;
   public var lastName:String;
}

var person:PersonVO = new PersonVO();
person.firstName = "Jesse";
person.lastName = "Warden";

When you don’t have strong-typing (JavaScript/CoffeeScript/Lua), #moarUnitTests. For JavaScript use jshint, jslint, and annotations that various frameworks use to infer type like Google’s Closure compiler for great justice.

Refactoring Paths

Beyond the suggestions below + mentioned resources, you have to use your own intuition. Refactorings on their own are good but when done as a team effort in strategic areas, you can make greater headway, earn more trust from your client, and do more “relevant” refactoring.

There are some things you’ll know are right such as investing in a good GUI logger, making friends with IT, etc. Others, on what to fix first, how to support dual architectures in some areas, and how to portray transparency to your client… some of those are just experience. When in doubt, go with your gut. The less code of the choices, the better.

A good refactoring path allows you the ability to code right now, satisfy non-programmers for trust earning, with assurance you haven’t coded yourself into a corner. It’s like a good marriage: compromise.

You’ll lose some battles. The goal is to win the war.

You’ll have detractors such as who don’t have their Code License; knowing the rules before breaking them… or some who are just afraid. Some are ivory tower zealots who aren’t punished for getting nothing done (some are rewarded). Some have emotional scars from attempting to fix things, and are hesitant to do what’s right. This is usually when they know the value but cannot effectively articulate it to stakeholders (this is called a “software developer”). Some are just insecure and don’t believe in their own refactoring ability and thus hold you back as well (low D on DISC). Some think you’re full of it.

Refactoring is temporal. You are creating a positive change to last for awhile… but not forever. It’s ok to refactor something, then refactor it again later as long as you’re actually making an improvement upon a problem. If a ship is taking on water, the first thing you do is plug the holes with wooden spikes. Later, when the French stop firing at you and your hangover is gone from drinking all the wine you found on their now apprehended ship, you can plug the holes proper with a leather & tar mixture. Then once in dry dock, replace the planks.

Architecture is not temporal… but can be as well. You’re supposed to establish rules and guidelines on how you approach a code base. Sometimes, however, you can jury-rig a solution to hit some deadline, or get a build out the door for some managerial milestone. Later, you can re-assess. Sometimes, you’re initial hack is controlled; meaning you are writing throwaway code on purpose. In doing so, you can make it easier to fix later.

Architecture is generally decided upon by the team, and you move forward for the length of the project on it.

Refactoring is a strategy that can change daily.

Step #1: Exceptions

Case: Service layer has 3 failure points: entry, error, and exit parsing.
Solution: Validate post data, log error, wrap parsing/factory/decoding.
Language: JavaScript using AmplifyJS

Bad Code:

amplify.request.decoders.appEnvelope =
    function ( data, status, xhr, success, error )
    {
        if ( data.status === "success" )
        {
            success( data.data );
        } else if ( data.status === "fail" || data.status === "error" )
        {
            error( data.message, data.status );
        } else {
            error( data.message , "fatal" );
        }
    };

amplify.request.define( "decoderExample", "ajax", {
    url: "http://server.com/service/doSomething/",
    type: "POST",
    decoder: "appEnvelope"
});

amplify.request({
    resourceId: "decoderExample",
    success: function( data )
    {
        data.foo; // bar
    },
    error: function( message, level )
    {
        alert( "always handle errors with alerts." );
    }
});

Refactored Code:

/*
Our decoder function is now:
1. shielded from bad data
2. assumes the response not well formed from the get go
3. logs each parsing error separately for easier diagnosis
*/

// modified appEnvelope decoder function
function ( data, status, xhr, success, error )
{
    try
    {
        if(data == null || data == "")
        {
            error("Decoding error in parsing data.");
        }
        else if(data.status == null || data.status != "")
        {
            error("Unknown status.");
        }
        else if(data.message == null || data.message != "")
        {
            error("Uknown message.");
        }
        else if ( data.status === "success" )
        {
            success( data.data );
        }
        else if ( data.status === "fail" || data.status === "error" )
        {
            error( data.message, data.status );
        }
        else
        {
            error( data.message , "fatal" );
        }
    }
    catch(e)
    {
       error("Decoding error.");
    }
};

/*
Notice we:
1. Validate our requestData (you could do more than a null check) before making an actual service call.
2. Additionally, we log the failed request with the Service name and reason for easier diagnosis in a larger application.
*/

if(requestData != null)
{
    amplify.request({
        resourceId: "decoderExample",,
        data: requestData,
        success: function( data )
        {
            data.foo; // bar
        },
        /*
        Our request error handling now has:
        1. a log, vs. an alert, for logging our error
        2. a common logging signature for easier diagnosis in a larger application
        */
        error: function( message, level )
        {
            console.error("MyService::error, message: " + message + ", level: " + level);
        }
    });
}
else
{
    console.error("MyService::error, requestData is null.");
}

Case: Null values in GUI. View/GUI must assume data is not always good; must handle null without exploding.
Solution: Check for null using if/then.
Language: ActionScript 3

Bad Code:

myTextField.text = person.name;

Refactored Code:

/*
We now:
1. check for a null VO first
2. if it is in fact null, we have a visual, conventional way of displaying null values w/o exploding.
3. if a property of the VO is null, that'll display as normal (in this case as an empty String)
*/

if(person)
{
   myTextField.text = person.name;
}
else
{
   myTextField.text = "???";
}

Case: Global Exception Handling
Solution: Add global handler that logs errors away from users to see.
Language: ActionScript and JavaScript

Refactored ActionScript Code:

package
{
   import flash.display.Sprite;
   import flash.events.Event;
   import flash.events.UncaughtErrorEvent;

   public class ErrorHandling extends Sprite
   {
      public function ErrorHandling()
      {
         super();
         addEventListener(Event.ADDED_TO_STAGE, onAdded);
      }

      private function onAdded(event:Event):void
      {
         loaderInfo.uncaughtErrorEvents.addEventListener(UncaughtErrorEvent.UNCAUGHT_ERROR, onUncaughtError);
      }

      private function onUncaughtError(event:UncaughtErrorEvent):void
      {
         trace("Uncaught Error: " + event);
         try
         {
            // note, fails in Asyncronous errors
            throw new Error("stack trace creator");
         }
         catch(error:Error)
         {
            trace(error.getStackTrace());
         }
      }
   }
}

Refactored JavaScript Code:

// via http://stackoverflow.com/questions/951791/javascript-global-error-handling
//
// more techniques here for Opera/Safari older versions
// http://stackoverflow.com/questions/645840/mimic-window-onerror-in-opera-using-javascript
//
// also note various browsers pass different arguments, and some suggest
// handling existing handlers if they exist
// https://developer.mozilla.org/en/DOM/window.onerror
//
// also note that the error in some browsers will still occur unless you handle it
// a certain way; here we're returning true from the error handler to do so.

window.onerror = function()
{
    if(arguments && arguments.length && arguments.length > 0)
    {
        var logStr = "";
        var len = arguments.length;
        for(var index = 0; index < len; index++)
        {
            logStr += "\t" + arguments[index] + "\n";
        }
        console.error("Global Error:\n" + logStr);
    }
    else
    {
        console.log("Global Error");
    }
    return true;
};

Step #2: Architecture

Case: Encapsulation of statefull, Model data.
Solution: Encapsulate Setting of Model Data to single place (DRY)
Language: Lua in Corona SDK

Bad Code:

_G.score = 0

local scoreText = display.newText()
function updateScoreText()
   scoreText.text = _G.score
end
updateScoreText()

local powerUp = display.newImage("powerup.png")
powerUp.name = "powerUp"

local enemy = display.newImage("enemy.png")
enemy.name = "enemy"
function enemy:collision(event)
   if event.phase == "began" then
      if event.target.name == "bullet" then
         _G.score = _G.score + 1
      end
   end
end
enemy:addEventListener("collision", enemy)

local ship = display.newImage("ship.png")
function ship:collision(event)
   if event.phase == "began" then
      if event.target.name == "powerUp" then
         _G.score = _G.score + 1
         updateScoreText()
         return true
      end
   end
end
ship:addEventListener("collision", ship)

function onTouch(event)
   if event.phase == "began" then
      local bullet = display.newImage("bullet.png")
      bullet.name = "bullet"
      return true
   end
end

Runtime:addEventListener("touch", onTouch)

Refactored Code Round 1:

--[[
Notice we've added an addScore method to:
1. have a central place everyone can update the model data
2. make it easier to identify WHO is calling this add function
3. ensure we never forget to update our Text Field with the latest value like in the original code
]]--

function addScore(amount)
   _G.score = _G.score + amount
   updateScoreText()
end

-- Fixing our enemy collision handler to now call our encapsulated method
function enemy:collision(event)
   if event.phase == "began" then
      if event.target.name == "bullet" then
         addScore(1)
      end
   end
end

Refactored Code Round 2:

--[[
Now we use a more generalized Observer pattern, and dispatch an event
when the value changes. The system is free to listen to this event
and react however it needs to. This allows the GUI to change w/o worrying
updating the model changing behavior to compensate for a changing GUI API.
]]--

function addScore(amount)
   _G.score = _G.score + amount
   Runtime:dispatchEvent({name="onScoreChanged", newScore=_G.score})
end

-- we now can update more GUI parts as they become needed
function onScoreUpdated(event)
   updateScoreText(event.newScore)
end
Runtime:addEventListener("onScoreChanged", onScoreUpdated)

-- notice this includes encapsulated GUI controls as well;
-- they can just listen in on the Event Bus.
-- Here, we have a Level Progress bar that listens to the score change
-- to visually indicate how far along the user is until they level up.
require "com.company.project.controls.ProgressBar"

LevelUpProgressBar = {}

function LevelUpProgressBar:new()

   local bar = ProgressBar:new({width = 100, height = 60})
   bar.levelMax = 1

   function bar:onScoreChanged(event)
      self:setProgress(event.newScore, self.levelMax)
   end

   function bar:onLevelMaxChanged(event)
      self.levelMax = event.newLevelMax
   end

   Runtime:addEventListener("onLevelMaxChanged", bar)
   Runtime:addEventListener("onScoreChanged", bar)

end

return LevelUpProgressBar

Case: Access Control for Statefull Model Data Using Application Logic (English: you need to update some model data, and multiple parts in your application need to do this). aka, removing global variables.
Solution: Utilize Proxy Pattern or Command Pattern for single access (DRY), this ensures there is only 1 way to update your model data, inputs are validated. This makes it easier to debug who is calling/invoking it, and what data they are passing, as well as ensuring those who need to know about the change are informed.
Language: ActionScript

Bad Code:

var firstName:String             = firstNameTextInput.text;
var lastName:String              = lastNameTextInput.text;
var birthdate:Date               = birthdateControl.value;
var myPersonVO:PersonVO          = new PersonVO(firstName, lastName, birthdate);
MyStaticModel.person             = myPersonVO;
MyStaticModel.person.lastName    = "McTaggart"

Refactored Code:

// First we create a factory class to test our creation & validation in isolation.
// This is also easier to write unit tests for. It's also DRY in that all classes
// who need to create PersonVO's and validate them are using the same code to do so.

class PersonFactory
{
   public static function getPerson(firstName:String, lastName:String, birthdate:Date):PersonVO
   {
      if(firstName == null || firstName == "")
      {
         throw new Error("firstName cannot be null nor an empty String.");
         return;
      }

      if(lastName == null || lastName == "")
      {
         throw new Error("lastName cannot be null nor an empty String.");
         return;
      }

      if(birthdate == null)
      {
         throw new Date("birthdate cannot be null.");
      }

      return new PersonVO(firstName, lastName, birthdate);
   }

   public static const validateLastName(lastName:String):Boolean
   {
      if(lastName != null && lastName != "")
      {
         return true;
      }
      else
      {
         return false;
      }
   }
}

// Second step, create a change event that those who need to know about a changing
// personVO in the model can be made aware of.

class PersonModelEvent extends Event
{

   public static const PERSON_CHANGED:String = "personChanged";

   public function PersonModelEvent(type:String)
   {
      super(type, false, true);
   }

}

/*
Third, create a Model class Singleton that:
- dispatches change events when it's internal person variable is changed, or modified
through accessor controls
- validates creation & changes
NOTE: If classes need to operate on PersonVO itself, create additional methods to
operate on the internal PersonVO to ensure proper change events are dispatched.
NOTE: For those who wish to remove Singletons, create as an instance, and inject the dependencies,
whether via a DI framework such as SwiftSuspenders/Guice/Spring, or manually via
getter/setters for those classes who need instance access.
*/

class PersonModel extends EventDispatcher
{

   private var _person:PersonVO;

   public function get person():PersonVO { return _person; }
   public function set person(value:PersonVO):void
   {
      _person = value;
      dispatchEvent(new PersonModelEvent(PersonModelEvent.PERSON_CHANGED));
   }

   public function PersonModel()
   {
   }

   public function setNewPerson(firstName:String, lastName:String, birthdate:Date):void
   {
      // first, validate data is good, don't want to pollute our model
      var personVO:PersonVO;
      try
      {
         personVO = PersonFactory.getPerson(firstName, lastName, birthdate);
         person = personVO;
      }
      catch(error:Error)
      {
         trace("PersonModel::setNewPerson error: " + error);
      }
   }

   public function updateLastName(lastName:String):void
   {
      if(person)
      {
         if(PersonFactory.validateLastName(lastName) == true)
         {
            person.lastName = lastName;
            dispatchEvent(new PersonModelEvent(PersonModelEvent.PERSON_CHANGED));
         }
         else
         {
            trace("PersonModel::updateLastName, failed because the lastName '" + lastName + "' is invalid.");
         }
      }
      else
      {
         trace("PersonModel::updateLastName, failed because the current person is null.");
      }
   }

   private static var _inst:PersonModel;

   public static function get instance():PersonModel
   {
      if(_inst == null)
         _inst = new PersonModel();

      return _inst;
   }
}

Refactored Code 2:

/*
In the case of need of many needing to affect the Model, and there is no
desire to all Models/Proxies to have public methods accessible by others,
you can use the Command pattern. Additionally, this solves the use case
where multiple Models are needed to be accessed and/or modified. Finally,
we also handle asynchronous use cases where you need to update a Model(s)
based on a server response. Notice how each step is validated, and a log is
produced if any step fails. You need to know WHERE it failed and WHY.
Also, no rollback support in the following.
*/

class CreatePersonEvent extends Event
{
   public static const CREATE_PERSON:String = "createPerson";

   public var firstName:String;
   public var lastName:String;
   public var birthdate:Date;
   public var personModel:PersonModel;
   public var organizationModel:OrganizationModel;

   public function CreatePersonEvent()
   {
      super(CREATE_PERSON);
   }
}

class CreatePersonCommand extends AsynCommand
{

   // injected dependencies; you could use a DI framework,
   // wrap Command class creation with these, or simply pass
   // in through the Event payload like the below shows.
   private var personModel:PersonModel;
   private var organizationModel:PersonModel;
   private var createPersonService:RESTFullCreatePersonService;
   private var personVOToCreate:PersonVO;

   // Create a PersonVO, set on the PersonModel,
   // and ensure the OrganizationModel is updated with the
   // new person once the server has verified it's creation.
   public function execute(event:CreatePersonEvent):void
   {
      try
      {
         personModel = event.personModel;
         organizationModel = event.organizationModel;
         personVOToCreate = PersonFactory.getPerson(firstName, lastName, birthdate);
         if(personVOToCreate != null)
         {
            createPerson()
         }
         else
         {
            throw new Error("ERROR: CreatePersonCommand::exeucte, PersonFactory returned null.");
         }
      }
      catch(error:Error)
      {
         trace("ERROR: CreatePersonCommand::execute, error: " + error);
         finish();
      }
   }

   private function createPerson():void
   {
      createPersonService = new RESTFullCreatePersonService();
      createPersonService.addEventListener("success", onCreatePersonSuccess);
      createPersonService.addEventListener("error", onCreatePersonError);
      createPersonService.createPerson(personVOToCreate);
   }

   private function onCreatePersonError(event:Event):void
   {
      trace("ERROR: CreatePersonCommand::onCreatePersonError, event: " + event);
      finish();
   }

   private function onCreatePersonSuccess(event:Event):void
   {
      // server has successfully created PersonVO, let's update ours with what
      // the server sent back to ensure we now have a proper PersonVO.ID set
      // by the server
      if(createPersonService.savedPersonVO != null)
      {
         personModel.person = createPersonService.savedPersonVO;
         organizationModel.addNewPerson(personModel.person);
      }
      else
      {
         trace("ERROR: CreatePersonCommand::onCreatePersonSuccess failed, server sent back a null savedPersonVO.");
      }
      finish();
   }

}

Step #3: Strong Typing

Case: Using loose typing/variants/Objects/*, resulting in loss of compiler time checking and thus errors that only appear at runtime.
Solution: Use strong-typing.
Language: ActionScript

Bad Code:

/*
Notice:
- personVO.lstName is a mispelling, but no error will be raised by the compiler.
- no error will be raised by the updatePerson function as Object is dynamic,
and lastName will be created and set on the personVO Object.
- no error will be raised if you pass something other than a personVO to updatePerson
except for runtime errors
*/

var personVO:Object = {};
personVO.firstName = "Jesse";
personVO.lstName = "Warden";

function updatePerson(person:*, newNameFromStringBlock:String):void
{
   var nameArray:Array = newNameFromStringBlock.split(" ");
   person.firstName = nameArray[0];
   person.lastName = nameArray[1];
}

Refactored Code:

/*
Notice:
- our "lstName" will be caught by the compiler
- our updatePerson function ensures only a PersonVO is accepted to be operated on. If not,
our compiler will let us know.
*/

class PersonVO
{
   public var firstName:String;
   public var lastName:String;
}

var personVO:PersonVO = new PersonVO();
personVO.firstName = "Jesse";
personVO.lstName = "Warden";

function updatePerson(person:PersonVO, newNameFromStringBlock:String):void
{
   var nameArray:Array = newNameFromStringBlock.split(" ");
   person.firstName = nameArray[0];
   person.lastName = nameArray[1];
}

Case: Service layer has no input validation, is hard to test, and has no error reporting. Burden is on class user to wrap in try/catch as well as parse server response including knowing business logic. Finally, asynchronous loader is not a class member variable, thus potential memory leak + harder to debug.
Solution: Create a Service class that is easy to use, validates inputs, and logs errors. Encapsulates business logic.
Langauge: ActionScript

Bad Code:

class CreatePersonService extends EventDispatcher
{

   public var loader:URLLoader;

   public function CreatePersonService()
   {
      super();
   }

   public function createPerson(person:PersonVO):void
   {
      var loader = new URLLoader();
      loader.addEventListener(Event.COMPLETE, onComplete);
      var req:URLRequest = new URLRequest("http://someserver.com/restful/api/");
      req.data = person.toJSON();
      loader.load(req);
   }

   private function onComplete(event:Event):void
   {
      dispatchEvent(new Event("success"));
   }
}

var service:CreatePersonService = new CreatePersonService();
service.addEventListener("success", onSuccess);

function onSuccess(event:Event):void
{
   var json:Object = JSON.decode(service.loader.data);
   var person:PersonVO = new PersonVO(json.firstName, json.lastName, json.birthdate);
}

Refactored Code:

/*
Note:
- createPerson validates you gave it a valid PersonVO.
- all errors within class are opt-in, no throw's. User has to register event listener to hear; #dontCreateGrenades
- class assumes single-concurrency if possible, kills previous load call before proceeding
- listens for all errors, and responds with 1 error with more detail logged if needed; simplifies API
- wraps actual call to verify SecurityError isn't thrown and if it is, it's logged w/ opt-in error
- uses united tested Factory for DRY parsing and assumes
- assumes Factory can fail, and reports it as such
- result in the form of a strongly-typed ValueObject in a read-only property to ensure proper API usage
*/
class CreatePersonService extends EventDispatcher
{

   private var _savedPersonVO:PersonVO;
   private var loader:URLLoader;

   public function get savedPersonVO():PersonVO { return _savedPerson; }

   public function CreatePersonService()
   {
      super();
   }

   public function createPerson(person:PersonVO):void
   {
      _savedPersonVO = null;

      if(person == null)
      {
         onError("You cannot pass in a null person to create.");
         return;
      }

      if(loader == null)
      {
         loader = new URLLoader();
         loader.addEventListener(IOErrorEvent.IO_ERROR, onIOError);
         loader.addEventListener(SecurityErrorEvent.SECURITY_ERROR, onSecurityError);
         loader.addEventListener(Event.COMPLETE, onComplete);
      }
      else
      {
         try
         {
            loader.close();
         }
         catch(error:Error)
         {
            // only exception that is ok to swallow
         }
      }

      try
      {
         var req:URLRequest = new URLRequest("http://someserver.com/restful/api/");
         req.data = person.toJSON();
         loader.load(req);
      }
      catch(error:Error)
      {
         onError("CreatePersonService::createPerson, error: " + error);
      }
   }

   private function onIOError(event:IOErrorEvent):void
   {
      onError(event.text);
   }

   private function onSecurityError(event:SecurityErrorEvent):void
   {
      onError(event.text);
   }

   private function onComplete(event:Event):void
   {
      try
      {
         _savedPersonVO = PersonFactory.parsePersonFromJSON(loader.data);
         if(_savedPersonVO != null)
         {
            dispatchEvent(new Event("success"));
         }
         else
         {
            onError("CreatePersonService::onComplete, factory returned a null PersonVO.");
         }
      }
      catch(error:Error)
      {
         onError("CreatePersonService::onComplete, parsing error: " + error);
      }
   }

   private function onError(errorString:String):void
   {
      trace("ERROR: CreatePersonService::onError: " + errorString);
      dispatchEvent(new Event("error"));
   }
}

Step #4: Fine Tuning

If you made it this far, and have managed to continue to maintain the above, congratulations! This stuff is icing on the cake. You can continue onto smaller scope refactorings, as well as continuing to increase your unit test coverage.

Final Tips

1. When casting, ensure result is not null before using. If null, log the casting failure.
2. For Flash Player, set ExternalInterface.marshallExceptions = true if using ExternalInterface
3. Log all errors
4. Code Review. Often. Weekly. Whatever, pick a schedule.
5. Object / * / Dictionary == if you can’t avoid, wrap with unit tests
6. Remember: small, low-risk changes.
7. TODO was invented for a reason.
8. If you haven’t committed to source control for 3 full days of working, abort and just check into a branch for later revisiting/reference.

Conclusions

Remember, the most important refactoring you can possibly do is reading the code base for 1 hour a day, no more. Just 1 person. This is has more positive impact than good unit test coverage. Daily code reviews, even in isolation, are extremely important. The earlier in the project life cycle, the better.

From that, you can make small, low risk changes. Keep track with TODO’s. Breathe, Rome wasn’t fixed in a day, and a pile of rubbish wasn’t fixed in one either.

For further, positive refactorings you can check an older list by Martin Fowler, and this book has a ton of wonderful ones + tests as well. While a Code Review has decreasing value the more people you add in terms of defects found, the ability to learn & make positive suggestions for architecture are wonderful. If you’re alone, use codereview.stackexchange.com.

In the next article I’ll show you how to get help with the above… and permission.