One assertion per test

There's a right way and a wrong way to write unit tests. I've seen lots of wrong ways (like unit tests with no assertions) and differing shades of rightness. One thing that isn't really "wrong" but is not as right is writing your test cases with multiple assertions per test case.

Here's a simple example of a test with multiple assertions:

function testSomethingThatReturnsAnArray() {
    $value = somethingThatReturnsAnArray();
    $this->assertEquals('Foo', $value[0]);
    $this->assertEquals('Bar', $value[1]);
    $this->assertEquals('Mitz', $value[2]);
}

This could easily be rewritten to multiple assertions:

function testSomethingThatReturnsAnArrayFirstElement() {
    $value = somethingThatReturnsAnArray();
    $this->assertEquals('Foo', $value[0]);
}

function testSomethingThatReturnsAnArraySecondElement() {
    $value = somethingThatReturnsAnArray();
    $this->assertEquals('Bar', $value[1]);
}

function testSomethingThatReturnsAnArrayThirdElement() {
    $value = somethingThatReturnsAnArray();
    $this->assertEquals('Mitz', $value[2]);
}

What does this gain? You can immediately see whether somethingThatReturnsAnArray() is broken for all of the tests or just the first. With the first example, if $value[0] is not 'Foo', the test will immediately fail and you won't even test whether the next two elements are correct or not.


But the method I'm testing is slow!


If somethingThatReturnsAnArray() is so slow that you don't want to run it three times, you've got two options:

• Make the method faster.
• Only run it once per test suite run.

Obviously making the method faster would benefit your code the most overall, but it isn't always possible. But you can always just run it once, either by saving its output as a member variable of the test suite or making the tests depend on each other.

Saving the output is easy:

$result = null;
public function testSomethingSlowThatReturnsComplexResult() {
    $this->result = somethingThatReturnsComplexResult();
    $this->assertEquals('Foo', $this->result[0]);
}

public function testSomethingSlowThatReturnsComplexResultSecond() {
    $this->assertEquals('Bar', $this->result[1]);
}

But there is a problem. Tests shouldn't require a certain order unless it is explicitly set. If you move the second test method above the first it will start failing. You could have a helper method that only executes the function once:

$result = null;
private function runSomethingSlow() {
    if (!$this->result) {
        $this->result = somethingThatReturnsComplexResult();
    }
    return $this->result;
}

public function testSomethingSlowThatReturnsComplexResult() {
    $result = runSomethingSlow();
    $this->assertEquals('Foo', $result[0]);
}

public function testSomethingSlowThatReturnsComplexResultSecond() {
    $result = runSomethingSlow();
    $this->assertEquals('Bar', $result[1]);
}

Now the slow function only is executed a single time, and the test methods don't have an unnamed dependency on any other tests.

But my method is really complicated!

If you have a very complicated method that has lots of dependencies, you may be tempted to test actions on all of the dependencies at the same time as well as the return value so you don't have to have all of the boilerplate code repeated. I've heard some otherwise very talented coders make this argument, which always surprises me. Test code is not really different from production code in that if you find that you're repeating code several times, it should be factored out into a method.

Let's try testing everything about this method:

function compareAppleToOrange(Apple $apple, Orange $orange) {
    return array(
        'sizeDifference' => $apple->getSize()
                - $orange->getSize(),
        'peelDifference' => $apple->getPeelWidth()
                - $orange->getPeelWidth(),
    );
}

You could test it all in one big test:

function testCompareAppleToOrange() {
    $apple = $this->getMock('Apple',
        array('getSize', 'getPeelWidth'));
    $apple->expects($this->once())
        ->method('getSize')
        ->will($this->returnValue(4));
    $apple->expects($this->once())
        ->method('getPeelWidth')
        ->will($this->returnValue(2));

    $orange = $this->getMock('Orange',
        array('getSize', 'getPeelWidth'));
    $orange->expects($this->once())
        ->method('getSize')
        ->will($this->returnValue(3));
    $orange->expects($this->once())
        ->method('getPeelWidth')
        ->will($this->returnValue(3));

    $expected = array(
        'sizeDifference' => 1,
        'peelDifference' => -1,
    );
    $result = compareAppleToOrange($apple, $orange);
    $this->assertEquals($expected, $result);
}

Better yet, you can test each piece on its own:

function testCompareAppleToOrangeSize() {
    $apple = $this->getMock('Apple',
        array('getSize', 'getPeelWidth'));
    $apple->expects($this->once())
        ->method('getSize')
        ->will($this->returnValue(4));
    $apple->expects($this->any())
        ->method('getPeelWidth');

    $orange = $this->getMock('Orange',
        array('getSize', 'getPeelWidth'));
    $orange->expects($this->once())
        ->method('getSize')
        ->will($this->returnValue(3));
    $orange->expects($this->any())
        ->method('getPeelWidth');

    $result = compareAppleToOrange($apple, $orange);
    $this->assertEquals(1, $result['sizeDifference']);
}

function testCompareAppleToOrangePeel() {
    $apple = $this->getMock('Apple',
        array('getSize', 'getPeelWidth'));
    $apple->expects($this->any())
        ->method('getSize');
    $apple->expects($this->once())
        ->method('getPeelWidth')
        ->will($this->returnValue(2));

    $orange = $this->getMock('Orange',
        array('getSize', 'getPeelWidth'));
    $orange->expects($this->once())
        ->method('getSize')
        ->will($this->returnValue(3));
    $orange->expects($this->any())
        ->method('getPeelWidth');

    $result = compareAppleToOrange($apple, $orange);
    $this->assertEquals(-1, $result['peelDifference']);
}

This allows you to change the peel calculation without touching the test for size, or the size calculation without touching the peel calculation.

If you wanted to really split the test up, you can go even further since the mocks  provide some assertions of their own with the expects() calls. We'll split up the sizeDifference tests:

/**
 * @return array Array from apples to orange comparison.
 */
function testCompareAppleToOrangeSizeCalls() {
    $apple = $this->getMock('Apple',
        array('getSize', 'getPeelWidth'));
    $apple->expects($this->once())
        ->method('getSize')
        ->will($this->returnValue(4));
    $apple->expects($this->any())
        ->method('getPeelWidth');

    $orange = $this->getMock('Orange',
        array('getSize', 'getPeelWidth'));
    $orange->expects($this->once())
        ->method('getSize')
        ->will($this->returnValue(3));
    $orange->expects($this->any())
        ->method('getPeelWidth');

    return compareAppleToOrange($apple, $orange);
}

/**
 * @depends testCompareAppleToOrangeSizeCalls
 * @param array $result Result of comparison.
 */
function testCompareAppleToOrangeSize($result) {
    $this->assertEquals(1, $result['sizeDifference']);
}

Each split gives you more information on a failure and makes debugging test failure easier as well as reducing the changes required for refactoring.

What's in a (method) name?

Through my coding career I've come with some signs that the method I'm writing or reviewing probably needs to be refactored. These aren't hard and fast rules, but in general these are sniffs that something may be amiss.




Methods with 'or' or 'and' in their names


It's a pretty good sign that you're doing too much in a single method if it has the word 'and' in its name. For example, addMessageAndNotifyUser() should probably be split into two methods: addMessage() and notifyUser().


Methods with 'or' in their name aren't as easy to classify. If the whole method is just a single conditional, the method name may make perfect sense. I would argue that it is just poorly named.




Methods with almost the same name


If you've got multiple methods that all have basically the same name except for a useless word, you're probably making the code harder to understand. For example if you wanted to add a message, which of these would you call?

• addMessage
• addMessageProper
• addMessageActual

If the first addMessage calls one of the other ones, is adding a message its main task? Could it be renamed to something that makes more sense?




Methods that start with 'do'


Do is always a wasted word. I see this mostly from coders that got their start with Visual Basic. Just don't 'do' it.




Why does it matter?


Readability is the most important thing to strive for in code. Bug free, maintainable, and fast code come from code that is readable. Most of the code's life cycle is spent in maintenance. The longest part of a typical maintenance task is to figure out what a particular piece of code is trying to do. Good documentation goes a long way towards making code understandable, but if the methods are actually well named you won't even need to look at the documentation.




Update (2012-03-15): I felt the need to revisit this post. Poor naming in the code base I'm working on wasted several hours of my day. I was trying to find where a set of objects was getting loaded from our data store. Looking through other code that did the same type of thing was a deep rabbit hole of included files and static calls to other classes. Finally I asked a teammate where the objects get loaded. He pointed out that they were loaded in a method named 'handleSortingAndPagination'. The method name doesn't provide any hint that it loads the objects from the data store before sorting and paginating them. The documentation for the method didn't either. This adds another bad naming smell: names that do not reflect what the method does.

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.

Forcing PHPUnit tests on Subversion commit

All of my PHP projects use PHPUnit for unit testing. One of my coworkers just refuses to understand the point of testing. He doesn't get Test Driven Development (TDD), and rarely runs the unit tests for his classes. This means that he frequently breaks my build. I can't force him to change his behavior since he doesn't work for me, but I can change the way my systems work. And I control the Subversion server.
I wanted to force unit tests to pass before allowing submission, but couldn't find anything about running PHPUnit tests in a Subversion pre-commit hook. So I wrote my own. Hopefully this will help someone out there:
To use this, copy it to the hooks directory in your Subversion repository and name it 'pre-commit'. Make sure the paths defined in the script are correct for your system. Make it executable (chmod +x pre-commit).

Thoughts on Frameworks

Every developer thinks that their way of coding is better than every other programmer out there. They think their APIs are more elegant, their style is more succinct, and their loops are more optimized.

They are wrong.

I've had the misfortune to work with several custom-built frameworks. They were all terrible in their own way. Some had a slight excuse in that they were built in aging versions of PHP, but the otherall crappiness of the framework still stands on its own. And I can't claim that I think they frameworks were utter shit because I didn't write them, since I did write one of them.

But in writing my own framework and realizing just how crappy it ended up gave me the perspective to realize that I should use one of the big frameworks. I should use a Zend Framework, or Cake, or Symfony, or whatever. The programmers that wrote those frameworks may not individually be a much better developer than me, in aggregate they tower head and shoulders above. They get the benefit of having security experts picking at the seams of their interfaces, optimization guys fine tuning little used paths, and usability experts chiming in on workflow. They get the benefit of people trying to run the frameworks on operating systems you've never heard of, trying things with them you've never thought of, and using browsers you're too good to use.

Every developer should write their own framework, throw it away, and use one of the established ones.

It's so tempting to have the control over every aspect of the framework and the complete understanding of how each moving piece fits together. It's nice being able to custom build functionality for each small piece of your application into the framework-level code. But what always happens is that you start a new project and reuse your framework since you've put so much work into it. Then you find yourself having to code around all of the special cases that you put into the original. You're better off just using a more general framework from the start and putting your business logic where it belongs.