Better Code Reviews in 6 SIMPLE STEPS

[@sjfarrell2.0]

I never nitpick on code reviews. I only care about the code doing what it is supposed to do, being simple to understand and easy to modify if the need arises. Unfortunately, most devs I work with are so pedantic about things that are meaningless, and have no impact on the problem being solved. And then you just get into pointless args because people have different opinions. It is a drain on the soul.

[@claasdev]

Being simple to understand and easy to modify is already something where opinions differentiate wildly

[@travisabrahamson8864]

My general starting point is to look at the build report from SonarQube/Lint, I realize this is an automated part but the goal is to help train devs to use these tools before a PR is requested. Second I look at what they are trying to solve and see if the tests reflect that. Next I look for performance issues or missed logic such as potential state that is not being handled that should be.

[@bjorn1761]

A different note: Ownership: what happens with the code after someones done? When technical debt or bugs appear because of the bad implemenation, who is going to fix it? Very probably not the original author. In my experience, better code and design result in minimal technical debt and bugs, so less time and people will spend looking at better designed code. But that also means less people will familiar itself with better design and code. Indeed regular pair programming between junior and senior could be a better alternative. Only thing I struggle with is some juniors without a CS degree might miss lots of basics, like the language itself, datastructures, ip networks or design patterns, which makes teaching them in a pairway very time consuming and expensive.

[@a544jh]

PR style code reviews in a team setting are so overrated. My brain just can't do that. Live pair reviewing is so much better. Gatekeeping also implies a lack of trust in the team. In my opinion, having your code reviewed is something you have the privilege to ask for, and not something to be forced upon you.

[@RogerThat902]

I would say the only thing I "nitpick" about is naming. I hate when someone has a misleading name or shortens a name by 2 characters instead of just spelling it out. I do it because anything that can add friction to reading/understanding the code is something to be called out. For example, it may be obvious to those experienced in the code, but would a new team member not versed in the terminology understand it? But here is also the thing, I always make a suggest or 2 on a better name. If I can't come up with something I will let it go. Nothing worse than someone complaining about something but not providing guidance on something that would satisfy them.

[@ErnaSolbergXXX]

Code review is basically a lack of trust that the developers can deliver a good enough work

[@Jollyprez]

I don't normally mind code reviews, but I DO hate it when the reviewer merely has a different opinion on something. More than one way to skin cats, as they say - but the reviewer will accept only his/her way.

[@trappedcat3615]

Agreed. It is a waste of time. I don't mind it if it is only a suggestion and appreciate different perspectives.

[@jerekarppinen]

This is an important point to make. It takes some experience as a reviewer not to nitpick about things that don't really matter, and focus on things that make code more readable.

[@toms7114]

If there is a style guideline, or design guideline, and they aren't being followed I can understand nit picking to getting the code back into those standards. But if the code is within those guidelines and works, leave it alone.

[@georgehelyar]

It depends whether there's actually a good reason behind their suggestion. Anyone reviewing the code should say why they are suggesting the change, and the code author should be able to say why they did it the way they did it. With mutual respect you will generally come to an agreement pretty quickly. The problem comes when someone doesn't explain their why, or doesn't want to discuss it.

[@bjorn1761]

​@@jerekarppinenit is all relative: from my point of view: it takes lots of experience as a reviewer not to nitpick about things that don't really matter (like formatting, readability etc) and focus on software design issues as they are pretty hard for juniors and mediors (even most seniors) to grasp and hard and extremely costly to change afterwards, and causes most logical hard bugs.

[@Rcls01]

Yes, yes and yes. I also hate code review. I hate it when it happens AFTER everything is already done, because no one from the team was willing to pair up or wanted to talk design beforehand as 'we all got stuff to do so let's just plan the sprint and disperse'. Then it sits for a week in the PR queue, sometimes going back and forth and you bothering people in Slack to review it. It's such a waste of time! Often enough I open up a draft PR to let people look at it before I submit it fully for review, so I can correct it early, but most of the time those are just ignored. It's so painful

[@trappedcat3615]

Positive feedback is a great tip!

[@tiagovdberg]

As said many times in this channel. Pair programming is the better and faster code review.

[@DineshkumarPuli]

@@tiagovdberg yes, developers would generally agree to this, but management would see it at 2x man hours for the same task. 😕

[@Dalamain]

The hardest thing is reminding yourself if the particular critique is genuine or an opinionated thought.

[@nickbarton3191]

Most of us know what bad code looks like, for example deep nesting, long files, magic numbers. Static analysis tells us most of this. I start with the unit tests. I look for tight coupling, and not coverage. Does the unit test, test by contract and just the characteristics of the requirements? If not, any tiny change breaks a bunch of tests. Do the tests show how the component should be used, by example? If not then the programmer missed the point. Pairing assumes that there is someone in the company that I could possibly pair with, adequate domain, technology, skills etc otherwise it'll take a month of Sunday's.

[@MarcDoornik]

Don't be polite because bad code will become legacy code and will never be removed. If the code is bad there will be another iteration for it to be improved.

[@NicodemusT]

And nothing bad happens, then 1 day later the company gets bought out by another company and completely wipes all of the work, regardless of how well it was written.

[@dinukweerasinghe7686]

At its core, code reviews happen to inculcate a “homogeneous group think” amongst a heterogeneous mix of developers. It’s easy to spot this; as time grows, the team develops a sixth sense of how to do things that incur the least objections, and code reviews start to be less of a bottleneck in the workflow. This, of course, requires ‘stability’ of the team; rotations, attrition, new hires change this - which is why you see the interview stage exhibiting these concerns (of finding someone who fits with the tribe). Alternatively, open sourcing the code is one way that companies find new talent that can integrate seamlessly into their teams. Outside of this, automation (in-house tools, assets, frameworks, reference patterns, libraries) is the other way where getting developers to write less code reduces the code review bottlenecks.

[@TNothingFree]

Many great tips!

[@muhammadharris4470]

Would love to see a conversation around AI and its impact on code reviews. Most code would be written by code assist tools in the future. Maybe AI would do some sort of code reviews given a set of checklist and have a human as an additional and less frequent review

[@mikakoivusaari4546]

Formatting should not be an issue, one set of formatting rules which IDE does automatically when saving, specifics should not be important but all the code should look the same.

[@ralify]

Asking questions is also a polite and good way to give comments

[@bjorn1761]

I do not completely agree: if a developer does not understand the code/vocabulair of an other developer, then that code still might be good. Most probably the less experienced doesnt have the knowledge yet what effective code is or how to design software in the first place. Examples might be things like when and how to use callbacks/lambda's. Or how you should design and construct thread-safe code.

[@NicodemusT]

I can't wait for the day when we cut the bloat in development. All of these reviews, process and testing every single thing are all viruses in the creative process. Programmers have forgotten that programming is often art. I wouldn't paint a single canvas if I had to endure creative review based on speculative feelings and vibes. The people who advocate for these things are followers, not innovators.

[@MarcDoornik]

@@NicodemusT Just go of on your personal project. Maybe develop a new js framework.

[@ILyaCyclone]

Can't do that in a team. If one wrote a code, that works, but noone else in the team can understand it (because how creative or how bad the code is), this code must not be merged. Or else it will have to be wiped out by the next developer.

[@NicodemusT]

@ It's funny because in 1998-2005 that was literally the case and it worked fine. If you're going to speak from authority, try investigating what other generations did and ask yourself why it was possible before but not now.

[@marzuraanmarzuraan1226]

You can tell she's done lots of code reviews by how much grey hair she's got 👵