How to write a Code Review

Created by thiago on 2020-05-22 10:54:25

Tags:

There is always those quite obvious things such as "don't be a jerk". Those are not the ones I will be talking now.

I want to talk about what I learned in those many years reviewing code and getting my code reviewed.

Change the perspective

Whoever wrote the code you are reviewing thought hard about that code. Hardly someone just writes carelessly some code, so if something is not clear, instead of saying the implementation is wrong or not ideal, try to understand and ask questions to clarify why it was done the way it was.

Opinions and preferences are alternatives, not the only alternative

Sometimes when you read a code you don't like and ask the author of the commit to change to your way, they might find your alternative as bad you think the code was. IF there is a clear advantage of your alternative, write it down clearly. You are working in a team, not alone, so your way is not necessarily the right way. Also, not always you are seeing the big picture.

Examples of alternatives with data:

some_class get_operation(read_only_conn &db, const std::string &operation_id) {
    return std::move(db.get_operation(operation_id));
}

Please remove std::move from get_operation to enable copy elision when returning the operation data.

Copy elision documentation from cppreference

Example 2:

some_class *get_operation(read_only_conn &db, const std::string &operation_id) {
    return new Operation(db.get_operation(operation_id));
}

Please use unique_ptr or shared_ptr as our policy doesn't allow to use raw pointer unless it's necessary by external dependencies or there is a considerable performance impact by its use.

Please see policy documentation

Big picture

Always understand the big picture, otherwise you're doing the work of a style checker/linter.

Given the code:

int totalize(const std::vector<int> &v, int pos) {
    int total = 0;
    for(int i=0; i < pos.size(); ++i) {
        total += v[i];
    }
    return total;

}

Examples of a bad review comment:

1. Use size_t for i instead of int as size_t might be bigger then int and overflow
2. Remove the whitespace at the end of the function

Examples of good review comments:

1. A bug is being introduced if you just totalize the vector without taking into consideration the size of the vector as very large vectors might generate a total bigger than the size of int. A suggestion is to return another type that holds a bigger value or either<int,error>
2. Totalizing the vector without taking into consideration the scaling factor will return a wrong value. You can either change the function to describe it's returning the unscaled factor of taking the factor into account

Conclusion

To wrap up:

1. Be sure you're not blocking someone else because of something it's your preference
2. Be sure to clearly expose in the comment it's your preference
3. Be open to discussions
4. Try to see the big picture and give a good advice in the CR. Don't be a linter. The person who wrote a CR worked hard for it. Do the same.

Tell me your opinion!

Reach me on Twitter - @thiedri