Slimy, smelly garbage!

That’s a rather extreme description, but reviewing someone else’s code, often without any form of documentation or guidance, can certainly be challenging.

Code reviews are sometimes required because a dispute revolves around whether the development has been done with ‘reasonable skill and care’ and the client has become concerned that even if all the known defects are fixed there are many more lurking beneath the covers.

In these situations another consultant, or the client’s own IT team, may already have reviewed the code and formed their own opinion, but its fair to say that I have yet to read a consultant’s review of even pretty good code that doesn’t find something to criticise. It’s also very easy to run most software through an automated code analysis tool and come up with a long list of issues.

The key for an expert code review, which could end up being tested in court, is to keep firmly in mind that there is a spectrum of quality, from the absolutely terrible to perfection, and try to be objective in placing the code on that spectrum.

As a simple example, naming conventions are ‘a good thing’ in code and databases. They make code easier to read and that reduces the number of silly mistakes that people make. In database design there are two fairly classic conventions used to name primary keys. Some people like to name the key for every table ‘id’, e.g. employee.id for the employee table. They then call the equivalent foreign key in other tables employee_id. Other people prefer to give all instances of the same key the same name, so it would be employee_id in every table.

Personally I prefer the latter, but both are fine as long as they’re used consistently. So while I would probably change the convention if I were taking the design over, for expert purposes I have to put my personal prejudices aside and grade both approaches the same.

Leave a Reply