Code Review Best Practices

We know that Code Reviews are a Good Thing. We probably have our own personal lists of things we look for in the code we review, while also fearing what others might say about our code. How do we ensure that code reviews are actually benefiting the team, and the application? How do we decide who does the reviews? What does "done" look like?
In this webinar recording, Trisha identifies some best practices to follow. She talks about what's really important in a code review and sets out some guidelines to follow in order to maximize the value of the code review and minimize the pain.
0:00 Introduction: Reading other people's code is hard
5:21 What to look for in a code review
6:37 Different code review workflows
8:42 One Size Does Not Fit All
9:01 Code Review anti-patterns
18:49 Why perform code reviews?
23:07 When do you review code?
24:37 Who is involved in the review?
26:39 Audience questions
31:36 Where do you perform the review?
36:53 What do you look for in a code review?
42:01 How do you perform the review?
51:26 Summary
53:40 Audience Questions
About the Presenter
Trisha Gee has developed Java applications for a range of industries, including finance, manufacturing, software and non-profit, for companies of all sizes. She has expertise in Java high performance systems, is passionate about enabling developer productivity, and dabbles with Open Source development. Trisha is a Java Champion and is a Developer Advocate for Upsource and IntelliJ IDEA.

[@burhanuddinrashid891]

Thank you @Trisha Gee for explaining this in detail. I like the Why, When, Where, What and How parts.Its very easy to remember.

[@joshs279]

Very clearly explained, human-centered, and thought-provoking. Thank you Trisha

[@TheNaddaeus]

The lowdown is at: 51:26 - Have Clear Objectives Thanks Trisha!

[@eduardozaldivar2004]

Hi Trisha Gee, Thank you for sharing your book! Great examples.

[@jimigrok]

Thank you for sharing your knowledge - nice talk!

[@samir.4326]

Thank you for this! Appreciated

[@Andi-dp9hy]

Loved it. Thanks for the input

[@donnyjoe7653]

Thanks a lot Trisha. This was really exceptionally helpful for me!

[@JetBrainsTV]

You're so welcome!

[@DarkNivo]

Awesome stuff. Helped me a great deal. Thanks

[@Mohamed-uf5jh]

Thanks for sharing your knowlodge and book

[@Luis-xx1sh]

Thanks! those are very useful & well explained dos and dont's

[@JetBrainsTV]

Glad you like them!

[@davedave9]

Nice one, approved!

[@MrChannnnnnnnnnnnnnn]

very helpful appreciate your insight!

[@JetBrainsTV]

Thanks for watching!

[@sandeeptiwari_yt]

Anti pattern are real bottleneck for code review process at most organisations. Well Explained 👍

[@tamilmaranc]

too intersting thanks aa lot madam

[@carminello92]

Hi, where I can find the souce code?

[@JetBrainsTV]

What source code?

[@carminello92]

@@JetBrainsTV of the starting bad code example

[@TrishaGee]

I'd really rather not publicly shame the project! The code was completely rewritten several years ago and is significantly better, but I don't want to point to the old code as it's a bit unfair to the current maintainers of the project.

[@carminello92]

@@TrishaGee I ask that because I want to apply those principle with my team, doing a refactoring of a bad code and I thought it was a good starting point

[@TrishaGee]

carmine laface that's a great idea! There's a project I've been using for that called Morphia, the code is not as bad as the example in this video but is still old enough to have accumulated plenty of code that could do with refactoring. github.com/MorphiaOrg/morphia

[@Leon-Li]

I dont like those reviewers dont point out which part of the code needs to be changed and why they want it to be changed. It doesnt help me to improve at all.

[@user-ss3bl7ew3j]

"who write this code " it happen to me when i read my code 6 months later, and complaint a lot, why I write it that way.... 🤣🤣

[@peternagy3654]

Hi Trisha Gee, thanks for the talk just watched, this is the best talk about this topic so far I encountered and I agree with most of the points that you mentioned, but I have few questions/debate, I hope it's not too late to ask. You did not separate open source and in-house source code projects review process, do you believe they have the same code review requirement by nature? Like gateway type is necessary for open source projects to protect their code base, but in-house application team misusing/misunderstanding gateway type code reviews. You mentioned you like when juniors do the review, but they might be pressured to accept the change. And in my opinion, the reviewer needs to have a good understanding of the code/language/tool/framework to be able to review. For example, it's useless to review UI code by backend developers who do not understand the code/language/tool/framework used. So what is the point to ask juniors or someone who does not understand the code to do the review? Why do you believe if something merged to trunk it's already released/in production? It's like code review a git-flow specialty. I do believe one of the most important code reviews is the security review, that you didn't mention, and because most of the developers are not aware of all vulnerability, it's usually another team (security) needs to do together with the developer team. The review process is different when you have an existing code base or the development has just started. If it's open source project one commit needs to focus on one particular change, but in-house projects or the early stage of the development the commit doesn't have to be this focused. Inside one commit you might refactor something which is more efficient to develop a particular feature and segregating to different commits it's impossible. Do you agree? You mentioned the author needs to commit in small pieces to be able to be reviewed. Do you agree sometimes that makes even harder to understand the changes? and also makes even harder to implement? I totally agree with you, XP is the best practice to do code review, but do you believe team members who work together for ages still need the same review attention than someone who just joined into the team? What do you think team members can achieve maturity/trust level when they don't require review? Do you believe review, misunderstanding the review can be harmful to organizations? Thanks for your time, I'm really looking forward to your answers.

[@TrishaGee]

Wow there are a lot of points in there! Obviously in a 1 hour webinar there isn't time to cover everything. For example, I have a whole blog post specifically talking about what to look for if security is your main reason for doing code reviews (blog.jetbrains.com/upsource/2015/10/05/what-to-look-for-in-a-code-review-security/), I didn't mention it here as it is just one of the many many things you might want to prioritise. You've brought up a lot of different use cases, a lot of different team structures, development styles, purposes of review, and suggested that things would be different in many of these cases. So of course, I agree with you - the key point of the webinar is that the "best practices" you choose for your own code reviews are going to depend on all of these different variables, and understanding this is the first step to creating a good code review process There's only one point where I think I need to reiterate my particular point of view. You said "It's useless to review UI code by backend developers who do not understand the code/language/tool/framework used", and I don't agree that this statement is true all of the time, or even most of the time. Imagine you are looking at code for some area of the domain you don't know, or that uses some technology you're not familiar with. If you can look at this code and understand what it does, wouldn't that prove it's readable, maintainable code? It's not always going to be imperative that everyone understands all code, but if people unfamiliar with the code review it, this means a) you're spreading key knowledge within the organisation and b) you will be encouraged to write code that is more generally understandable.

[@peternagy3654]

Hi@@TrishaGee, thanks for your replay, really appreciate it. I just want to emphasize one more time, generally, agree with almost everything what you shared in this video and I'm very thankful for it. I found this is a good and an honest speech about this topic, covers a lot how I -- as a developer -- feel about code review, but I still cannot agree with you about this. I understand your point, maybe useless not the best adjective to describe when juniors/non-expert do code review. The intention to spread knowledge and write generally understandable code is a very nice and catchy aim, however, I'm not so sure if we ask junior developers to do code review can help to achieve that. In case of gateway type code review - on an open-source project, I hope you agree not to ask junior developers/non-experts to do code review. - on a closed-source project, I also don't get why should we do that, because - this is not the right place to pick up knowledge, (XP is way more beneficial to integrate someone into the team). - this definitely gives a very difficult position to both the juniors and the senior developers. The juniors need to confront someone who supposes to show the way to them how to be a good developer. Some of the senior developers might feel insulted because their commit is blocked by someone who does not understand the general concept. It inevitably poisons their relationship. For example, can you imagine when a junior developer who just heard that UncleBob said a good function should not be more than 2-3 lines, but definitely not more than 20 lines in any circumstances, and because of that he/she refuses the PR, in which a batch insert on an item list where each item has 10+ attributes needed implementing. - after several times the junior does not want to mess with the team and it became automatic to accept all of the commits. Otherwise, the senior developers get frustrated because their work is blocked. - I worked with teams where managers -- because they want to look competent -- did the code review, sitting on the PR for very long days and refused them because of a spelling mistake inside a private function variable name. Obviously, they never could address any real issue, because they are not expert. And I just don't want to encourage anyone to practice or accept this. In case of audit type code review also don't get what should we expect from juniors? Read through the code base on their own and create a ticket if they don't understand a function or a test case? Or even do the refactor? Maybe it's just me who can't see how it should work. Or my previous example without having any knowledge on the particular UI framework(s), HTML, CSS, javaScript, I still seriously doubt the feedback on that particular code can be beneficial for the team. Do you think it's understandable for someone who is not an expert but seeing these lines was added into a ts file? export function slideToRight() { return trigger('routerTransition', [ state('void', style({})), state('*', style({})), transition(':enter', [ style({ transform: 'translateX(-100%)' }), animate('0.5s ease-in-out', style({ transform: 'translateX(0%)' })) ]), transition(':leave', [ style({ transform: 'translateX(0%)' }), animate('0.5s ease-in-out', style({ transform: 'translateX(100%)' })) ]) ]); } So I totally agree with the aim, but I still have doubts code review made by junior or a non-expert can help to achieve that. I also see the advantage to have all tiny details in the code nice and clean all of the time, but I see the downsides too. Instead of doing code review which blocking the integration, can't it be just refactored the next time when someone opens that particular file and not happy what inside. You always have the opportunity to monitor every commit, if you are not happy with one of them sit next to the developer who made it and you can discuss with him how'd you like to make it.

[@zes3813]

wrr, no such thing as havex or criticx or not, do, say infix any nmw and any s perfx

[@sairajkalkundre2183]

The words "Generally Speaking" should be automated in this video as it was spoken 18times.

[@moy2010]

What a horrible language is Java :S

[@papanino4415]

If you hate code reviews so much why do a whole presentationon them? The speaker seems to be someone who takes critism of her code very personally so I'm not sure why she's even giving this talk. Besides, if the goal of a code review is to just approve code then why not skip the review altogether and push directly to production? Overall this was a terrilble presentation that only shows the speaker's hostility and bad attitde.

[@joachimdietl6737]

code reviews are a waste of precious time

[@cristianpallares7565]

Can't agree with you 🤔

[@joachimdietl6737]

@@cristianpallares7565 it is ok when you think different.