Making code review on BitBucket suck less

I'm a big fan of code review, I feel that almost everything should be given a once over. I'm used to it in various work places, I'm especially used to it when working in an open source project.

BitBucket is a follower, not a leader, and has managed to get a lot of what makes Github work well added - inline comments are great.

BitBucket has lead in one area though - much like a 'merge pull request', on each change there's an Approve button.

If you are happy with a commit, it's a click away - you don't even have to say 'LGTM', if not, have a discussion.

This adds a handy tick in the commits list, letting you see what is accepted or what needs to be talked about.

There are two problems with this though:

• Every time you hit the button, notifications are sent
• The review is per commit, not per feature or branch. If you use a simple 'all in' model, it's hard to group a stream of work - later fixes are applied that invalidate your review, all in the same push.

The best answer I have for issue 1 is to create email rules. If it has to works "commit approved" from bitbucket.org, mark as read and archive it.

This dramatically changes the signal to noise ratio, but lets you still leverage the visual indications of review having taken place.

For the second, feature branches and pull requests are the next best bet - BitBucket allows you to do a pull request from a feature branch on your repository to master.

This lets review take place on sets of code and features; and works extremely well for integrating stable changes only.

Good habit: in_array()'s third param

I got lulled into a relaxed state of mind with using in_array() to guard against input.

I had a validation method like:

$valid_types = array(0,1,2,3,4);

$type = 'string string string';

var_dump($type);
var_dump($valid_types);

var_dump(in_array($type, $valid_types));
var_dump(in_array($type, $valid_types, true));

Without executing it, what do you think happens?

I thought: bool(false), bool(false).

WRONG! in_array() does type conversion, so (int)"string string string" is 0; and yes, that's in our array.

So; to avoid surprises, always supply the strict param to in_array().

Its also a good thing to keep an eye on with code review.

php style: foreach is better

Since it seems that everyone wants to know about reading and writing csv with php, I thought I'd take a moment to push the virtues of the foreach loop.

First: It's neater. Consider that

foreach ($collection as $item) {

}

is a billion times neater than:

for ($i=0; $i<count($collection); $i++) {
 $item = $collection[$i];
}

But time and time again, I see the latter used.


Second: What happens if your collection isn't neatly ordered? In this example, the size of the collection is 2, and the for loop will completely miss things. This rarely happens, but can be very frustrating when it does.

$collection = array(1,2,3); unset($collection[1]);

for ($i=0; $i<count($collection); $i++) {
 $item = $collection[$i];
}

Third: I cannot think of more than one situation where I've ever had to do anything other that forward-traversal. So why waste time with a for() loop unless you really, really need it. Consider the readability of this code below, and the nightmare people can experience when trying to maintain your code.
This was some code for interacting with an open office document, extending the DOM:

if ($stylesList->length > 0) {
$styles = $stylesList->item(0)->getElementsByTagName('style');

if ($styles) {
    //Load styles
    for ($i = 0; $i < $styles->length; $i++) {
        $style = $styles->item($i);
        $name = strtolower($style->getAttribute('name'));
        $this->styles[$name] = $style;
    }
}
}

and this is the same code, after refactoring:

$nodeList = $this->getElementsByTagNameNS(self::XMLNS_OFFICE, 'automatic-styles');

if (!self::checkNodeList($nodeList)) {
 return array();
}

$node = $nodeList->item(0);
$nodeList = $node->getElementsByTagNameNS(self::XMLNS_STYLE, 'style');

if (!self::checkNodeList($nodeList)) {
 return array();
}

foreach ($nodeList as $node) {
 $name = strtolower($node->getAttributeNS(self::XMLNS_STYLE, 'name'));
 $this->styles[$name] = $node;
}

Notice how everything is easier to read, but the level of complexity has increased? Foreach makes life simpler...

Forth: It's faster.