this post was submitted on 19 Feb 2024
53 points (98.2% liked)

Programming

17364 readers
246 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

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 [email protected]



founded 1 year ago
MODERATORS
 

Whether you're steering an open source project or leading full-time a software development team, the key to maximizing productivity lies in efficient code reviews.

all 11 comments
sorted by: hot top controversial new old
[–] [email protected] 1 points 8 months ago

The things he asks to do are the reasons why I find no joy anymore working in coding. Hammering my thumbs seems to be more interesting than doing most of these actions. I swear, I got so bored I couldn't finish the read. Specifically "if you find yourself commenting on every line of code" the right thing to do is to setup a meeting with te hiring department.

[–] [email protected] 6 points 8 months ago (1 children)
[–] [email protected] 1 points 8 months ago

Interesting take I can't agree with. Maybe their product environment is very different.

once the feature finally arrives in production after a lengthy review, it is in truth no longer aligned with the user’s needs.

I've never had this happen in my development.

In my team, it's fine to skip reviews and commit on main, when it's reasonably small and confidence is high. An obvious and small change also does not take up much time to review.

The effort put into creating a well-/reasonably-described review is effort put into well design changesets and Git history. It helps you cover all bases, cases, and considerations while developing.

Review necessity, investment, and urgency are dynamic. If you as a reviewer don't have the time of mindspace, saying so is fine. Reviewers can be changed or skipped. Reviews can be to different depth and completeness, or partial.

Be mindful of what is reasonable and efficient and reviews are not hard blockers beyond what they should reasonably be. Reviews serve as significant quality, issue, and common understanding gates.

[–] [email protected] 13 points 8 months ago (2 children)

I think a lot of these are opinions stated as facts.

The nitpicking one seems to be using a different definition of "nitpick". To me, a nitpick is to pick on something entirely meaningless (eg. Fullstops at end of comments, slight incorrect variable names, code alignment). If i see a review full of those I assume the reviewer skipped the correctness checks, and phoned in the review.

The git push --force is definitely a controversial suggestion, im personally happy with doing that, but I have also personally accidentally force pushed dev/main and seen others do it. Squash on merge is probably a safer habit to have. Also, gitlab and bitbucket both get a bit confused if you forcepush to a branch that is part of a MR.

Reviewer fixing problems is also situational. For open source stuff, if you rely on the submitter, youll frequently jusy end up with an abandonned PR. For team stuff, the original author may have already moved on to another ticket, so pushing it back may stretch out the development cycle and cause the code to become stale, and potentially unmergable. Our solution is to just communicate. "This is wrong, I am going to fix and merge. Cool?"

The article is very light on how to actually review for correctness, which in my experience is the thing people struggle with most. Things to look for (Non-exhaustive):

  • C: Allocations, and deallocations.
    • Are there leaks in any codepaths?
    • Are scopes used correctly?
  • API usage: return values checked? API called correctly? Safe API should be used over unsafe
  • Thread safety: Are there locks? If yes, focus on these paths, locks are hard to get right. If not, is there anything that should be protected? Some APIs are not threadsafe.
  • Loops: Are bounds correct, do they terminate correctly.
  • Comments: Do the match the code? Do they add value? (This is subjective, and down to team preferences)
[–] [email protected] 5 points 8 months ago* (last edited 8 months ago) (1 children)

The git push --force is definitely a controversial suggestion, im personally happy with doing that, but I have also personally accidentally force pushed dev/main and seen others do it. Squash on merge is probably a safer habit to have. Also, gitlab and bitbucket both get a bit confused if you forcepush to a branch that is part of a MR.

You can add branch protections that will prevent you from accidental force pushing to main or dev.

IMO when I see a PR with "WIP" commits, I just assume that minimal effort was put into keeping the commits organized, and I squash all commits to review the PR. If I see many meaningful commit messages, I will try reviewing one commit at a time.

When I make a PR, I force push to keep my commits organized. If there are changes I want to make as a result of feedback, and they would create significant churn to rebase all of the patches, then I will apply the feedback in a follow-up commit.

[–] [email protected] 2 points 8 months ago (1 children)

I do almost exactly that workflow as well, but I just know its bitten me before. Protecting main/dev is fine, but I have also accidentally force pushed to the wrong branch and wiped out its work as well.

Muscle memory + Fatigue == Bad time :/

[–] [email protected] 4 points 8 months ago (2 children)

Pays to know how to recover code from the reflog in these cases.

[–] [email protected] 2 points 8 months ago (1 children)

Pays to know how to recover code from the reflog in these cases.

At first I read that as re-flog. Is that with a cat o' nine tails? 😂

[–] [email protected] 2 points 8 months ago

Your Git requires a regular flagellation to function properly.

[–] [email protected] 3 points 8 months ago

Yeah, or sprint to your colleague and ask them to force push their branch again :D

Another tactic for for getting clean git commits is to do all your messy commit work in a scratch branch, and then when your happy, create a new branch, and with meld, organise your changes into complete logical commits. We do that a little bit.