At some point in your developer career you'll have to do a code review. It seems everyone does it differently. I spend about an hour on a code revi...
For further actions, you may consider blocking this person and/or reporting abuse
My suggestion for a thorough code review of any given PR/MR would be:
Some of this can be done quickly for small PRs, other times you may need to do 5-15min spurts of review as you have time.
Then approve and merge that sucker in!
I'm not particularly structured in my approach, so you'll get more relevant answers from others, but here's what runs through my head....
In my particular role of being fairly big-picture-oriented, I tend to ask whether the approach taken is in compatible with future needsβTests certainly play a big role in this, but asking whether the next thing built with this new code as a dependency will work is important.
Sometimes even the worst code style is perfectly fine if it's hidden behind the right interface and it's not going to result in any major hidden gotchas after we forget about the problem being created.
Ah totally agree with this.
It's something I always referred to as: problems nearer the "leaves" of an application can be more lenient, but those closer they to the "trunk" of the application, require greater strictness.
This is awesome advice! I've always treated code reviews in isolation, definitely going to start thinking about the big picture in my next reviews.
Outsource all style issues to a linter that runs in CI. You don't need to spend brain power worrying about bracket placement.
Aside from any checks of the complexity and maintainability of the code, the PR itself should have some context as to what problem it is solving. I recently rejected a one-line change PR because it was setting an environment variable and there was no context in the ticket or PR about why this was being set or what problem it was solving.
Each PR needs to strive for greater maintainability of the code. They need to tell a story about what is being solved. Without exception, any PR that isn't solving a problem is a waste of time. "Updating README" vs. "Updating README to make it more clear how upstream libraries interact with this API." Now there's a story, there is context for the code changes.
Yes, your review will be much more valuable the more robots can do.
I think we should occasionally evaluate the robots and see what you can add to the linter/tests or take away too though; and no better time than code review to do that.
Couple of things I usually look out for:
From a personal standpoint, I have an additional question: how deeply should we be looking out for business logic here, or should that be done during manual testing/QA?
Many good things have been addressed in the comments. A few things I usually take into account when reviewing are below.
For the code:
save()with a comment βsave the entityβ).NoneorOptional)TODOs in the code not addressed with a ticket to be worked on laterFor the ticket:
If by style, you mean code formatting, try to automate it. I mean the formatting. In the long run, it takes less time than fighting over it.
In a code review, probably the most important thing is to make sure that it is always about the code and never about the person. If you have that, you are on the good path to meaningful discussions.
Hopefully other people in the team ask themselves the same questions you asked and replies should be noted and probably they should be part of the code review guidelines in your teams.
I elaborated a bit more on this topic in a post, you might find it interesting:
Sane office environment with code review guidelines
Sandor Dargo γ» Mar 28 '18 γ» 6 min read
If I'm very familiar with the code in question, I approach the changed code in the review with a critical eye with how it changes the behavior.
If I'm not familiar with the code in question, I do what you listed: style issues, code placement, lament over functions that do way too much. (We basically don't have tests. But, yes, there should be tests. My code base has plenty of awful non-linear functions that are between 10,000 and 100,000 lines long, with cyclomatic complexity that can only be properly expressed with transcendental numbers.)
Management believes that code reviews will transmit subject matter expertise through osmosis. But in my experience, code reviews do nothing of the sort except give management a warm fuzzy that they are facilitating and encouraging knowledge transfer.
To get knowledge transfer, you either need to be in a mentor/mentee situation or pair programming. Both of which also entail continuous code reviews.
Also, if the code review is small, I can assess it well. But as it gets larger, it becomes difficult to see problems except at a gloss level. I saw one tool (from Google, iirc) size code reviews as wee (1), tiny (<5), small (<30), medium (<100), nominal (<300), large (<1000), freakin' huge or mondrian (<2000), jupiterian or jovian (<3000), month long (<4000), whopping big (<5000), mother of all code reviews (<10000), grandmother of all code reviews (<20000), category 5 code review (anything larger).
Where "gloss" are things like suggesting better named identifiers, misplaced tabs if the team convention is spaces (or vice versa), misplaced curly braces from the team convention, suggesting a breadcrumb comment for some confusing logic. Those are basically superficial things.
Automate everything that you can, then code reviews can focus on code design, complexity, maintainability, etc.
And, very important but too often overlooked, look for what's not in review (shouldn't that other file be changed too? why did you change this and not that? You're adding an implicit relationship with some other part of the code --e.g. a mail template links to a page of the app without using a shared router to build the URL, possibly because the router is clientside in the SPA--, then please add comments both sides so we can remember to change the other in case we change one, etc.)
I decided to repost here an article that helped a few developers in the past.
This is related to Pull Requests but it may add value to some of you too!
A few tips:
I'll be interested to see other responses!
One thing that did work for us at my old job was to modify the pull request template in GitLab to have a short list of questions that the user should have good answers to before submitting.
Some of it was as general as, "Did I write documentation?" or "Did I write tests?", and some as specific as, "Did I use one of the deprecated functions in new code?" (We had some ill-conceived abstractions early on, didn't have the time to clean them all up, but didn't want to pile on more technical debt.)
We mainly wanted to head off some of the non-negotiable stuff that were hard requirements of the contract. Better to catch yourself not writing some unit tests now, than attempt to write hundreds of them months from now.
For me, I found that I'd spend that extra bit of time making my pull request look good, knowing that I'd encouraged people to give an honest review.
I think your post is focused more on the tactical details of how PR's are accomplished, but I just wanted to share that the whole process of PRs engenders enough frustration in me that I felt compelled to write an entire post about it not-too-long ago:
dev.to/bytebodger/the-contentious-...
Written about this a few weeks ago:
Things to look at when doing code reviews
Alexandru-Dan Pop γ» May 23 γ» 1 min read