[tboyd47]

> Code review at the end of the task.

This is something I still struggle with. I think it's a result of an open source tool (GitHub) bringing bits of culture along with it.

In open source world, all contributions are entirely voluntary, and most follow the Benevolent Dictator For Life form of governance. So you do a bunch of work, and you submit a polite request for the BDFL to pull your work into the project. He or she may decline your Pull Request, request a long list of changes, or just wait ten years to give any response at all.

In a business setting with hourly pay, deadlines, and co-workers chosen for you, not by you, it makes less sense. People tend to wait until the last minute to give or request feedback which leads to a lot of thrown-away work. Unfortunately, willingness to throw away work becomes a mark of pride at some places. Maybe enterprise projects need some kind of "managed push" system instead of a "pull request" system.

Edit: "Unfortunately, willingness to throw away work becomes a mark of pride at some places." to elaborate, the thing that makes this unfortunate is that the person assumes that throwing away work is necessary for code quality to stay high. By requesting feedback at intervals (which is what the author of the article recommended), you can avoid throwing away work without sacrificing quality.

[hawski]

What I don't like in "commercial" code reviews is that most reported issues are there just to fill space. It's busy work. There is this formal comment system. So instead of fixing this typo in a code comment directly reviewer writes a comment. Code style issues that arise, because of absence of a tool like go-fmt also warrant a comment. That's sad.

There are meaningful comments, but usually people have to fill their review quota.

Lowest hanging fruit should be just an in-place edit that would be automatically retested and that a reviewee can easily take as a patch and check himself. Instead of the back and forth bullshit.

It's a bit funny that placing a red herring comment that anyone can safely comment on makes the workflow faster. It's like this story about an artist who would place something ridiculous so when the time comes the CEO would just say: "It's good, just remove the thing".

[dionidium]

Part of this process, especially for new team members or junior devs, is to signal that, yes, we take typos (and formatting, and proper grammar, and the style guide, etc) seriously. I could fix this myself, but I shouldn't need to. We all make mistakes, but too many "small" mistakes is an indication of sloppiness. Is this actually a mistake? Or were you just being lazy?

[somethingsimple]

Agreed. In my team I am known for being extremely meticulous about this, and some people don’t like it. If I see one or two typos or spacing issues, I understand that’s an honest mistake. But if I see typos, spacing issues, bad indentation, bad naming, etc. all over your diff, I can’t help but think you’re sloppy. If you don’t care about the little things like properly formatted code, I have a hard time believing you care about the big things like error checking and proper test coverage.

[Clubber]

A code formatter will save you tons and tons of man hours.

[gboudrias]

Good comment. The thing to remove in the story is a duck: https://en.wikipedia.org/wiki/Law_of_triviality#Related_prin...

[boomlinde]

On the other hand, pointless bikeshedding quickly gets mutually tiresome in a setting where every line of code is reviewed in small increments. I also like to address these tiny inconsequential hangups politely when I disagree that they're important. Let's say some guy suggests changing "name" to "someNeedlesslyDescriptiveName" and I'll point out that it's already clear what it is because it's the only variable in the needlesslyDescriptive function.

[epicide]

In business settings, I've seen plenty of pull requests "go stale". Usually, they come about from a) the person not updating their code based on reviews, b) the reviewers not reviewing in a timely manner, or c) the change is no longer necessary / a priority.

Usually, it is some combination of a and b. All cases are an indication of a deeper problem. None can be fixed by calling your "pull request" a "managed push" nor by giving people unrestricted access to pushing code. It's typically either a problem with communication or priorities/expectations.

[pjc50]

> willingness to throw away work becomes a mark of pride at some places

Is this a typo for "willingness"?

Code review is important not just for preventing junk from entering the codebase but for ensuring that someone else has seen the thing and has some idea of how it works. I agree that "pull request" is the wrong terminology for a commercial environment.

[tboyd47]

Understood. It's just that the PR model only encourages that code review to happen at the very end of the process, when the submitter may have been polishing their work, testing it, and refining it for several days. It would be more efficient for it to occur periodically throughout development, rather than at the very end.

[house9-2]

I usually create a PR after the first commit and mark the PR as a WIP, then it is available for review throughout the development process. YMMV

[boomlinde]

I don't necessarily see code reviews as the problem here. I think it's a very important aspect of development that unfortunately seems first in line to get cut back when other parts of the organization fail, when for the sake of quality I believe that the organization should be designed to always accommodate it.

For example, you mention waiting until the last minute and throwing away work. If this is such a big deal, maybe the units of work are too big or too loosely defined. Loosely defined as in "eh, we never needed this in the first place" or big as in "sorry about that but your 2000 LOC commit just isn't the right approach". Maybe github style reviews/pull requests are also not the best way to go about it. IMO the github style is pretty weird and shouldn't be conflated with reviewing in general.

At my current workplace, we use gerrit to review every commit. The only way that they'll ever get to master or a shared development branch is if they get a +2 vote. Maybe this is closer to a "managed push"? It's not rare that someone finds an issue, and when that happens it's very rare that it means scrapping the entire patch, and when that happens, it's usually a very small patch for a problem/feature that ended up being fixed elsewhere, so I definitely don't think it's a waste of time. Instead I am grateful that many of the patches didn't enter the codebase in their initial state.

[tootie]

Tech lead is dictator. You should have a tech lead who is at least 50% hands-on coding and is enforcing the code review system and participating in as many as possible.

[tboyd47]

This is an option, except for the fact that the tech lead is beholden to the same timelines and pressures from management as everyone else.

[xstartup]

What do you mean by throw away? The submitter can always make the requested changes. And if the pull requests are not even relevant then it's entirely a different issue.

[tboyd47]

Meaning the submitter just spent one week writing code that will have to be written over again. If they had requested feedback informally before the PR, they would have written the code correctly the first time.

[Bjartr]

Having a PR workflow in place doesn't (or at least shouldn't) stop that person from seeking out that informal feedback earlier. To me it seems like what you're really looking for is pair programming, not pre-completion reviews.

[Clubber]

There is a dev design/plan time that could be utilized to solve that.

[flynnieous]

Or you can do pair and/or mob programming, where the review happens as the code is created. This eliminates the feedback loop and the creation of throwaway code.

[house9-2]

Even when I pair program it is nice to have someone else review the code before merging. Or at a minimum do a 'self' code review after the work is complete, once you have pulled your head out of the weeds things might look different.

[detaro]

> Maybe enterprise projects need some kind of "managed push" system instead of a "pull request" system.

What would you do different in "managed push"?

[tboyd47]

Not sure? Maybe letting developers push directly to master, with an automated test run at each commit and a project manager or tech lead in charge of tagging and deploying releases?

[zAy0LfpBZLC8mAC]

> I think it's a result of an open source tool (GitHub) bringing bits of culture along with it.

Erm ... GitHub is entirely proprietary!?

[frobozz]

I imagine the meaning is 'tool for doing open source', rather than 'tool that is open source'.

[zAy0LfpBZLC8mAC]

Which still doesn't really make sense?! Plenty of open source development happens outside of github, and plenty of proprietary software development does use github ... so if that is what you mean, you could essentially say that about any software that happens to be used by some developers for the development of open source software!?