Coding for easier and quicker code reviews

If your company doesn't do code reviews, you most certainly should. If you're bound and determined to ship crappy software by not doing reviews, this post won't do much for you.

Code is meant to be read

Many people are under the mistaken impression that their code is meant for the computer's consumption. While it's true that a computer will consume your code, it is not who you're writing for. Programmers are your intended audience. That includes you. Coding a new project is the easy and short part of the software's lifecycle compared to the time code spends in the maintenance and upkeep phase.

So, assuming that you want to write code that's easier to read as well as easier to review, here's a few tips. All of them come from dealing with real coworkers. If you've worked with me and recognize something that you still do, you should feel bad. You're probably the reason I wrote this.

Descriptive variable names

Yeah, that's just good software development practice, but it can really help during code reviews as well. A short variable name that doesn't describe what it does is much harder to figure out during a code review since most review tools (at least that I've used) don't give a huge amount of context around the change. So while someone going in to edit the code might quickly figure out what the variable is supposed to contain, a reviewer has to dig.

Exceptions: Obviously loop control variables should be short ($i, $j, $k) as well as variables that use a single letter commonly, like representing coordinates ($x, $y, $z).

Useless commit messages

"Fixed a bug" or "Made change suggested in review #1234" are nearly useless. While they may be factually correct, they don't really tell the review what and why you're changing something. In the first case, the reviewer has to figure out what the bug was before they can decide that you've fixed it (and fixed it in the best way). In the second case, they have to go back to the original review to figure out what the suggested change was (and whether it was a good idea in the first place) before they can determine whether you'd fix the original problem.

Exceptions: If you've got a code sniffer (PHPCS, for example) that complains about things, having a commit message of "PHPCS" is plenty clear that you're appeasing PHPCS.

Trailing commas

This one is a bit controversial, especially if you frequently switch between PHP and javascript. PHP allows trailing commas for arrays while certain paste-eating browsers don't allow it in their javascript parsers. Creating an array with the trailing comma makes later reviews easy to read. For example, if you have an array like:

    $letters = array(
        'a',
        'b',
        'c'
    );

and you need to add a new letter:

    $letters = array(
        'a',
        'b',
        'c',
        'd'
    );

code review packages will show two changed lines:

    $letters = array(
        'a',
        'b',
        'c',
        'd'
    );

If you always include the trailing comma it makes it obvious during the review that you're only intending to add a new element. This is a trivial example, but it helps to show your change's intent better, which helps a reviewer better understand what you're doing.

Code reviews are meant to help you

Don't get your feelings hurt when someone points out mistakes that you've made. Everyone is trying to get better. If you send shorter review requests that are easy to read, it's much more likely that you'll get responses quickly and with less confusion.

Unit testing Google Closure applications from the command line

I've been playing around with building an application using Google Closure. I tried searching for a good way to run the unit tests from the command line as part of an Ant build, but either there wasn't anything out there to do what I wanted to do or I just couldn't find it.

As an aside, naming a language after a programming construct is really dumb. If you try searching for blog posts on Google Closure, you get a bunch of stuff about javascript and closures since they're such an important language concept. I think Prototype might have been a more successful language if they hadn't named themselves after a language construct as well, though the framework itself has its issues.

I'm used to writing code using test driven development, which doesn't work particularly well writing some Javascript. DOM-related code in particular is difficult to write with TDD. But there are parts of Javascript applications that can and should be well unit tested. When coding in PHP or Python, I normally have two terminal windows open for my Vim sessions and one for my build and source code management activities. I'll typically save the file I'm working on and them immediately alt-tab to the build window and hit up then enter to run my unit test target. I really wanted to do that with Javascript.

At my job we use jQuery. This comes with Qunit which is easy to run automagically with PhantomJS. But Qunit didn't look like it would be easy to make work with Closure in that it wouldn't handle the dependencies for me. And I might as well use the unit test framework for the library that I'm working with. I had a few goals. I didn't want my Javascript tests to fire up a browser. That kind of thing should be handled by Selenium. I wanted to be able to add new tests without having to manually add the test name to any file. I wanted the output to be at least somewhat pretty and clean, particularly if there were no failures. As a PHP developer I'm used to the PHPUnit output:

I decided to try to code up my own test runner to make this work. Here's the first version:


First, the build target concatenates all of the test files. In my case, test files are in a directory called tests and each of them ends with Test.js.

Next, it fires up the Closure Compiler. In this example, we've got a Javascript file called foo.js. We compile that with the concatenated test file (tests-concat.js) and the test runner (we'll get to that soon). The compiler creates a file called tests.js. We run that with phantomjs. The build will look something like this:

Most of the magic happens in testRunner.js:

Basically how it works is to override some methods in goog.testing.TestCase to capture the results. We don't particularly care about successes, so they're replaced by dots. Failures show as an F and any errors are explained in more detail after all tests finish.

I've uploaded all of the example code to Github at https://github.com/omnicolor/Closure-CLI-test-runner.

Consistency is the key

http://www.flickr.com/photos/richard-g/3549285383/

In my last post, Keeping it simple, I wrote about a few things that can make you a better coder, or at least a more valuable member of a coding team. This is the next step down the path of coding nerdvana.



Style isn't just for the stylish


Every coder has their own preferred style. Left to our own devices, we tend to write code our own way. As long as you're the only one looking at the code, this isn't a problem, but consistency should still be valued.

If you're part of a team, you hopefully have a well documented style guide that everyone follows. Hopefully it covers the gritty details so that developers don't get into fist fights with each other about style differences. The last thing that a growing code base needs is for you to be able to tell who wrote a piece of code without checking your source control's blame log.

But what if it doesn't cover a style point?

Stay consistent!


Perhaps it's my upbringing as a military brat or my overly-logical thought process, but I just can't handle disorder in code. It seems so simple to me, but I see code like this entirely too often:

function foo($bar, $mitz) {
    if ( 0 == $bar ) {
        doSomething();
    }
    if($mitz == 0){
        doSomethingElse();
    }
}

There are some very valid reasons to write your if conditions one way or the other (0 == $bar instead of $bar == 0). I'm sure there are people that will make arguments about whether to put spaces outside of the if condition parenthesis, or extra spaces inside them. But doing it two different ways in a single method is just crazy.

Mixing styles in your code makes it an order of magnitude more difficult to read. Code that is hard to read is hard to maintain. Your code will spend more time in maintenance than in development, so why wouldn't you do everything you could to make it easier to maintain?

Keeping it simple

http://www.flickr.com/photos/r_rose/102766969/

I make no claims at being really smart. I don't even claim to have above average intelligence. But I have worked at companies that have a higher than average bar for employment and consequently above average employees. And working with really smart people, I've noticed that they tend to make one common mistake:

They write really complicated code.


A lone wolf


If you're a single developer working on a project, complicated code might be okay for you to write. Assuming that you write good documentation and are as smart as you think you are, you can write some really clever code. If you are truly as smart as you think you are, you can then maintain that code when you come back to it later. My experience with my personal projects has led me to a few realizations:

• I'm not as smart as I once thought I was
• I'm not as good at writing documentation as I thought I was
• My unit tests aren't as clear as I thought they were

Again, you may be smarter than me, but you're probably not as clever as you think you are.

Joining the pack

Now, join a team of developers. You've got a group of people with varying skills and experiences. None of them are as smart or clever as they think they are. If you can't even understand the complicated code that you wrote as a team of one, how likely is it for the rest of your team to understand your code?

You're no longer writing code in a vacuum. It almost immediately becomes impossible for any team member to understand how the whole system works as the system becomes more complicated. So each developer has little fiefdoms that they wrote, and since they're trying to impress other developers they make sure their intelligence shows through in the code.

Now, join your team with other teams in the workplace... You see where I'm going with this?

Your sanity went that way

The solution is to keep it simple. Assume that when your code breaks, you're going to be expected to fix it on a Friday night after drinking a dozen beers or at the darkest part of the morning when you've run out of coffee. The last thing you want is to have to figure out what your code is doing before you can fix the problem. That means write more documentation about how the code actually works and what it is actually doing. It means avoiding anything that makes it more complicated than it needs to be.

Things to avoid in your simple code:

• Big methods - They're hard to write, hard to test, hard to debug, and most importantly hard to understand.
• Magic numbers - If you don't immediately know what a number means by looking at it, it should be replaced by a constant. And even if you know what the number means, does everyone on your team know? Many coders know that there are 86,400 seconds in a day, but that doesn't mean it shouldn't be replaced by a constant.
• Conditionals - Sure, you're going to need if statements to write a decent sized program, but each branch your method has increases its complexity. You can have a small method that is extremely hard to understand if there are many branches.
• Planning ahead - Programmers tend to be lazy. We try to think of every possibility ahead of time and program for things that may never happen. We needlessly complicate simple code thinking we can see into the future. And if that future never happens (more likely then we would like to admit) the code is wasted. And worse then wasted, it's difficult to understand. Since the code is only supposed to do one thing but you've coded it to do three, maintainers will assume that the three things it does are all equally important.
• Bad names - As part of the growing complexity, it's easy to throw an extra bit of functionality into an unrelated method. Suddenly your simple method sendMessage(), which should just send a message from point A to point B can send a message or log you out of an application or change a configuration option. But if you're not intimately familiar with the code, you naively assume that the method just sends a message.
• Static functions - Static classes and methods look great. You can call them from anywhere, and you can consolidate the similar functionality into a class. You can even unit test the heck out of that static class. But they rapidly increase the complexity of your lower-level code. They easily allow you to include huge chunks of functionality all over your application just by making a static call. That sendMessage() function needs permission, so it's easy to add a SecurityHelper::hasPermission() call inside sendMessage(). Suddenly, sendMessage() doesn't just send a message. It really becomes sendMessageIfSecurityHelperHasPermissionSaysSo().

Writing lots of documentation, adhering to a style guide, and doing test driven development can help keep your code simpler. You'll thank yourself later, trust me.

Building rock solid software in the real world

http://www.flickr.com/photos/preef/32995286/

Recently (2011-11-08) I gave a talk at the Dallas PHP meetup about building rock solid software as a team. For my first experience talking in front of a crowd since high school, I thought it went pretty well. Several people have asked for me to post my slides (which I did), but they were made in a way that doesn't really help people out if they didn't see the talk. The talk was recorded and is available on Ustream, but I thought it might be helpful to do a blog post on the topics I covered as well.

This post is mainly meant to aggregate links to the topics that I talked about.


Tools


I covered several tools. All of these should be available to the developers as build targets and run in your continuous build. Lint and your unit tests should be run as part of your submission process.

• lint - The bare minimum, it just detects syntax errors in your scripts. Code that doesn't pass the lint test won't pass any other tests or manual QA.
• PHPUnit - Standard unit testing framework. There is plenty of information about it elsewhere.
• PHP Code Sniffer - Detects code smells that should be fixed. Many bad programming practices can be written as "sniffs" along with most rules from your smile guide.
• PHP Mess Detector - Statically analyzes your code for possible bugs or coding practices that tends to hide bugs.
• PHP Copy Paste Detector - Scans your code to find large similar blocks which can be factored out to a common method.
• PHP Dead Code Detector - Scans your code to find code that can not be reached. For example, code after a return statement.
• Code coverage - Adding the xdebug extension to your system allows PHPUnit to calculate how much of your code is run by unit tests.

Code reviews

I talked about two different code review packages:

• Review Board - You install it on your server and host it yourself.
• Codereview.appspot - Use Google's code review tool.

And I talked about three different ways of doing reviews:

• Pre-review - Code doesn't get submitted until a peer reviews it. Keeps bad stuff out of your code base.
• Post-review - Code gets submitted, then gets peer reviewed. Comments made about the code may never get resolved, but code reviews don't slow down getting code into production.
• Public shaming - Put the code up on a projector and discuss as a team. Great way to destroy programmer morale.

I mentioned a few points about what to look for in a code review:

• anything the tools couldn't catch
• logic errors (like ifs that don't make sense)
• loops with off-by-one errors
• performance problems (SQL in a loop)
• things to refactor (large methods)
• or things they missed
• Style problems (not really wrong, but you know, wrong)
• Typos (variable names, documentation)
• Tests that don't have assertions
• Methods without tests

Style guides

There's two ways to choose a style guide:

1. Roll your own - Look at your existing code and build the style guide from what you're already doing.
2. Use existing - Such as Zend or Pear.

How to Destroy Productivity on Your Programming Team

Coding on a team of developers is challenging. You've got different coding styles, different personalities, different strengths, and different weaknesses. Aligning all of the team members together can be extremely difficult, and can result in some pretty amazing things when you get it right. But it also results in higher expectations for the future. If you over deliver, you're expected to do the same in the future.

Here's how to make sure that doesn't happen:

• Treat members of your team differently. Only allow certain members of your team to work remotely. If a team member is used to working remotely from previous employment, make sure that he is required to do all of his work at the office. This ensures that he puts in only eight hours a day and can't maintain focus on projects during off hours.
• Make sure there are plenty of interruptions in your developers' office. This is easy to do if you have a cubicle environment, since there is no way for any team member to find silence or privacy. If you do have offices, be sure to cram several developers in to an office. For even better results, make sure that developers that work on the same project do not share offices with each other. Best results can be achieved by seating developers with non-developers that have lots of phone calls and in-person discussions. Salespeople and customer service representatives are perfect for this. Cubicles provide yet another benefit in that they transfer vibration very well. One team member's finger drumming can completely disrupt concentration of others.
• Don't use source control. Allowing developers to use source control keeps them from stepping on each other's toes. It also keeps a record of changes so that the source of bugs and regressions can be found.
• Avoid unit testing. Unit testing helps avoid functionality regressions and new bugs. While it seems that doubling the amount of code developers write would be good for destroying productivity, the gains that come from good unit testing should be avoided. However, you can still impede productivity by halfway using unit testing. This means having one team member write and maintain unit tests for the entire team. Other team members should never run the unit tests.
• Develop an in-house coding standard different from all other coding standards for the language you program in. This ensures that automated code formatting and linting tools are at best difficult for the team to use.
• Ignore established testing and design patterns. Using common idioms makes it easier for developers to keep everything in their head.
• Think about using one of the new software design methodologies. Agile and scrum are very popular for development teams. You should think about using them, maybe have some meetings to discuss the concepts and benefits. Under no circumstances should you actually try them. It is relatively easy to find excuses not to use them.
• Switching the task that individual members of your team are working on helps them to be more well-rounded developers. Allowing them a sufficient block of time to finish a project would only let them gain satisfaction in some finished code. For best results, constantly switch their top priority so developers leave a long line of half finished projects in their wake.
• Make sure that developers do not have adequate machines to do their work. Developers like to have many applications open at any given time. They should have to shutdown Eclipse to load Photoshop for example.
• Impose restrictions on the tools that developers use. Developers get used to using certain programs. Require all developers on your team to use Emacs, or Vim, or Eclipse, or Netbeans. This is also a great way to kill some productive time by arguing about which development environment the team should use.
• Have lots of meetings spread throughout the day. Meetings are a great time sink. In addition to the time actually spent in the meeting, developers may have to prepare for the meeting. To amplify the effect of your meetings, make sure they are spread throughout the day. Developers work best in large blocks of time. So instead of putting all of your meetings in one contiguous morning block, spread them throughout the day so developers will have 45 minute pieces of development after taking in to account commuting to/from meetings, replenishing caffeine, and using the restroom.

Do you have any other ways to subtly impede the productivity of developers?