Comment by raducu | Hacker News Reader

Comment by raducu | Hacker News Reader

Parent Full thread

I must be the exception, but I've only had bad or really "meh" experiences with code reviews:

Doing the reviews myself:

A lot of simple code splintered in 100 files -- I and all others that I know only review the diffs.

if(THIS_COERNER_CASE){do this ; do that}; xml configs relating to this or that business case

There's nothing to review, really, I don't know the business case, the code looks okish.

I really don't care about unused imports (unless you make a special commit called "code cleanup" that pollutes history and I hate you for that), lines too long, if you used nested ifs or returns, unless it's really nasty code (which I very rarely stumbled upon, usually written by some coding style nazzi in his youth);

Feedback for my own/others code:

- this or that "coding style" issues by some coding style nazzi; almost fine, it improved my inner compiler/code formatter

- "don't use this language feature, because I don't like it" from the highly oppinionated nutjob that leaves the company in a couple of months anyway to join a smaller company where his genius is appreciated more

- "why did you do this and that, you should have done...." half an hour later in person/over the phone conversation... "oh, yeah, I guess that actually makes sense"

- "whaaaaaat, you expect me review this monster commit? Why didn't you break it in multiple smaller commits?" -- because I care about the convenience of the person that is actually interested in investigating a feature/bug 5 years from now and not of the lazy code reviewer now

- "How could this security bug have slipped us, we have CODE REVIWS, don't we?"

> because I care about the convenience of the person that is actually interested in investigating a feature/bug 5 years from now and not of the lazy code reviewer now

Doesn't seem like such a big problem to make a PR with multiple commits, and then squash and merge into 1 commit after the review.

Personally I think that is pessimal, because if I am the person investigating in 5 years time I want to see the change split up into multiple logical commits. So lots-of-commits-plus-squash-and-merge requires the developer to do the work of splitting up the feature implementation but doesn't give the benefit of having a usefully split set of commits to the future-debugging-person...

The change is a feature, it is by deffinition a "logical commit".

Like "migrated this JMS queue to kafka", that can span across tens of files.

Just because the commit is big doesn't mean it's not also logical and consistent.

In fact the reverse can be said -- if you split the commit in smaller commits (and your build is still green with each commit) -- each commit can seem contrieved, and good luck later on, when you try to understand how a piece of code works and how this xml/drl/properties file is connected to this piece of code if you have multiple commits.

If it's a single commit, you can usually narrow down your search tremendously and can easily see the logical deoendencies.

So for me, if it's a JIRA issue, I usually squash my commits before I push.

I agree. If you want the "clean" straight line from the git DAG just use --no-ff in your PR system and you get a nice clean integration log with things like git log --first-parent (git praise --first-parent, git bisect --first-parent) and can still keep the extra history around for those of us that find it useful when investigating things 5/10 years down the road.

> Doesn't seem like such a big problem to make a PR with multiple commits, and then squash and merge into 1 commit after the review.

I fundamentally disagree here. If you're reviewing code, the code that is pushed should be what is reviewed, not a draft of what is reviewed..

If you squash and merge shouldn't the final pushed code be exactly the same as what was reviewed?

Not necessarily. There's a lot of dark arts to merges and there's a lot that can be hidden in them. If you've never heard of, as just one for instance, the git rerere cache and campfire horror stories of how it can be corrupted consider yourself lucky.

you forgot the one where you go like this

- "you are right, i completely forgot about that. thank you for catching this, i am changing the code and asking you to review it again after the fix."

The most efficient peer reviews I participated in were early code walkthroughs (prototyping/implementation phase) combined with code reviews. The actual code reviews were much more interesting because you could see what and how the programmer implemented points of critique that came up in the walkthrough. Navigating through the code was also much easier (with bigger changesets), because you already knew to some extent what was changed.

Reply on news.ycombinator.com