First principle of code review: Check Yourself At The Door

<< Previous Article: 'The Principles of Code Review'

For years, I looked forward to reviewing someone else's code. I looked forward to pulling out the scalpel and dissecting the lines of their code to find a better algorithm, to find a better structure.

For years, some of my code reviews descended into arguments over whose approach was better. For years, most of these arguments ended with the code unchanged, the developer ever more convinced of how correct his code was, and further convinced that I was a fool. I'm sure he thought that. After all, that's what I thought of him.

For years, when someone reviewed my code, I would behave the same, too. I would defend my code, I would justify its existence, like a defendant making a submission to the court. And, more often than not, I would stick to my guns. I still do. I can't help it.

The mistake I made when doing code reviews was a basic one: I assumed that the quality of my code review was also the quality of my expertise. If my review was bad, and missed things, then that reflected poorly on me, not on the coder. Which is why I was precise and nit-picky. I didn't want to miss anything and look stupid.

Anything that looked unusual to me was pointed out, and I tried to make it less unusual. Which meant instructing them in how to code in my style, because that's what I was most familiar with.

That's all fine, except for one thing: a code review isn't about me. It's about the code. My job as a code reviewer is to make sure they have written something that accomplishes the job. Not something that accomplishes it in the way I think it should.

Leaving your ego at the door isn't going to help you make a wiser code review. But it will stop you from trying to convince someone that your approach is better. If they disagree at first, that's their problem, not yours. There are few times when good developers disagree with a recommendation that is obviously better. People disagree when the improvement is more a matter of opinion. If you come to the table with your ego, many of your recommendations will be loaded with your point of view, not your technical knowledge.

Your job as a code reviewer is simple:

• Offer factual observations, not opinion of style. Observe possible division by zero errors, not better organization of the division algorithm.
• Don't get bogged down on better names of variables (unless everything is named $a, $ab, $iii ...). Don't get bogged down indentation. Don't get bogged down on brackets. Don't get bogged down switches vs conditionals. A code review is not the place for style reviews. A code review is the place for making sure it does what it's supposed to do, in a way that's maintainable. End of story.
• Don't reinvent their approach. If they way they coded it doesn't violate basic principles of coding, don't call them on it.
•  Ask yourself one simple question: is my suggestion based upon my opinion of how it should be written and, if so, why is it better than his opinion?

When you review the code, keep your eye on the ball: you are making sure the code is correct, not that it does it in the way you would do it yourself.

Next: The Second Principle of Code Review: Understanding the Code

Why am I not getting back the correct id of my insert on an auto_increment table?

Oh, lovely MySQL. Everyone loves you and everyone forgives you for your subtle insanities. Like auto-increments.

If you have a table that is auto_incrementing, the id of your insert is returned using LAST_INSERT_ID(). This works jim dandy when mysql determines the id. If, on the other hand, you are so presumptuous as to overrule mysql and set your own value of the primary key, then the database goes and pouts in a corner.

mysgl> create temporary table test( id integer not null primary key auto_increment);
mysql> insert into test values (null); select LAST_INSERT_ID();
+------------------+
| LAST_INSERT_ID() |
+------------------+
|                1 | JUST as we expect. Life is good ...
+------------------+

mysql> insert into test values (null); select LAST_INSERT_ID();
+------------------+
| LAST_INSERT_ID() |
+------------------+
|                2 | Things are going good ...
+------------------+

mysql> insert into test values (33); select LAST_INSERT_ID();
+------------------+
| LAST_INSERT_ID() |
+------------------+
|                2 |  !! WTF! 
+------------------+

mysql> insert into test values (null); select LAST_INSERT_ID();
+------------------+
| LAST_INSERT_ID() |
+------------------+
|                34| !! and now we're back to normal!
+------------------+

For more discussion of LAST_INSERT_ID() gotchas: http://dba.stackexchange.com/questions/21181/is-mysqls-last-insert-id-function-guaranteed-to-be-correct

Symfony logging tutorial

Symfony logs can be a very helpful tool when you are developing and/or testing.

Writing to a symfony log

The dev log file is written to whenever any code in Symfony writes using the Monolog service (in the dev environment):

$this->get('logger')->info("hello world");

Type the following command in a terminal, and it will watch the dev log file, spitting out the contents of the log file as the contents are written.

tail -f PATH_TO_YOUR_APP/logs/dev.log

Things to know about the Monolog Service

There are some behaviours with logging using the Monolog service you should be aware of.

• when in production, logger->info() messages are simply discarded (for performance)
  
• when in production, logger->err() messages can trigger emails, indicating the message being logged
  
• when in production, if logger->err() is called, monolog *will* write any logger->info() messages to the prod.log file, that were leading up to when the logger->err() was called
  
• when *not* in production, logger->err() and logger->info() write the error into the corresponding test.log or dev.log file
  
• logging with monolog is like writing a trace of activity in the application (take a look at the existing log files, and you will see what I mean)
  
• use it when a critical error occurs, and you need to be notified immediately

  $this->get('logger')->err("the database is not responding");

For information on configuring Monolog to email production errors: http://symfony.com/doc/2.0/cookbook/logging/monolog_email.html

For information on Monolog and Symfony: http://symfony.com/doc/2.0/cookbook/logging/monolog.html