For today's blog entry, I decided to write a shoutout to my favorite programming book. "Refactoring" by Martin Fowler fundamentally altered the way I program. To quote his summary of the technique of refactoring:
"Refactoring is a controlled technique for improving the design of an existing code base. Its essence is applying a series of small behavior-preserving transformations, each of which "too small to be worth doing". However the cumulative effect of each of these transformations is quite significant. By doing them in small steps you reduce the risk of introducing errors. You also avoid having the system broken while you are carrying out the restructuring - which allows you to gradually refactor a system over an extended period of time."
How Do I Refactor?
I use refactoring as a daily aspect of my development practice. It manifests in a number of ways:
Capturing Meaning
Sometimes I investigate or debug some code (in my engine that I wrote) that doesn't make immediate sense to me. Once I figure out what's going on, I immediately find a way to instill that knowledge into the code to make it less confusing. That can be extracting a function, renaming a variable, or adding a comment. I do several of these sorts of checkins every day, and my code becomes steadily more readable as a result.
Make Callsites Understandable
If a function call doesn't immediately communicate what it will do, I refactor the function to improve that. One of my favorite versions of that is replacing lists of default parameters with a single default parameter struct. This gives you the convenience of multiple optional parameters, without the confusion of trying to order them, and remember what they mean.
struct RenderParams
{
Vec2 scale = Vec2(1.0f, 1.0f);
float rotation = 0.0f;
float* uvOverride[4] = { nullptr };
bool isHud = false;
Color tint = Color(255, 255, 255);
float tintStrength = 0.0f;
float alpha = 1.0f;
};
void DrawRect(const Rect& rect, const Vec2& paletteCoord, const RenderParams& params = kDefaultParams);Simple Commits
I try to make every commit simple and clean. If every commit accomplishes one simple goal, it's easier to look back at the diffs to confirm that I did them correctly. And its easier to identify where bugs came from, as I can test each step of the full change.
For a large new feature, I typically do 3-5 non-functional commits to adjust the structure of existing code. Then I do 1 commit that actually implements the feature. Finally, I do 1-3 more non-functional commits to get the code into a state I'm happy with before I move on.
I recently decided to extract some logic from my overall InputStateMachine into a separate InteractingStateMachine (for interacting with machines). Rather than one giant commit that moved a bunch of code between these two classes, I broke it into several smaller ones.
Create InteractingStateMachine.
Create the Interacting states, triggered from the InputStateMachine.
Move the game logic from Input to Interacting.
Delete the Input states.
Actually add the new Interaction states that the feature required.
Ergonomic Refactors
I freely and happily rename classes for my coding ergonomics. Growing tired of typing characters that don't contribute information to me, I renamed Component to Comp, Definition to Def, Vector2 to Vec2, StringHash to Hash, Position to Pos, Velocity to Vel.
An Example
At a previous company, I was fixing a bug in the interface between our scripting language and the C++ engine. I found myself struggling to wrap my head around the way values were extracted from the script. The callsite looked something like this:
void TeleportUnit(ScriptValues& values)
{
ScriptParamHelper Params(values);
Unit* unitToTeleport = Params("Unit");
Vector3 targetPosition = Params("Target", Vector3::kZero, false, true);
bool checkCollision = Params("CheckCollision", true, true, false);
if (!Params.HasError())
{
// ...
}
}What's going on with checkCollision? That's a lot of booleans that apparently might not be necessary, judging by Params("Unit") above it. These ScriptParamHelper instances were used in literally thousands of places in the code. Every time C++ and script interfaced, this struct was doing some magic with some booleans (maybe defaulted, maybe set) dangling off the end of the call.
It turns out, the last boolean was called "explicit", and its purpose was lost to the sands of time. No compiled code actually read it. A mildly complicated regex operation chopped that variable out of all callsites, and I was able to delete it forever. (That code review got cheers from my teammates)
The second to last boolean was "required". If it was true, but the script didn't specify the value, HasError() would return true. That's pretty understandable, but not clear from looking at the callsite. The parameter before it is an optional default value, in case required was set to false. But if required was true (or omitted, in which case it defaulted to true), this didn't do anything.
Thus, I refactored (using some more regexes) to the following:
void TeleportUnit(ScriptValues& values)
{
ScriptParamHelper Params(values);
Unit* unitToTeleport = Params.Required("Unit");
Vector3 targetPosition = Params.Optional("Target", Vector3::kZero);
bool checkCollision = Params.Required("CheckCollision");
if (!Params.HasError())
{
// ...
}
}No optional parameters. No unused parameters. The option of type-checking the optional value against the return type. Function names that tell you what's actually going on. Beautiful.
Conclusion
Refactoring is my favorite method of programming. By breaking changes into little pieces that each are obviously correct, I gain confidence that I'm not breaking anything. And it makes the actual functional changes easy to understand, since they're not mixed with stylistic/structural changes.
Do you use refactoring in your development? What books have shaped the way you work? Join the discussion on Reddit.
Comments