Front end developer specialising in JavaScript and React. Experienced in all aspects of modern front end development. Passionate about making accessible, secure and performant software.
Good question. Please share your top 5 as well afterwards :)
I try to structure the code review like this:
Is the code relatively good at a glance?
Does the code actually work (I run it and do manual testing if necessary).
Optional: Examining the code closer for smaller details.
Initial view of the code
The first thing I look at is: Do I understand 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
At this point, all is hopefully good. An optional fifth step is to look into little details of the code that can be improved. For example things like renaming a variable to a better name, replacing a for loop for map and 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.
For further actions, you may consider blocking this person and/or reporting abuse
We're a place where coders share, stay up-to-date and grow their careers.
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.