Random thoughts on code review_random thoughts on coding-优快云博客

1. Code review is a worthwhile practice/long-term investment with the following advantages:

a. Improve overall code quality

i. Especially helpful for novices

ii. Especially helpful for those who are unfamiliar with the infrastructure

b. Enforce coding standard

c. Enhance communications among developers

i. Know each other’s work

ii. Prevent reinvention of wheels

iii. Help with each other

d. Learn from each other’s code: good/bad algorithms, good/bad coding styles, idioms/common mistakes

e. Learn to spot problems quickly

2. Some disadvantages:

a. Obviously, need extra time and effort

b. Might cause tension between people when they have difficulty to appreciate each other

3. Difficulties and issues in practicing code review:

a. Because of the required extra time and effort, a barrier coming from the executive level might exist. This should be overcome before anything else. Some detailed analyses of the following aspects might be necessary:

i. The general and specific pros and cons of code review

ii. The impact of code review to the development and maintenance both in the long term and in the short term

iii. A moderately detailed and sound plan for conducting code review

iv. Plan-based estimation of the extra time and effort

v. Cost-benefit tradeoffs that the code review will cause

b. Because some developers might feel intimidated if a mechanism is established to allow other people to assess their code, a barrier coming from the developers might also exist. To overcome this, the following things can be done:

i. Present the pros and cons of code review.

ii. Show clearly what benefits each developer can obtain from such a practice.

iii. Ease and relieve any concerns like feeling embarrassed when a deficiency is caught by others. Emphasize code review is not for attacking/embarrassing people.

iv. Show the actual plan that will effectively minimize the possibility of personal attack.

v. Make it clear it is a required process from the higher level.

c. In what form should a code review be conducted?

i. A team-wide meeting can be a good choice.

ii. Better off avoid meetings just involving two people, where bullshiting and other political silliness could likely happen.

iii. Asynchronous reviews might be effective as well and might cost less.

d. How often should a code view meeting be conducted?

i. Once every two weeks might be a good choice if code view will take off one whole day.

ii. Shorter period can also be good if code view is significantly shorter.

e. How to get people involved?

i. Every teammate could be involved in general.

ii. If the team is big, 2 to 4 reviewers each time might be a good option.

iii. If some people are extremely unfamiliar with the programming language in which the to-be-reviewed code is written, they can be spared.

iv. People who are sick or extremely busy can be spared.

v. People who cannot attend the meeting should submit their reviews by email.

vi. The manager should not attend unless he has something to contribute to the reviews.

vii. The coder(s) should present the to-be-reviewed code at least two days before the review. It is better to have the code printed into a PDF file that can be delivered to every reviewer by email. The code should be printed with line numbers.

viii. One person will be responsible to summarize all comments in a precise and timely manner.

f. How much code to review each time?

i. One or more coders can submit their code for review each time.

ii. Each code is better within 200 lines excluding comments.

iii. More lines are OK if the algorithm is not very complicated.

iv. Less lines are preferred if the algorithm/logic is difficult.

v. For complicated algorithms, introductions will be very helpful or even essential.

g. Reviewing guidelines

i. A check list could be provided as a reference.

h. Documentation

i. The summarized comments should be documentated for further reference.

ii. A unique ID for each review could be created for easy reference.

iii. A check of the compliance of the implementation with the comments could be followed up and documented, and any rejections should be rationalized, discussed and passed.

iv. Contributions from people should be included.

v. The same code from should not be reviewed again unless it has been changed a lot since last time or some issues from last time has not been settled.

i. Rewarding mechanism

i. Experienced coders might not benefit a lot from the review.

ii. A rewarding mechanism might be useful to stimulate the enthusiasm out of the experienced coders.

j. Moderator

i. Guides the meeting along and keeps everyone on task.

ii. Making sure the meeting stays constructive and focused on the code rather than the coder

4. Code review guidelines:

a. Look for code that breaks the code-standard

b. For multithreading programs, look for race conditions, dead-locks.

c. Look for bad algorithms, which includes (but is not limited to):

i. Unnecessarily complicated algorithms

ii. Very inefficient algorithms

iii. Algorithm that makes the use unnecessarily complex/difficult

d. For security-critical programs, look for the following issues:

i. Possible `printf’ format string vulnerabilities

ii. Potential buffer overflows issues

iii. Plain strings of sensitive information

e. Look for exception-safety issues:

i. Make sure the function satisfy one of the following exception guarantees: no-throw, strong, and weak exception guarantees.

ii. Make sure exception is used only for exceptional situations, in other words, no abuse of exception in the code.

f. Look for clear definition of pre- and postconditions:

i. Make sure code enforcing these conditions is there.

ii. Make sure the program handles different conditions properly.

g. For number-crunching programs, look for the following:

i. Number overflow and underflow issues

ii. Accuracy issues (Does the operation result in an accurate result?)

iii. Divided by zero issues

iv. Square root of negative number issues

v. Loss of accuracy in floating number summation

h. For clear and effective definition of interfaces, look for the following things:

i. The responsibility is clearly defined

ii. The class/function does only one major thing

iii. The name of the class and function is suggestive and clear:

1. Too long or too short names are bad

2. Use of unusual abbreviations is generally bad

3. Implication of insult, violence, sex, and politics is bad

4. Ambiguous names are always bad

5. Non-suggestive names are bad

iv. Encapsulation is properly done

v. Interface is complete (Does it allow all valid operations?).

vi. Interface is tight (Does it have a minimal set of functions?).

vii. Interface is easy to use.

viii. No hidden traps and pitfalls

i. Look for bad designs:

i. Use of global variable

ii. Implicit share of objects among different classes

iii. Tight coupling between different external classes

iv. Too many responsibilities for a single class or function

v. Non-orthogonal functions/classes.

j. Look for wasteful use of memory.

k. Look for resource leak:

i. File handles

ii. Database connections

iii. Sockets

iv. Memory leaks

v. Double release of memory or other resources

vi. Reference-counting

vii. Event traps

viii. Exception traps

l. Look for code-testing issues:

i. Is it easy to write test cases for the code?

ii. Are the current test cases sufficient enough?

m. Look for object-copying issues:

i. Is it a shallow copy whereas a deep copy should be used?

ii. Is the object-copying efficient?

iii. Does it cause illegal memory use?

n. Look for code that may not be portable

o. Common mistakes

i. Out-of-bound array index

ii. Reading uninitialized object

iii. Type mismatch

iv. Bad/unsafe type conversion

v. Bug-fix change function’s behavior

vi. Returning reference to a temporary object

vii. Hug array in stack

p. Guideline can be specified for a particular field of programming.

5. Art of reviewing

a. Bottomline – reviewing is mainly a process of learning, understanding, and discussion, rarely of accusation, attacking, and commanding.

b. Focus on the code, rather than the coder.

c. Be inspiring, instead of dismaying.

d. For an obviously “mistake” or an oddness, query whether there is a reason behind that, instead of saying the code breaks thing or the code is wrong.

e. Avoid using “why” when asking questions, instead say that you see something unusual and state what you would do and the reasons, and then query the author’s reason(s) backing his choice.

f. Don't blame people for bugs. No finger pointing!

g. Don't forget to praise.

h. Be patient in explaining technical details to the reviewee.

i. For reviewee, don’t assume attack. Explain with patience your intention for the problematic code.

j. For reviewee, have the guts to admit mistakes.

k. It might be better to not cover coding standards violations in the meeting, instead mention these via email or in person.

l. Before review is done, code can be checked into a private branch. After review and after all changes are done (any rejections should be discussed with and passed by the reviewers), the code is merged into the parent branch.

m. Stick to reviewable issues. Develop a list of reviewable issues. Don't allow reviews on other items without changing the list. This is for preventing people from spending time on off-topic arguments.

n. Unless a manager has something to contribute to the reviews, they shouldn't be involved.

o. If issues come up during the review that everyone in the team could benefit from, make the wisdom public (wiki might be a good choice for this).