Hello folks,
I am always listening/reading about clean-code practices, dos and don't for certain programming languages etc. Though the base for everything is common, but there are many approaches one can take to write their code and all can be correct in some or the other way.
There are also many practices like DRY, KISS, WET which are followed by many developers. While doing code reviews, we do check for all these things in the code. Code reviews are super important as they help to better your code and even reviewer gets to learn new things.
So what are those things which you consider while doing a code review? What is your process for that?
Top comments (3)
Good question. Please share your top 5 as well afterwards :)
I try to structure the code review like this:
Initial view of the code
There have been code reviews I've done where, after reading the code for 30 minutes, I had no idea what was going on... (and it wasn't just me). We're talking god classes strongly coupled to multiple other classes with difficult to follow program flow and lots of public properties hardcoded with magic values acting as configuration.
If I can understand the code and the code seems relatively good at first glance,that's a very good first step.
During this stage, I consider what people would call "clean code" and programming principles. Things like whether the code is structured well, is easy to understand and is easy to change (e.g. doesn't have unnecessary strong coupling). If anything obvious stands out that could be improved, I note it down.
Checking that the code works
In places where I've worked, code reviews had accountability. If you approve a code review, you put your stamp of approval on it that the code is good and works. If bugs are discovered later, you are partially accountable.
So, if the code isn't a trivial change, I often run it and do some manual testing. (It's rare but, sometimes code submitted for PR doesn't actually work.)
I also check security and accessibility (and rarely performance), depending on the change.
I also look at the tests (the code), to see if they cover the expected use-cases.
Remaining details
mapand things like that.I actually try to avoid this step, because it can feel very picky and pedantic. If the overall code is very good, then these little details won't matter very much. Overall, it depends on the team. If the team cares a lot about clean code, then I comment. If the team instead says "ship it when it's good enough", then I might not bother.
Final notes
That was probably too much detail lol. If you were asking for something more specific to clean code, feel free to let me know.
I recently wrote a post on how to conduct a constructive code review.
I find that people tend to review the code in a change request and focus solely on the negatives, forgetting that any other kind of a review also highlights the positives.
Personally, I tend to start by looking at the tests as they'll give me more information about what the change is, how it might be implemented and why.
Then, I'll look at the files with the largest number of lines changed as I find these tend to be the files where the main implementation details are located.
As far as a process is concerned, I'll first check whether the piece of work is valid - I've seen changes being made far too often that either duplicate functionality elsewhere, or where the ticket prompting the change is incorrect in some way. Then I check if the code is relatively readable and that it works functionally. If these appear to be fine, I'll then look at edge cases that may not have been considered. I'll also identify any optimizations or ways in which the readability could be improved - however, I tend to recommend that these be put into a follow-up change request rather than preventing the fix/feature from continuing.
this is the order that i check stuff in:
1st: is there any errors
2nd: does it work efficiently
3rd: how easily can it be understood
4th: how could it be simplified
5th: can it be easily modified