Programming

26193 readers

187 users here now

Welcome to the main community in programming.dev! Feel free to post anything relating to programming here!

Cross posting is strongly encouraged in the instance. If you feel your post or another person's post makes sense in another community cross post into it.

Hope you enjoy the instance!

Rules

Follow the programming.dev instance rules
Keep content related to programming in some way
If you're posting long videos try to add in some form of tldr for those who don't want to watch videos

Wormhole

Follow the wormhole through a path of communities !webdev@programming.dev

founded 2 years ago

MODERATORS

snowe@programming.dev

Ategon@programming.dev

UlrikHD@programming.dev

bugsmith@programming.dev

Spyro@programming.dev

Your thoughts on Code Reviews (infosec.pub)

submitted 3 days ago* (last edited 3 days ago) by mattreb@feddit.it to c/programming@programming.dev

48 comments fedilink hide all child comments

Hey, I want your opinion on code reviews, what is the best way to use them in a professional environment? Pick one of the following and give me your thoughts (from the most forgiving to the most strict):

no code reviews, they are useless
optional code reviews
mandatory reviews on code that is already merged, optional fixes
mandatory reviews on code before merging (like a pull request), with a time-frame for optional fixes (i.e. whether to fix what has been pointed out is up to the author), merge will occur anyway.
mandatory reviews on code before merging (PR) with mandatory fixes.

Of course in open source development with public contributions, you'll often see (5), but I'm not convinced it could work in professional dev.

Edit: I'm talking about a team of 5 mid to senior devs (no junior or interns) working on a 2-3 year project without many security concerns, but feel free to give me your general opinion.

you are viewing a single comment's thread
view the rest of the comments

[–] jtrek@startrek.website 7 points 2 days ago (1 children)

All code going to the main branch must have a corresponding pull request reviewed and approved by someone with knowledge of the codebase. You really shouldn't have the front end guy approving backend code.

Ai doesn't count as a code review.

At my previous job, the policy also said you were supposed to actually check out the code and run it locally. Found a lot of bugs and issues that way.

At my current job, it's often a rubber stamp. I've seen things like "that's too many parenthesis. This won't run" sail through. This is bad.

There should also be automated tests and checks.

A long time ago a director told me "software engineers are the most sensitive people on the planet" and I think he was right. Some people just can't take feedback. They take something like "please sort your imports. We agreed to use isort last week" as a personal attack.

[–] TehPers@beehaw.org 1 points 2 days ago (1 children)

They take something like "please sort your imports. We agreed to use isort last week" as a personal attack.

I would take this personally as well, to be honest. Using isort over Ruff? Blasphemy.

[–] jtrek@startrek.website 2 points 2 days ago

As discussed at length in last week's planning meeting, we agreed to continue using isort at this time. Here is the decision document to review: {confluence link}. If you would like to relitigate the issue, which I would not recommend, please add it to the tech planning meeting agenda.

(More seriously, I started using ruff and have no complaints about it.)