These notes are based on my experience of reviewing submissions for the Concrete CMS marketplace and before that as manager of software projects and as a consultant called in to review projects. This covers an enormous scope, but the principles remain the same.
A developer works on some code and at some point refers their code to others for review. A developer can be an individual, a team, or an entire business. A reviewer can be an individual, a team (a review board), or all the way up to representatives of many interested parties. Review may be a reading exercise, or may involve further testing.
The principles remain the same.
This is such an important consideration and extends into every aspect of a review. Do everything you can to make the reviewers job easier. You will see aspects of this repeating through everything below. Making the reviewers job easy impacts both the technical and the psychological aspects of the review.
By making the reviewers job easy, you make them psychologically predisposed towards saying 'yes' and everything will be over quicker. Conversely, if you make a reviewer's job unnecessarily difficult, the review will take longer and you have made them psychologically predisposed towards saying 'no'.
By making the reviewers job easy, you also make your own job easier.
Every review process will have some rules. Read the rules and follow them. Every minute a reviewer spends correcting you on the rules delays them from actually getting on with the real review of your work and on other reviews they may be working on.
If your code incorporates 3rd party components, make sure you observe any license requirements. Licenses are legal documents must be complied with.
Before submitting your work for review, do your own QA. Do your own testing. Read your own code. Test as if you were a customer installing your code.
Skimping on your own QA on the basis that "we can save time now and let the reviewers find our problems for us" ultimately wastes both your time and the reviewer's time.
The earlier in the overall process a problem is found and fixed, the less it ultimately costs.
Closely related to the above, solve any problems before submitting your work for review. A reviewer's role is not to find solutions for problems you already know about. If you know about a problem, solve it before the review. If you need help, get help before the review.
The reviewer may have the necessary expertise, but the review is not the place for them to provide such assistance.
You should learn from a review, but don't treat it as a school or as easy consultancy.
Throughout your code, explain what you have done and why. Make this part of your submission, such as using comments in the the code. Don't wait for a reviewer to ask.
Consider two scenarios
Depending on the question/response medium and the review format, scenario (2) takes from minutes through to weeks longer than scenario (1).
Providing a system for the reviewer to run your code saves them needing to set up such a system. In the context of a Concrete CMS marketplace review, a reviewer will usually have a test system they can install and test on. But if your code requires anything more than a default installation on a localhost, you can save them a lot of time by providing a system with everything ready for them.
Similarly, if your code integrates with an external service or API which require an account, provide the reviewers with a suitable account. Don't expect them to spend time registering their own account with such external services, unless the account registration is actually part of the review.
Some issues identified by a review are absolute. Some are subjective. Some are because a reviewer knows more than you about the bigger picture. Project specific conventions. Coding standards. How stuff fits together. Or perhaps the reviewer has more experience at looking at things from outside your immediate point of view.
Within all this, there can be changes that a reviewer considers more important than you do. Whilst you have every right to ask for details and propose alternatives, don't let it become a protracted argument.
Apply realpolitik. If your objective is to get your work through the review, it is often quicker to make a requested change than it is to get into an extended discussion about not making a change.
A good reviewer is reviewing your work, they are not reviewing you. A comment about the code, a request for a change or a suggestion for an extra function will be intended to create a better product at the end.
An added complexity in these days of international projects, you or the reviewer may not be communicating in your first language. Something intended to be polite and helpful may appear unnecessarily blunt when translated literally. Allow some leeway for translation.
Its not about ego. Its about the best product.
Where a review requests changes, track the changes you have made and confirm they have been made for the next cycle of review. 'Forgetting' to fix something only for a reviewer to immediately ask 'what about that change from last time' is a needless way to break rule #1.
When a reviewer finds a problem, don't just fix that problem. Look for similar problems and fix them before the reviewer finds them. On your next review, don't repeat that problem or similar problems.
Simplifying to an extreme. A reviewer asks you to fix a specific spelling mistake on line 3 of paragraph 2 of your user guide. So you generalise and fix it everywhere. The same applies to code.
Whilst you can expect reviews to be reasonably consistent, don't count on reviews or reviewers to be 100% consistent.
Just like if you were to write code for the same functionality again, there would be some overlap and some differences in how you wrote it, you can expect similar overlap and differences between reviews.
Don't think of a review as a chore, an unwanted hoop you have to jump through and could do better without. Think of it positively, as a way to create a better product and an opportunity to learn on the way.
Play some mind games with yourself. You have a right to review. You have a right to a thorough review.
Reputation carries across between reviews. If you gain a reputation for failing to do your own QA, for making the reviewer's job unnecessarily difficult or for making reviews take longer than they need to, then reviewers will be predisposed against your work next time they are involved in reviewing it.
On the other hand, when a reviewer remembers your work for all the right reasons, they will welcome your next work with anticipation of everything being smooth and looking forward to saying 'yes' again.
If you would like to discuss any of these thoughts, please start or continue a thread on the Concrete CMS Forums.
"It takes 20 years to build a reputation and five minutes to ruin it. If you think about that, you'll do things differently." Warren Buffet.
The term egoless programming was coined by Gerald M Weinberg in his book The Psychology of Computer Programming. It can be summarised that all criticism should be made and accepted in a constructive way without letting egos get out of control.
Realpolitik is about being pragmatic rather than idealistic. It was fist coined by German politician Ludwig von Rochau in 1853.