Better Code Reviews FTW! - Tess Ferrandez-Norlander - NDC London 2024

This talk was recorded at NDC London in London, England. #ndclondon #ndcconferences #developer #softwaredeveloper
Attend the next NDC conference near you:
ndcconferences...
ndclondon.com/
Subscribe to our KZbin channel and learn every day:
/ ‪@NDC‬
Follow our Social Media!
/ ndcconferences
/ ndc_conferences
/ ndc_conferences
#code #softskills
Code reviews is feedback, but if your friend or partner or colleague would ask you for feedback on some of their work, you would probably not send them a list of 50 things they did wrong. Yet in code reviews, this is perfectly normal.
Could we do better code reviews by just changing the way we give and take feedback? I'll share 10 tips that I try to follow to turn code reviews from a boring exercise or something you dread to a much more pleasant experience.

[@user-bk9wt8gk1q]

The more talks and presentations i listen to, the more often i hear something along the lines of "you are doing topic X wrong, because you don't understand it well enough or have completely misunderstood it". Which is a really weird state that the world of software development is in. A field of work where being clear, precise and having a good understanding about your work is mandatory. On the other hand it's a comparatively young field of work and we don't have houndreds of years of softwaredev we can look back upon. 😅 A new junior software dev recently asked me what is expected for a dev to do in a code review and i couldn't give him an answer i was happy with (not a dev myself). This talk definitely helped me get a better understanding about what code reviews should be about. Thanks!

[@quantrox8191]

Whenever I see that someone tries to convince me that I am doing something wrong I know I am actually doing a very good job

[@michaelutech4786]

If finding defects is not a main objective but sharing patterns is, then what you end up doing is sharing defects, isn't it? I find it horrible that arguing has a negative connotation. Arguing is the process of exachanging arguments. If that becomes an unpleasant experience, the only reason I can think of for that is that the arguments do not serve the right purpose. If both parties have the same objective, exchanging arguments support that purpose and both parties should enjoy that. The fact that most often one side was right is a downside of arguing, but that can't really be helped. Then again, both sides have opportunities to learn, no matter who wins, transforming arguments into win-win activities. This goes wrongwhen both parties do not have the same objective. That creates a new objective which would be to examine respective goals and redefine them accordingly. The problem is not that there is an argument, the problem is that it's the wrong argument. The reason why reality does not align with my description is that most people are not interested in shared goals, they follow their personal agenda for personal gains. Sad as it is, drawing the conclusion "let's not argue" is probably the most stupid strategy to handle this problem, as it just accepts hostility as a natural modus operandi and not as something that has to be worked on.

[@michaelutech4786]

The more I listen to this video, the more it gets under my skin. "We don't necessarily want to get feedback". Well that's why there is such a thing as workflows enforcing code reviews. Some (actually all) developers make mistakes, and many of these mistakes have real consequences. Every time my mail address is leaked by a company that does not ensure sufficient security standards, I am upset, because I have to cope with even more spam. I am more upset when my credit card info gets leaked. As a customer, I really don't care about the feelings of a developer that didn't care about the quality of their code that leaked my credit card. I don't care for a team member that makes "my code" break in such ways. I want an additional pair of eyes look over that code and if they see something bad, I want these eyes make a report to the brain and then I want a mouth to speak up. Then I want ears to listen and hands to hack corrections into that code. What is all this being nice and politically correct stuff about, when the topic is how to write good or at least good enough code, such that people don't die when they get medical treatment or get robbed when they pay online? What kind of ethics is this?

[@PaulSebastianM]

Shared ownership where everyone shares ownership does not work because no one can be responsible for everything.

[@davidbottiau5408]

I got it not as "no one or everyone is responsible" but as each developer is responsible for the feature he is working on while having an interest as a team to focus on making the whole product better, achieving overall quality. Works great with small teams of skilled and/or motivated people.

[@user-bk9wt8gk1q]

The point beeing made is more that "as soon as the code is commited, it's our code not the code of person X; So a code review can help to ensure that another person can understand the code without studying it for hours." The person who wrote it could switch teams or companies in a few weeks and suddenly it's "the teams code" anyway. How does that work then if not everybody is now responsible for that piece of code? Does one poor soul inherit all responsibility of the code from person X if they leave the company?

[@chauchau0825]

Agree. In reality, it turns to Anarchism eventually

[@PaulSebastianM]

What I am trying to say is that shared ownership must be transparent. Most often than not it's sort of implied and never questioned or verified.

[@thomasbittner1009]

Thank you for the interesting talk about such an important topic. I have one addition, since I feel that many of the suggestions revolve around the root problem of having only limited chat-like communication in the PRs. So in our company, the ideal code review is not an asynchronous PR, but a synchronous review. It looks like this: 1) The author opens the (hopefully short) PR. 2) The author calls the whole(!) developer team. Whoever has times, joins in. Team members in meetings are excused. 3) A developer (not the author!) takes the lead and reviews the code. The team agrees on ToDos and comments them in the PR. The ToDos are distributed among the team. 4) After the review, the developers finish the PR by working on the ToDos. 5) When all ToDos are implemented, the PR is merged. Very seldom, there is a second code review. I feel like this alternative process tackles a lot of the problems mentioned in this talk. I know that the prospect of having your work interrupted multiple times per day is probably frightening, but I highly encourage you to just try it. We haven't had a team that ever wanted to go back from this.

[@penaplaster]

I agree with Tess that the heated argument in a PR is an indication of a broken development process. In our case, having an architecture/design session before the implementation solved majority of hard issues.

[@TechTalksWeekly]

This talk has been featured in the recent issue of Tech Talks Weekly newsletter. Congrats Tess! 🎉

[@michaelutech4786]

If and whenever I have to accept a change to "my" code that I perceive as defective, it's no longer my code. I don't commit defects into my code, not unless I mark them with FIXIT. For me, the term "ownership" either means literally that it's mine and I have the last say how that is changed, or it means that I am dedicated to it and then I really don't want it to be made defective. What kind of respect is it, when my view is catered to because I am a member of some group and not because it's right? That only means that my view is irrelevant. If it's wrong, than being irrelevant is a good thing. If my view is right, I want it to matter, because otherwise, I get no respect, it's the team that owns the respect, not me or the intrinsic value of my views. If I want to feel good, I go out with my friends. When I'm developing software, I want to see an excellent result, that's my work ethics. In that context, I don't care for the feeling of my team mates. If it's a good team and I'm a good team member, they are friends anyway and we're producing good code, not mutally owned defects. If I ever start thinking of a mutual defect as an accomplishment, I'll stop writing software and start growing tomatoes in a green house. If you need code reviews to keep external audiences up to date, then you do something wrong in your documentation or communication. That is not and should not be the purpose of code reviews. A code review should review THE CODE. Why would you do that if not to SEE if it's ok?

[@tacticalassaultanteater9678]

A very important recognition is to avoid using"you"and otherwise targeting the author, but there are grammatical devices to do this without swapping it out for 'we" which is so superficial that most people just mentally translate it to "you" combined with the most psychopathic HR smile they can visualize

[@tacticalassaultanteater9678]

The moment I read "can we" in a PR is the moment I shut up and submit whatever is necessary to stop talking to that passive-aggressive reviewer

[@brynyard]

Uhm.... what do you mean by "argue" if this clearly entails being very hostile towards each other?! We argue _all the time_ at my current job, but it is mostly always constructive. It's not about finding "flaws", but for the originator of an idea to expose his reasoning and for others to provide feedback and possibly input on this. So it sound more like you need to review how you "argue", and hearing that the mere concept of an "argument" for some is itself scary is a bit sad to hear.

[@michaelutech4786]

"How can we do [reviews] better, how can we not argue [..] and still stay friends" - Simple, just don't do reviews. Everybody with access to the code can review it on their own terms. They can nicely talk about things they like and ignore bugs they see. Or they can just be nice to each other and ignore the code nobody seems to care about alltogether. To me, these reviews look like a waste of time.