The Code Review Checklist
-
Comments posted to this topic are about the item The Code Review Checklist
-
Nothing formal, no.
I want to move to a Git-based workflow, so we can utilise pull requests and authorisations. That's a goal for the next 12 months. We currently use Redgate's SQL Source Control, but with a central repository using Team Foundation Server.
-
This is a very timely article because I foresee us possibly adopting code reviews soon. At this point we've never had a formal code review process. In fairness to my employer, I've never worked anywhere that had a formal code review process. We're going to a Git based code repository system and each such system I've encountered all have a pull request paradigm built in. I'm both looking forward to it and dreading it at the same time. Your description of how you'd pick someone to review your code made me realize that I'd do the same approach for the same reasons. I'm eager for the chance to learn from others. But I've got more reservations about adopting code reviewing.
The first concern is that only a few people will be stuck reviewing code. One bad habit I've seen around here is people quickly identifying something you do, normally the last thing you did, and then they only assign you the same tasks over and over again. We recently had a training with a Microsoft Developer Advocate on branching and merging Git repos, who insisted that it is best to share the responsibility around everyone, so everyone gets a feel for the whole code base, rather than the portion they work on. This is something I'll push for.
The second concern I have is how do you avoid not being judgmental about the code you review. I know myself, I can be judgmental. And I don't really have an answer to how to review code in a psychologically safe manner.
I'm hoping we adopt code reviews and learn how to do them reasonably well.
Kindest Regards, Rod Connect with me on LinkedIn.
-
This hits home for me.
About 11 years ago, not long after I started working for my current employer, the head of development decided we would start instituting code reviews. I thought this was a great idea. He gave me the first assignment to review the code of the new guy we hired. He used to be a consultant and was used to throwing some code together to solve a problem and getting out. No one ever reviewed what he wrote, and he felt his work was gold. We all figured out pretty quickly that he was very touchy about anyone commenting on his work, so I was ready to handle his review with a very light touch. He had never written C# before so I thought I could just give him a few hints and let it go at that. Instead of creating functions for code that he used over and over, he just copied and pasted the code. Great, an easy suggestion. Can you turn these pieces of code into functions and call them instead of copy and pasting them? This piece of code goes on for hundreds of lines. It would be easy to break this into smaller pieces that would be easier to test and review. There were lots of little things I could have gone into, but I thought I’d just give him a few easy things to fix for the first review. I also made sure I complimented his work and was very careful with how I phrased my suggestions. That didn’t help. He stormed off to the head of development to complain about how horrible this was, and he was formerly a big-time consultant, and this was terrible treatment. The head of development and I worked together at 2 other companies, so he knew me and knew that wasn’t how I work. We talked about what our new hire said, and I told our boss what I said during the review and showed him the code. My boss would have made the same comments and a lot more because he’s more direct than I am. This was the first and last code review at my company.
-
Tom - that is the perfect example of WHY code reviews SHOULD be done - the reaction that individual had would also be a good pointer to go and review all his prior code - as well as having some training sessions to have him learn better ways of doing things - those unwilling to learn are not good for any company. (although those unable to learn need different treatment).
In most companies being a new hire would likely result in "do as we request or leave the company" as a consequence of what he did.
Code reviews, like standards, exist of some very good reasons - on the particular example you mentioned one of the important aspects is maintenance of code - whoever needs to take on that code to fix will have a hard time trying to change it all - and if it is someone good at what they do, they will likely end up rewriting it as it may be faster than fixing the existing code.
-
Frederico - I absolutely agree. I would have loved if we continued to do code reviews instead of caving in to someone who's ego was bruised. Now we're so understaffed that we couldn't do that if we wanted to.
-
If you're trying to get started, one thing I'd recommend is make a REPO to keep your code review checklists. You can alter/change those as well with a PR and manage them, but it's nice to have a list of things your environment cares about for C#, SQL, Java, etc. Maybe even code checklists by application if that helps.
When you find bugs or problems slipping through, try to update the code review checklist.
-
Steve, please excuse me for being obtuse, but what did you mean by, "... one thing I'd recommend is make a REPO to keep your code review checklists"? I'm wondering if "REPO", in caps, has a special meaning?
Rod
- Doctor Who 2 wrote:
Steve, please excuse me for being obtuse, but what did you mean by, "... one thing I'd recommend is make a REPO to keep your code review checklists"? I'm wondering if "REPO", in caps, has a special meaning?
I'm sure he simply meant 'repository', given the mention of a PR in the subsequent sentence. I suspect the caps were for emphasis only
The absence of evidence is not evidence of absence
- Martin Rees
The absence of consumable DDL, sample data and desired results is, however, evidence of the absence of my response
- Phil Parkin -
Ah, OK, thank you, Phil.
Rod
- Phil Parkin wrote:Doctor Who 2 wrote:
Steve, please excuse me for being obtuse, but what did you mean by, "... one thing I'd recommend is make a REPO to keep your code review checklists"? I'm wondering if "REPO", in caps, has a special meaning?
I'm sure he simply meant 'repository', given the mention of a PR in the subsequent sentence. I suspect the caps were for emphasis only
Repo as in respository. We have lots of repos at Redgate, some are for docs or other things we track, like ADRs (Architectural Decision Records). I have multiple ones in SQL Saturday. such as one for tools, and one for old data.
Repos are cheap. They're a good place to store things like a series of checklists. Add them as a text or MD file in the code area.
-
Redgate SQL Prompt has several features that are useful for code reviews, automated static code analysis, and refactoring.
First, there is the 'Code Analysis' feature.
https://documentation.red-gate.com/codeanalysis/code-analysis-for-sql-server
Then there is the feature to find invalid objects and unused parameters and variables.
Also, a little known feature called 'Summarize Script', is something I use to help make sense of a 10,000 line monolithic script or legacy stored procedure.
Then of course the SQL formatter.
"Do not seek to follow in the footsteps of the wise. Instead, seek what they sought." - Matsuo Basho
-
Where I work currently we have to have two approvals on every PR. Not only do we review the code but also are expected to test the changes - the documentation prior to submitting a PR includes testing steps but we are free to test our own way. Our application is on one repo and the database code in another so sometimes we have a PR in each repo that go together. I am fortunate to work with a team that is very accepting of comments on their PR's. There is a strong desire to have the code follow certain standards so you sometimes get comments that some might think are nit-picky but in the overall scheme works quite well. I am one of the newer members of the team (but with more experience than most of the team) and have been able to get my code to match the 'style' used in the application and by the rest of the team. The team is also very amenable to doing things a different way when presented for good reasons for doing so.
All in all, I find the PR review process on my team to work very well. It does take a bit of time to accomplish but we all feel it is worthwhile to releasing a better product. It also helps minimize having to rework a recent change because something is missed.
Viewing 13 posts - 1 through 12 (of 12 total)
You must be logged in to reply to this topic. Login to reply