This post was originally published in my blog smartpuffin.com.
Code reviews are a great practice to apply in software development. The approach is very simple: when you're done with your code, give it to someone else to look at and leave comments.
Despite of its simplicity, it brings considerable advantages. Here are 7 reasons why this practice is useful.
1. Finding bugs
Another developer, just by looking at your code, may find bugs or forgotten scenarios.
No one writes perfect code all the time. A second pair of eyes is really helpful.
Even quality assurance engineers can miss some obscure cases which can easily be spotted by looking at the code with fresh eyes.
2. Finding problems
You can spot cases when it is not strictly a bug, but something still can be improved. Think questions like this:
- "This is not exactly a bug, but do you think it would be better if we did X instead?"
- "If there are too many elements, this method is going to be slow. How about we sort the items first and use the binary search? How about we use a hashmap, a heap, a graph, a set, a smart algorithm, an index on this field, a garbage collection trick?"
- "Maybe we should normalize these tables and not duplicate this data?" (Or, perhaps, the other way around.)
- "A test is missing for the situation N."
- "When the user does X, the system behaves in way Y. But maybe users would prefer Z?"
- "You implemented something that was missing from the specification, let's figure out what it should be."
Here are some tips about how to improve your thinking about corner cases.
3. Improving code quality
You might be using a static analysis tool, such as StyleCop for C#, Checkstyle for Java, or Lint for CSS. However, the tool cannot catch all problems.
For example, a reviewer can give an advice about a class name, or how to split the logic between methods, or how to separate responsibilities between modules or services.
Overall, the reviewer should be able to read the code easily. After all, we read the code 10 times more often than we write it.
A developer writing the code is so immersed into the context that they understand everything. However, after a couple of months all knowledge is lost, and your own code looks as unfamiliar as if someone else's. So making the code readable is for your own benefit.
And what is the better way than, you know, ask someone to read it?
4. Testing
The reviewer runs your code to see if it's working, and how exactly. By doing that, they may find some bugs. This is great because it helps you to publish a better product, you get the feedback faster, and the load on the QA is reduced.
5. Educating junior developers
It would be a mistake to think that junior developers cannot or shouldn't review the code of more senior ones. They are very much able to find bugs or problems with code readability.
But another important aspect is that they learn how to do things. By giving them your awesome code to review, you teach them on an example.
6. Sharing the knowledge
A team member, while reviewing someone else's code, learns about the part of the system which is being changed. They build a better understanding of the system as a whole.
And next time they have to implement a feature in that area, they are prepared.
7. Building trust
I know this one sounds a bit strange. How am I supposed to trust the colleagues who are going to criticize me?!
But hear me out. Code reviews, when everyone in the team is doing them equally, help the team members to feel like they are being trusted. It builds the sense of reciprocity. If you are asked to review someone's code, you feel like they respect you and your opinion. If you ask someone to review your code, you show that you value their input and are open to feedback and improvement.
You need to make sure to criticize constructively and not attack or condescend. If someone says to you: "that's a horrible code", no trust is going to be built for sure. It is demotivating and simply annoying. You are not going to want to ask that person anymore.
The feedback should be given in a kind and constructive way, and make sure (!) to always (!) explain the reasons. Absolutely do explain why you think it should be done in a different way. You found a bug? You found a problem? You cannot read it easily? Does it not play along with other parts of your system? Explain that.
If you can give a constructive advice about how to do it better, that's the best.
Do you do code reviews? What does the process look like? What advantages do they have for your team? Please share with me, I'd love to know!
Top comments (19)
Hi, nice article. Let me try some another approach. I saw a lot of benefits really similar when we are pairing in all development about the feature. So, and if the team pairing instead of to make the review process? This way the spirit/mindset of collective code code could be more shared by the team, and the feedback can be almost immediately without need to wait until of conclusion of the feature.
What do you think?
And, answer your question in my current project we are pairing and doing a code review. As we have a distributed/remote team with different timezones this way have been extremely helpful for us. As the code is a legacy usually we can have someone that already touched in that code looking on this and bringing up some new context about some decisions made.
In a mature team my feeling is that the code review is almost optional since we are doing the cards pairing and with pairing rotations.
Regarding the mature team, I personally think it is still worth reviewing (or pairing). It will still help to find bugs and problems and will spread the knowledge about new features.
If you feel pairing helps you to achieve all that, perfect!:)
Do you pair 100% of time? Can you tell me a bit about the process when you pair?
Hi Samuel,
Thank you for your reply! I never managed to pair-program for a prolonged period of time and consistently. But I guess it should be almost the same. Two pairs of eyes looking at the same code should definitely help.
On the other hand, pair programming 100% of time strikes me as, maybe, a bit too much? With code review you spend less time on reviewing then on writing.
Finally, I usually work in iterations, making my code work first and good in the end. I can imagine my coding partner to be frustrated by this in the beginning, and giving me feedback too early.
But again, I only pair-programmed on several occasions, so I don't have enough experience with it.
And what is your experience?
Sorry by the late message.
About my experience pairing in features and bug end-to-end:
Yeah, I really recommend and I could say that the benefits could be noticed fast; Obviously that all the team should accept the idea to try it.
If I could point to big themes should be:
=)
Thank yoi for sharing! I like the "our code" feeling.
I also wonder if you really measured velocity and saw it's higher? I imagined, based on other people opinions mostly, that velocity should drop. If you managed to speed up, it deserves a publication!
Yeah, I'll try write. :)
The count base where we could notice the velocity increase was points delivered of features vs bugs card in some short cycles.
It was not so evident in the first ~2 months about after that we could compare the numbers. It's a mindset change of the team but I truly believe worth it.
I have a recent really good example where I had pair with a team from another squad in a feature and the solution came up with an idea that I put on the desk and his as well. When we shared the solution with the team in a stand-up meeting that team was really surprised with the "out of box" solution.
Perfect summary !
In the "1. Bugs" section we can add security issue (a kind of bug rarely tested in the continious integration).
Another advantage to do code review is it require to make small (atomic) commits and focus on separate your massive code into a list of structurated commits. So we avoid mixing "typo fix" and algorithm implementation.
Sadly we don't do code review in my team but checking your coworker code can be done without allocated time ;)
Thank you, Jeff!
May I ask why you're not doing code reviews? Did your team try and cancel, or someone doesn't like it at all, or the company culture doesn't allow you, or the stage of the project, or smth?
In the first team, one week after I joined I started writting code, before my first commit my team leader started complaining about everything ("Remove this space", "variable must be used more than one time", "this loop is not performant", "this variable name is not good" ...). I through I'll get fired but I stay. Since this time I review my code before each commit. In this team we made review but it was not in the process, it was our personnal initiative (when someone worked on a big feature or had bug).
In the second team we had no quality (no unit-tests) so ...
In the third team (my current one) we "have no time", but I don't make this suggestion because some of my coworker love refactor every line of code they saw. I think it will not be as positive :$
We use GitHub Enterprise at work. So every Pull Request will be reviewed and merged by another developer and in the process they might first give review feedbacks as comments and post we incorporating them the PR will be merged.
Another thing that might help is when time permits go through PR merged by someone else how this helps is :
1) Understand what and how something is implemented.
2) A new pair of eyes looking at them. Might catch some problems missed.
3) sometimes learn a new technique to solve a problem.
Nice approach, thanks for sharing, Adarsh
I appreciate your emphasis on being constructive and kind with feedback. It can be intimidating to have someone assess your work, so it's important for reviewers to be mindful of how they respond. I completely agree that it helps build trust and a stronger team. Great post!
Unfortunately, I've worked on teams where reviews caused a great deal of tension. I think it's a practice that needs to be closely monitored, depending on the team personalities. I guess some people see it as an opportunity to try and stroke their ego. In some cases, it can poison the well for an entire team while simultaneously forcing productivity to a crawl.
I certainly can imagine this happening. If this happens, I see it as an opportunity for team members to grow personally, to become kinder, to realize other people are people too. I'm sure it can be worked on, if they want to improve.
It's cool as long as it's working like you described. I sometimes end up in situations like:
Or asking to improve pending code makes someone feel guilty. But hey, then we probably have far bigger issues than just code reviews.
You're right! I think if the team/company wants to do code reviews, there has to be a process, and everyone should be responsible enough to not forgo it:). Otherwise, they can end up not testing (psst! Can you pretend you tested my feature and it works?), or not working at all (psst! Can you tell our manager I was here today?).
I mean, it might sound like I'm exaggerating, but what's the real difference?
If, on the other hand, they don't want or don't need reviewing at the current stage of the project, for whatever reason, then it should not be mandatory.
Very useful.
#6 and #7 are my favourites! Great post, thanks!