Programming

26193 readers

287 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

[–] gsv@programming.dev 5 points 3 days ago (1 children)

From a scientific modeler perspective: Always trying to do 5 (or 4), but I’m having difficulties getting a culture of reviewing each other's codes going. Many times I was asked to “just merge” months after submitting a PR. In the context of operational or large community codes, 5 is usually strictly enforced. Weather services don’t appreciate broken code.

[–] mattreb@feddit.it 1 points 3 days ago (1 children)

when you were asked to "just merge" why was the review still on going? too large, or you didn't find an agreement with the autor?

[–] gsv@programming.dev 2 points 1 day ago (1 children)

It was more a question of finding the time within the daily business of doing science. I suppose it is also a culture issue, the priority to do a review is usually low. In other words, there was de facto never a review.

[–] mattreb@feddit.it 2 points 1 day ago (1 children)

Thanks, that's actually interesting. I've found making #5 work with limited human resources / deadlines challenging, and wondered what to do. My answer in the past has been to lower review quality (reviewing faster) while keeping #5.

In my case the priority for reviews was high, but we were limited by the reviewers/developers ratio since most people would not do reviews...

[–] gsv@programming.dev 1 points 1 day ago

I'm glad to hear that lack of time/resources for code reviews are more common. Also for clarification: I was the author and requested reviews by my colleagues. In reverse, I did not receive requests to review PRs so far. Tbh. I would really like such a review culture as it is already standard in scientific publishing and it would have avoided some obvious bugs we did encounter in the past. Having that said, as I did not receive any review at all and I would appreciate low(er) quality reviews better than none.