How do you perform code reviews?

At my current job we do not have a formal process, so everyone treats code review differently, and it's awful.

Some treat it as an opportunity to catch issues and learn. They'll accept feedback and post their updates until everyone is satisfied. That's great.

Some treat it as an inconvenience they have to go through in order to get a rubber stamp on what they've already decided. They do not readily accept feedback and only rationalize, not even justify, their original code. That's terrible.

Some people push their code to the repo before sending out the review, which usually leads to a string of useless commit messages as they address CR comments. Some people use patches to post reviews and updates before it ever hits the repo.

Unfortunately there are politics and egos at play so it's a nightmare trying getting an actual process in place.

I preferred the system at my last job where everyone sent a review before code went to the repository, got at least two others, and had to post updates until everyone marked to "ship it". Commit messages had to have the names of the people who signed off, or at least the CR id. That worked incredibly well for us.

The best code reviews tend not to be memorable, which is nice. They have a small set of changes, they've explained the purpose of their changes in a description, and the code is clear. Everybody in the review is responsive to questions and changes. They often take less than 15 minutes total.

The worst code reviews are memorable. Two come to mind.

The first was a really complicated set of changes that merged 3 features into one giant review, required knowledge of our legacy systems and their interactions, and required attending architecture reviews to understand what was being attempted. The code was messy and extremely buggy and spread out across 4 different modules. The dev who wrote it was the kind who only wanted to get a rubber stamp and would not accept feedback.

That review took 4 weeks and 40 cumulative developer hours to complete. In the end, none of the devs wanted to sign off on either the architecture or the review, but it still got checked in. Then after 3 weeks (!) of testing it went out and brought down our site.

The other review was smaller in scope, but also done by a dev who would not accept feedback. It had performance concerns, maintainability concerns, and introduced an obvious way for any of our users to exploit our site. The dev never posted an update, decided they were done hearing comments, closed the review, merged everything in, and cut a release with all those concerns still in place. It's an escalating battle to keep it from going out.

/r/learnprogramming Thread