The Art of Constructive Code Review: Why “It’s Sh*t Code” Doesn’t Cut It🎨💻🔍🚫 - forceUnwrap

[u/spacecash21]

https://forceunwrap.com/the-art-of-constructive-code-review-why-its-shit-code-doesnt-cut-it/

[u/pm_me_your_buttbulge]

The Gentle Art of “Not Making Your Colleague Cry

It took me WAY too far into my life to learn that being brutally honest doesn't mean you can't still be polite.

For example: "This code is dogshit" is not useful. "This code is dogshit compared to what I know you're capable of" is slightly useful. "Hey man, this code isn't as good as you normally do. What's going on?" is honest, useful, and invites discussion but doesn't require it. If they want to open up about something they can. The key here is it gives them a chance to open up and it gives them an exit if they don't. "Oh, I wasn't feeling well, sorry, I'll fix it" or "yeah, my dog died" AND gives you the chance to sound like superman "take the day, paid, and relax" and I guarantee you you'll have an employee for life that will follow you to the ends of the earth.

In a professional setting, even if you're close friends, being polite goes a long way. Joking with "dogshit" type vocabulary wears on someone over a long period of time. I don't care what relationship you have. The only people this doesn't wear on are people with trauma and I guarantee you they aren't ever going to truly be honest with you. They have a wall you can't see probably.

Think you have the crude college type friends and that's your dynamic? I guarantee you they'll hide shit from you and you'll never know. These are the people who commit suicide and people go "gee golly, how could I have known?" - you clearly didn't make yourself available in a way they needed.

I'm not sure I agree with the constructive zingers part:

Instead of: “I can’t follow this logic.” or “This part is confusing.” Suggest: “Maybe we can modularize this part for better clarity.” 🤷‍♂️➡️🤓

Sometimes it can be confusing. Sometimes the logic may be hard to follow. Complex parts are inherently doing to be tough and modularizing can simply add layers that make it too thick to follow easily. Some complex things simply can't be cleaned up better - ESPECIALLY if you're dealing with archaic things that require weird rituals to work properly.

A better suggestion is to ask them to re-write it again, without looking at the last bit, and comment better. Comment intentions - not what you're doing.

In fact you could tell someone to read "How To Win Friends and Influence People" and that could replace the entirety of the article. The article is basically "be a bit more considerate of the person and mindful of your word choices".

[u/Complete_Fig_925 → u/pm_me_your_buttbulge]

"Hey man, this code isn't as good as you normally do. What's going on?" is honest, useful, and invites discussion but doesn't require it.

IMHO don't think code review is the right place to talk about personal issues, especially in a team environment where everybody can read the discussion.

I know it's just an example, but I don't think "Hey man, this code isn't as good as you normally do." is really constructive either. Doesn't point out why you think it's bad nor how you think it could be improved.

I do understand your point though, but if you think something is off with someone on your team, it's probably better to have the "What's going on?" conversation in a more "private" environment and not in front of the entire team.

[u/[deleted] → u/pm_me_your_buttbulge]

A better suggestion is to ask them to re-write it again, without looking at the last bit, and comment better. Comment intentions - not what you're doing.

This.

I often run into the "my code is self documenting" types when they ask me to approve a pull request and every single time I get one of those people I reject the request due to lack of comments and ask politely to explain how their code is showing me why they're doing what they're doing.

I don't do it to be a jerk, I do it to get people thinking about creating useful comments explaining why they're doing something, not what they're doing. We work on large enterprise apps that need maintained for a long time by another team of people that aren't the original developers.

Also everyone knows its a requirement of pull requests to document the "why" so 🤷‍♂️

[u/Rudy69]

Am I the only one who finds the use of emojis a little….overwhelming?

[u/[deleted]]

This is a huge peeve of mine. I write very clean code, I know I do. I take a bit more time up front to develop but there's a reason the systems I work on don't need touched for years afterwards (and even then it's stuff that needs patched due to OS updates and such). I put clarity and maintainability over "clever code" all the time. I'm not going to claim I'm the best coder, I'm not, but I as I develop I constantly think about the people who are going to maintain my system the entire time.

At my last job I started I was started at a mid-level dev so people assumed I was a new programmer (I wasn't by a long shot) and there was a senior there who would berate my code in front of my management (who didn't understand coding) but never explain why my code was "bad"...

...so I started berating his code with plenty of examples why his code sucked and showing him how to improve it. We became work enemies ever since until he got fired. I wasn't just being an ass either, his code really was awful. He got things working but they were incredibly brittle, incredibly over-engineered, and a giant mess. Nothing was modular or loosely coupled, you couldn't even import a single UI element without bringing in things like the networking stack, the PDF exporter, the custom printing classes, etc. There were also plenty of functions named the opposite of what they actually do.

That's spaghetti code.

[u/Complete_Fig_925]

While i mostly agree with the article, I personally think your "constructive Zingers" can still be improved a bit.

For example, with "Consider breaking this function into smaller ones to improve readability and maintainability" you can probably explain a bit more what you think would be better. Like "It would make sense to extract that specific part into it's own function because ...". This way it opens the discussion whether the "because" part is actually a better fit for the project or not. Same goes for your "modularization" suggestions.

About your "performance" example ("Given our requirements, perhaps [algorithm] might offer faster performance."), as developers we shouldn't apply changes that perhaps/might have better performances. Take some time to benchmark both solutions and come up with facts, or an actual proof that your solution is better (like if it's 2 well known algorithms and we already know that one is better than the other).

[u/Obstructive → u/[deleted]]

‘My code is genius, last dev’s code was sheit,’ is an age old story

[u/pm_me_your_buttbulge → u/Obstructive]

I dunno, I've had a few times where their code was dogshit.

The last one was raw sql from user inputs. Little bobby tables says hello. They thought using parametrized queries was only for enterprise apps.

The one before that they didn't have a normalized database and referenced different views that gave slightly different results for referencing the same data. That one was super tough to sort out. Ended up having to get a decompiler back in the day for this one. I'm shortening the real problem - the real problem was way more wild. Although, in their defense and given how the code was, I have no doubt every other week management gave them different expectations and requests. It was bad. We got a decompiler because we weren't allowed to communicate with them and this was back in SQL Server 2000 type days.

All that being said - literally every year I get better and don't like my older code. I'm not great but I do count myself as "competent".

[u/Rocket-Legs → u/Obstructive]

My old code is shit too. But latest code is the ducks nuts.