Code Review Antipatterns

Don’t you love code reviews? Having your grand solution scrutinised, criticised, and optimised by your colleagues makes you feel awkward? It doesn’t have to be that way. Code reviews are actually fun if done right and if the participants are aware of the pitfalls. They are not just conducive to achieving a high level of code quality, but they are also an ongoing learning experience.

There are three basic forms of code reviews: informal reviews (“hey, can you have a look at this?”), formal reviews (a formal task assigned to one ore more peers), or pair programming. The latter form has the advantage that it is done at the time when code is created. It is also the only form where two individuals engage in a direct conversation. There is no time lag. This makes pair programming the preferential form of review for high-priority tasks.

Agile teams working with git (or any other version control system) often encounter code reviews as a step in their code merging workflow. The typical unit of code being reviewed is a merge request, aka pull request in this setting. So, here are a number of things to avoid when doing a code review.

Not doing code reviews – Let’s begin with the obvious. Four eyes (or six or eight…) see more than two. Errors, problems, vulnerabilities, and bad design gets spotted early on. Corrective measures are still inexpensive at this stage. There is a dual benefit. Code quality increases and knowledge is shared. At least two people of a team are familiar with each feature or function, namely the author and the reviewer.

Confusing code reviews with functional reviews – Code reviews focus on code. Duh! The review is primarily (but not exclusively) concerned with the non-functional properties of the software. Functional reviews, on the other hand, ensure that functional requirements are met. The latter activity is closer to testing, therefore it makes sense to perform functional reviews in a separate procedure, probably as part of testing.

Bitching, nitpicking, fault-finding, patronising – It goes without saying that treating colleagues with kindness and respect is necessary for productive collaboration. Code reviews are no exception. Offensive behaviours only derail the process, while courteous and supportive comments go a long way. The reviewer must guard against coming across as a know-it-all and keep office politics out of the dialogue. In written reviews, annotators should aim at being helpful without being wordy.

Discussing code style conventions – Code reviews should not be about lexical code style, formatting and programming conventions. There are automated tools for this. If you care about formatting and code style, use a linter. More importantly, decide on a set of programming style conventions and configure the linter accordingly. It is totally OK if the team agrees to leave certain options open to individual preference. The formatting and style of code that passes the linter should not be argued, because it’s simply a waste of time.

Ignoring naming – Programming conventions may or may not encompass identifier naming conventions. However, linters cannot determine whether a given variable name is good or bad. Machines don’t care about names, only humans do. Phil Karlton has described naming as one of the two hard things in computer science. By all means, do review the identifier naming in the code review. As a rule of thumb, if an identifier name leaves you puzzled the first time you read it, it is probably not well chosen.

Debate – Code reviews aren’t meant as a podium for discussion. The objective is to improve the resulting artifact, not to find out who has the better arguments. Sometimes, people do have different opinions about practices and solutions. It may be helpful to resolve these by giving examples and references to commonly accepted best practices. If that is to no avail, a third party (or fourth or fifth…) should be be involved. In a written review, consider involving a third party, whenever the dialogue drifts into debate.

Not getting a third opinion – Involving a third party into the review is not only useful in cases where two people cannot agree. See first antipattern: four eyes are better than two, and six are better than four. If an important architectural question is at stake, why not involve the whole team? See: mob programming.

Reviewing huge chunks of code – Code reviews should be limited in scope, ideally to a few pages of code that can be completely read and understood in 20 minutes. Studies have shown that thoroughness and number of spotted issues are inversely proportional to the amount of code being reviewed. Therefore, results are improved if smaller units of code are being reviewed. Including time for reflection and annotations or discussion, the entire review should not take longer than 45 minutes. Anything exceeding one hour is questionable.

Unidirectional reviewing – The general idea is that everyone should review everyones code. Most teams have both senior and junior developers on board or perhaps a team lead or an architect. This often leads to the situation that seniors only review juniors, but not vice versa. This is wrong. First of all, seniors can make mistakes, too. Second, juniors can benefit from reading code written by senior developers. Third, varying fields of specialisation can be leveraged. A junior developer might have special knowledge or skills in a certain narrow area.

Rewriting code – It’s called code review, not code rewrite! Rewriting someone’s code is just not appropriate in most situations. It’s akin to pair programming where the navigator just grabs the keyboard from the driver whenever he feels like it. It’s simply rude. If the reviewer has a better idea, he should annotate the merge request with code examples.

Overdoing it – Programming is not a beauty contest. Code does not need to be perfect to be functional and maintainable. For example, even though I might prefer a functional loop construct over an imperative one, it’s just fine if the imperative one works. Perfectionism sometimes gets in the way and leads to other antipatterns such as premature optimisation and presumptive features (YAGNI). Also: A piece of code written for a one-time migration or ops procedure does not require the same high standards as production system code.

Underdoing it – Being too forgiving is likewise unhelpful. If the code review is executed as a mere approval formality, it’s a waste of time. Code should always be inspected for things like security holes, logical errors, code smells, technical debt and feature creep. It is important to strike a balance between clean maintainable code and reasonable effort.

Not reviewing tests – Unit tests are often mandatory and sometimes functional and acceptance tests are also added to the artifact. These should also be reviewed to ensure that the tests are present, logically correct and meet the standards. It’s a good idea to verify an agreed upon code coverage percentage as part of the review.

Not using a diff utility – Most teams probably use a tool set with built-in diff and annotation capability. In case you don’t, you make your work harder than it has to be.

1997

As the end of the year is drawing closer, it is time for some reflection. Let me take you back not just one year, but twenty. At the end of 1997, I found myself at the epicentre of the Asian financial crisis, also known as the Tom Yum Goong crisis, which originated in Thailand and followed the devaluation of the Thai Baht earlier that year. I was in charge of a startup company in Bangkok providing software services to local companies in Thailand. Within just a few months, almost all of our clients either became insolvent or withdrew from their contracts. As might be imagined, this presented quite a challenge for a fledgling startup business. At times, I was worried that I would not be able to pay out salaries to our employees at the end of the month. Fortunately, it never came to that. Due to providential circumstances that allowed me to bridge the worst periods with loans and advance payments, my company avoided joining the increasing numbers of Asian crisis casualties.

 

However, by the end of that year it became clear that the economic slump was of a more permanent nature and that I needed to restructure the company’s business if I wanted it to have a future. Fortunately, there were two things on our side. The Internet was taking over the world rapidly, creating a sudden demand for web services and web-savvy software and at the same time making remote collaboration more practical. Secondly, the company was small and agile, which meant it could adapt to these changes quickly. Thus I put all eggs in one basket and began to restructure the business as web development company. We moved away from the more traditional enterprise software market and embraced then emerging web technologies. I hired two graphic designers and our programmers learned HTML, Perl and JavaScript.

Perhaps more importantly, we started to look abroad for new business, particularly in Germany and the USA. The idea was not just to offer a new type of service, but also to offer it to a new type of clientèle, namely one that is located offshore in different places around the world. Within six months, between 80% and 90% of the company’s revenue was derived from web services, particularly from creating web sites for small and medium enterprises. As our portfolio was growing, we established a reputation and were able to attract bigger clients. By mid 1998 we merged with a small local web development company and thus cemented the path taken. The transition from working locally to working globally took a bit longer, however, as it turned out to be more challenging. Cooperating remotely with international clients presented not only technical and organisational difficulties. There are also a cultural barriers between Thailand, Europe and America that need to be taken into account.

Among the many cultural differences one finds besides language, communication style, business practices, social habits, the degree of risk acceptance, awareness of hierarchies just to name a few. Efficient communication turned out to be the most challenging issue. It became necessary to put project leads in charge who are not only fluent in English, but who also have some understanding of the way things are done outside of Thailand. In most cases, this adds a layer of management and administration. For example, requirement and specification documents have to be translated. Customer expectations must be communicated clearly to those who execute the work. By 1999, all of our major clients were international. It took almost two years to complete the transition from a local consulting business to an offshore services company. In the end, my company had escaped the snag of the Asian crisis. Perhaps more importantly, it had reinvented itself. The newly defined direction laid the foundation for years to come.

The Agile Samurai

The Agile Samurai

The Agile Samurai
by Jonathan Rasmusson
1st edition, 280 pages
Pragmatic Bookshelf

Book Review

Over the last ten years, I've been working with teams with different degrees of commitment to the agile process, ranging from non-existing to quite strong. I was looking for a text that summarises agile methodology to help me formalise and articulate my own experiences, and of course to enhance my knowledge of some of the finer points of agile practices. I have to admit that this book did not meet my expectations. The first eighty pages up to chapter six are mostly about project inception and read like a prolonged introduction. From chapter six onwards, the author finally comes to the point and discusses the core concepts of agile processes, so the book does get better with increasing page numbers. Unfortunately, Scrum isn't discussed at all, instead Kanban is introduced in chapter eight. The discussion of typical technical processes, such as refactoring, TDD, and continuous integration is compacted into several brief chapters at the end of the book.

The writing style is very informal; the author uses a conversational tone throughout the book. Almost every page contains illustrations, which makes it an easy and quick read. The style of the book is comparable to the Head First books. It left me with the the impression that I sat in an all-day meeting where someone said a lot of intelligent things to which everyone else agreed. Unfortunately, not many of these things seemed radically new or thought-provoking, so I fear I won't remember many of them next month. Of course, this may be entirely my own fault. I prefer a more formal, concise, old-school language. I also prefer dense and meaty text books with lots of diagrams, numbers and formulas. In return, I can dispense with stick figures, pictograms, and even with Master Sensei (a guru character used in the book). I feel that a lot of the deeper and more complex issues of agile project management have simply been left out.

To be fair, it must be mentioned that I probably do not fall into the target group for which this book was written. It is more appropriate as an introductory text for people who are new to agile project management, or even new to the entire business of project management. Think "trial lesson" and "starter course".

Oracle buys Sun

Generally I don’t comment on events in the business world, since this blog is about web development and software engineering. However, the acquisition of Sun by Oracle, which was officially announced yesterday, is so large-scale that it is likely to affect the engineering halls in many subtle ways. Quick facts: the deal is $ 7.4 billion worth, it was unanimously approved by Sun’s board and it will be closed this summer. Sun and Oracle published identical press statements yesterday which sing the praises of the acquisition.

I am not sure whether this is good news. While it was apparent to most observers that Sun was past its zenith, one wonders what will happen to its employees and its innovations. Granted, an acquisition by IBM would have tipped the scales even more in favour of Big Blue’s dominance in the enterprise market and that might have distorted competition. But one may doubt that Oracle will uphold Sun’s commitment to the open source community. Sun’s market was driven by innovation and open source products. Oracle’s market is clearly not.

In particular, one wonders what will happen to MySQL which was bought by Sun earlier last year and which competes with Oracle’s core products. Pessimistic observers have already called it MyToast. Will Larry Ellison allow MySQL to compete in the enterprise market? Probably not. Other items in Sun’s portfolio once considered crown jewels, such Solaris and Glassfish, might also be on the endangered list. But that is pure speculation at this moment. Whether this acquisition will turn out to be a good move for Oracle is currently debated by the industry experts. Whether it is a positive turn for open source community may be reasonably doubted. Time will show.

Scala Hits Top 30

The Scala programming language has for the first time hit the top 30 of the TIOBE index in April this year. The TIOBE index measures the popularity of programming languages by counting searches for the respective programming language in the most popular search engines. In April 2009, Scala searches were tracked at 0.237% of all searches which places it at rank 28. This means it is already ahead of other functional languages such as Haskell, Erlang amd Caml. The TIOBE track record is an indicator of Scala’s growing popularity. Scala entered the TIOBE index in early 2008 and appeared in the Top 50 for the first time in the fourth quarter of 2008.