How To REALLY Do Code Reviews

Follow-Up to • How To Do Code Reviews with many moooooore details on code reviews. Scenario: A user sent in a GitHub pull request for our Google Photos clone. Which means we have to do a code review. How should you do such review? What is or isn't important? Learn all about my "every line of code is a liability"-philosophy when reviewing pull requests in this episode!
► Timestamps
00:00 Intro
01:07 What Is a Code Review
02:48 Levels
05:26 Ego
06:13 Philosophy
07:19 Project Type
08:24 Location
08:59 Time
10:38 What We Will Review
11:51 Review Style
13:02 Inspecting a Pull Request
19:04 Giving Feedback
22:44 Outro
► FOLLOW ME
www.marcobehler.com
/ marcobehler

[@__Name_It__]

you haven't mentioned the case of two juniors reviewing each others' code 🤦‍♀ that's when the ego goes above and beyond. thank you for this video!

[@MarcoCodes]

That is indeed a "great" case :)

[@ahmedal-sharabi5322]

Great video

[@artellandoe-solucions2749]

I feel somewhat a bit of a picky prick doing reviews. First I read the comments, then take a light look to the diff, add the comments that come to my mind reading... And then I pull the branch, run some tests, maybe go back to the issue description... If I see too many things, I prefer to have a conversation/call with the colleague instead of putting a long list of comments that could be felt like a long list of denials. I think it feels better having a conversation. In fact, lots of times I am the one doing/testing something wrong, and that helps me to get the point. If there are some specific things to point out, I use the comment. Then, another look at the diff. And if other reviewers add comments, I review them, and maybe they could raise a new comment now that I am aware of something, new ... But for me, the most important/complex thing to deal with is when there's a draw: you have your point of view, and the other side has other. And the amount of reasons I had found is quite huge: - sometimes you cannot argue too much because the other side does not have the same depth of view than you have. And I´ve been there... I mean, when a domain expert is reviewing your code and you joined the team 3 months before, you feel like you are completely blind. But the reviewer is sooo busy that it is necessary to find a balance to make your addition profitable, and a step forward. In that case if I (as the reviewer) don´t see something that could hurt the project in the short term, and assigned tasks promise me to be able to touch it and refactor in the future, I choose to not struggle too much there and make the move a bit later. Because once that colleague can see the refactor (I will add her/him as a reviewer), she can get a new point of view, compare, and understand better the advantages and disadvantages, because he had worked on it, and is able to understand it. - sometimes you have different backgrounds, readings, schools, interpretations ... But both positions are fixed. In that case, I choose to let the evolution of the code give an answer. And maybe it is neither A nor B, but C. But surely it will say something difficult to ignore :D - If there's some kind of hierarchy, maybe I could think of asking to follow a strict guideline. But that's not something I like too much (except if there's something harmful). And for me it is more usual to have an horizontal team, with some exceptions (that is, those 2, 3, 4 ... people you need to be there when things start getting complex and you stop having answers for the questions). In that setup, for me it seems it works better using work as an example. You can suggest, and point to similar things you did earlier. And you can learn from the things your colleagues have done when you review their code. And it is more enriching than some long discussions without a clear winning argument. I hope it could help someone, or raise some interesting comments to learn from :)

[@MarcoCodes]

Thank you for your comment and those thoughts - really!

[@artellandoe-solucions2749]

@@MarcoCodes Hey, it is your fault! Don't post videos about such interesting matters :D

[@djxak]

Thanks for the video! And please don't consider comments to the previous video as "mean" or "rude". Please follow your own advice about ego. Those comments basically are "Code review" of your **video** and not attacks to you as a person. I can't say the previous video was bad or not useful. It was great. Just **named** incorrectly. I (and considerigng the comments, many others too) expected to see how to properly do a code review (based on the video's title), but instead saw how to do a refactoring. :) That was also very entertaining, actually. I watched to the end. This says a lot. So, please don't be frustrated by these comments. It was "righteous anger" from developers (including me), who still beleive in unicorns and in existence of easy one-size-fit-all way of doing a code review. :) The title deceived these expectations. Like these titles of news in yellow press, when title is specifically crafter to be clicked, while has nothing to do with the contents of the article. And I honestly understand how this happened. It was really much easier to do a refactoring yourself (the 3rd way of "code reivew" from this video :-) ) than do a long and very time-consuming and also stressful and frustrating (because it's hard to explain in words something, that should be explained in a code) review. And also because, as you said, if you would not do this refactoring yourself from start to finish, you will not come to solutions you came in the previous video. It's hard to see all possibilities to improve based on the 1st version of the code. Usually it's a iterative process (so, the ping-pong). But who has **that much** time for code review of such small feature? :) And yes, this is very small and simple feature. They are not always that small and simple. :( And divide to smaller chunks is not always possible too. The content of **this** video is much closer to the title! :) The thought about dependency of the style of code review on the type of a project was very interesting. Yes, it's obvious, but I never thought about it, just did intuitively. Anyway, keep up the good work!

[@dinov5347]

Have to agree with your observations here. Not sure what take aways from the last video for code reviews and making fun of the feedback was a little weird. It is very difficult subject because like Marco in most cases I want to refactor code every time when I review code. A better approach is probably pair programming but that too has its problem.

[@MarcoCodes]

Thanks for your elaborate and thoughtful answer, Ruslan! Just so we're all not overthinking it and can relax a little: I didn't take the comments as personal attacks, my lines were merely there to lighten up the video/make it more YT engaging with a bit of smirky-ness, nothing else :) In any case, I might just finally rename the previous video, any suggestions for non-yellow-press titles? :)

[@djxak]

@@MarcoCodes I suppose, it is too late for this at the moment. :) This video is named after the previous video and if you will rename the previous video, you will need to rename this too.

[@qwerty-hc7od]

@@MarcoCodes I propose to rename previous video to "How to start code review in Intellij Idea (+ refactoring example)" or somethins like that. This name contains main keywords from the video and it will help you to avoid the same comments over the next few years)))) In general, thank you for your videos. I like how you explain every single thing you do. :)

[@theceilidhboy]

How about a 4th option: add a comment with your suggested changes using ```suggestion...``` and ask the pull requester to consider committing these changes to their PR branch?

[@MarcoCodes]

Yup, also possible.

[@aicantar]

Nice OfficeJet :)

[@MarcoCodes]

Well spotted ;)

[@cypherliquid]

Alive?

[@MarcoCodes]

Yup, alive and incredibly busy :D next video is in post production already. Dropping soon

[@cypherliquid]

@@MarcoCodes take your time we love you

[@deconcoder]

This is not about code review??!! This is about social pathology. Who is on top of who, how to manipulate people without having the object go ape on you. I bet this guy's reviews are full of patronizing nonsense. Sorry gonna go unwind by rereading Bentham's piece on the Panopticon...

[@MarcoCodes]

Were you high when you wrote this? :)

[@deconcoder]

@@MarcoCodes sure funnyman. Go read a real source about psychology then maybe your zeal for the dimestore variety here will wane.

[@KrigRaseri]

@@deconcoder Someone is being a sad lad.