How to write a Code Review
Tags: codereview
Published: 2020-05-23 19:29:35

---
title: How to write a Code Review
published: true
description:
tags: #CodeReview
---

Source: http://thiagocafe.com/view/20200522_how_to_write_a_code_review

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:

```cpp
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](https://en.cppreference.com/w/cpp/language/copy_elision)

Example 2:

```cpp
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](http://whatever-is-the-doc-repository)


## Big picture

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

Given the code:

```cpp
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.