Macadamian Blog

A Patch A Day Keeps The Rework Away

I met with Ovidiu this week, the new developer hired in our Romanian office, to go over the material of first week training. Once you start development, I said, we have a requirement that you must submit "1 patch a day". Silence on the other end of the phone, a not uncommon response from any new team member, local or global. Ovidiu speaks English very well, but in this case it was like I was speaking a different language. "Let me explain what it's all about - you've heard of test-driven development, you've heard of pair programming. Well patch-a-day may just be an even stronger development process."

What is a Patch?

A patch is a portable file which contains new code that a developer wants to add to source control. It might be one new function. It might be a bug fix. It might be a skeleton of a new class. The rule is that it should be small and self-contained.

A developer works on this code on his local machine. When he is ready to submit the code to source control, he generates a .patch file, a text-based file that lists the lines of code that were added, deleted and modified. The patch file can be used to revert or re-apply the changes on the developer's machine. The patch file is portable because the developer can give the file to other developers on the team and they can apply the patch to their local copy of the code.

Index: comp/ajax/signup_user_prepare.php
==============================================
@@ -14,18 +14,48 @@
if (isset($_REQUEST['vl_id'])){

- $sm->assign("vl_id", $_REQUEST['vl_id']);
- $sm->assign("ta", $_REQUEST['ta']);
- //$content = $sm->fetch("signup_ajax.htm");
-
- $content = $sm->fetch("signup_ajax_player.htm");
+ $sm->assign("vl_id", $_REQUEST['vl_id']);
+ $sm->assign("ta", $_REQUEST['ta']);
+
+ $content = $sm->fetch("signup_ajax_player.htm");

Many source control systems (CVS, Subversion, etc.) have an option to generate patch files. Others can be adapted to generate these files or similar.

Why Use Patches?

Why even use patches? Why not commit the code directly to source control?

In Macadamian's process, the source control must always be 100% functional. A customer request for a working version of the code, a tester's request for a last minute bug fix, a request to release an application update could come at any moment. Team members must always have access to the latest copy of working code. Nightly automated unit tests must all pass on the integration server. If the utmost quality in the source control is not preserved, then it becomes a bottleneck.

The second reason to use patches is that they facilitate code review. The process works like this:

1. Developer sends an e-mail to the project mailing list, containing:
   
   • the patch file attached
   • a brief explanation of the changes
   • the name of one team member who will peer review the code
2. The reviewer and and developer then discuss the code publicly on the mailing list, refining it as necessary
3. Other team members, the architect, the manager, etc. are free to comment or do ad-hoc reviews on the code
4. When all are in agreement, the patch is committed to source control

The patch file is easily readable by the readers of the mailing list, either in plaintext format or by using special viewers like WinMerge to do highlighting. Having the patches attached and sent out gives the team no excuses not to open up the patch and do an ad-hoc review.

Why Every Day?

Submitting a small patch every day has several advantages:

• small patches are easy to understand, so you'll increase the changes of getting a quality code review (if you dump a huge amount of code, the reviewer may miss things or worse, skip the review altogether)
• frequent patches mean that problems will be caught early, before the developer goes too far in the wrong direction. This is especially important when your team is made up of team members in different time zones.

Every Day, Are You Sure?

What if my function is not finished within a day? What if my class is not completely implemented? I can't submit a patch unless my code is pristine, right?

Yes you can, you must, and in fact, it's easy using the power of stubs and todo's.

A stub is a class or function which has little or no implementation. In order to produce a patch a day, when you start a new class, your first patch is probably going to be a stub:

class LoginDialog
{
bool validateUsername()
{
// TODO: validate the user name length, valid characters, etc.
return true;
}

bool validatePassword()
{
// TODO: validate the password in the database
return true;
}

void doLogin()
{
// TODO: 1. pop up a login dialog
// TODO: 2. get and validate the user credentials
// TODO: 3. login to system
}

string username;
string password;
}

With a stub, your first patch really just outlines the design of your class. You submit this code for review - it compiles, it will not hurt the source control, and it's small enough for the reviewer to understand easily. Maybe you need a function to get the database instance? Maybe it's a security risk to store a password variable? The reviewer is not concerned about implementation, he is concerned about reviewing the design, and catching problems early.

The next patches will replace the TODO's. Perhaps the next patch implements the login dialog:

class LoginDialog
{
bool validateUsername()
{
// TODO: validate the user name length, valid characters, etc.
return true;
}

bool validatePassword()
{
// TODO: validate the password in the database
return true;
}

void doLogin()
{
LoginDialog dlg = new LoginDialog();
dlg.ShowModal();

if( validateUsername(dlg.GetUser()) && validatePassword(dlg.GetPwd()) )
{
// TODO: login to system
}
}

string username;
string password;
}

This time, the code reviewer notices a flaw: what if username or password validation fails? Where's the error handling?

Instead of going back and implementing the error handling, another TODO can be added before committing to source control:

if( validateUsername(dlg.GetUser()) && validatePassword(dlg.GetPwd()) )
{
// TODO: login to system
}
else
{
// TODO: handle login error
}

The code still works and is error free, although right now all it does is bring up a dialog with no functionality. Perhaps the next patch implements the password validation and login. Perhaps later on, another patch will validate the username and even later on error handling is added, etc.

What Do I Implement Next?

The use of patches, stubs and TODO's means that your developers have to think ahead about what they're going to implement. Which is a good thing. Too often we focus on the details of an algorithm and start implementing full speed ahead, without knowing exactly what direction we're going in. Working with patches means getting your team to do a little self-planning. Like in chess, every team member needs to think a few moves ahead: they should always know what the next 2 patches will be.

Working with TODO's allows you to prioritize your work. Let's say you need a rough demo by Friday - instead of implementing the whole Login dialog, implement only the essential code for the demo, and leave things like validation and error handling for the next iteration. Consistently using the same keyword "TODO" means you can always search through the code and get a feeling for how much code is left to be written, what error handling and special cases are not yet handled, etc. Getting a TODO count gives your lead a good idea of how close to completing the project.

What about once I submit my patch and am waiting for review. How do I continue working on the code? The nice thing about having patches as files is that they can be created and code reviewed in parallel. In the above example, I might write a patch for validatePassword() function, and another for validateUsername() function while I am waiting for the first to be reviewed. I might submit patches for another class I'm working on besides Login.


Bottom line: the process encourages good design, planning and decomposition of tasks from everyone, not just the project leader or manager. Once team members get good at planning ahead and breaking down their work, they might have 4-5 patches being reviewed on the mailing list all at once.

Patch-a-day is not part of a waterfall or agile methodology necessarily - it is a development process that could be implemented in any methodology to:
* improve organization of development team's work
* facilitate fast, thorough and frequent code review
* encourage structured design, decomposition of classes and functions
* prioritize implementation of critical functionality
* track remaining work including error handling and special cases

For more information, drop me a line. It'll be worth your time.