onion2kMar 17

But you can’t just not review things!

Actually you can. If you shift the reviews far to the left, and call them code design sessions instead, and you raise problems on dailys, and you pair programme through the gnarly bits, then 90% of what people think a review should find goes away. The expectation that you'll discover bugs and architecture and design problems doesn't exist if you've already agreed with the team what you're going to build. The remain 10% of things like var naming, whitespace, and patterns can be checked with a linter instead of a person. If you can get the team to that level you can stop doing code reviews.

You also need to build a team that you can trust to write the code you agreed you'd write, but if your reviews are there to check someone has done their job well enough then you have bigger problems.

Show threadanal_reactorMar 17

I never review PRs, I always rubber-stamp them, unless they come from a certified idiot:

1. I don't care because the company at large fails to value quality engineering.

2. 90% of PR comments are arguments about variable names.

3. The other 10% are mistakes that have very limited blast radius.

It's just that, unless my coworker is a complete moron, then most likely whatever they came up with is at least in acceptable state, in which case there's no point delaying the project.

Regarding knowledge share, it's complete fiction. Unless you actually make changes to some code, there's zero chance you'll understand how it works.

Show threadrecursivecaveatMar 17
Do people really argue about variable names? Most reviews comments I see are fairly trivial, but almost always not very subjective. (Leftover debug log, please add comment here, etc) Maybe it helps that many of our seniors are from a team where we had no auto-formatter or style guide at all for quite a while. I think everyone should experience that a random mix of `){` and `) {` does not really impact you in any way beyond the mild irking of a crooked painting or something. There's a difference between aesthetically bothersome and actually harmful. Not to say that you shouldn't run a formatter, but just for some perspective.
Show threadjffhnMar 17

>Do people really argue about variable names?

Of course they do. A program's code is mostly a graph of names; they can be cornerstones of its clarity, or sources of confusion and bugs.

The first thing I do when debugging is ensuring proper names, sometimes that's enough to make the bug obvious.

Show threadvaylianMar 17
The greatest barrier to understanding is not lack of knowledge but incorrect knowledge. That's why good names matter. And naming things is hard, which is why it makes sense to comment on variable names in a review.
Show threadwhynotmaybe

Unless the naming convention were written in the 90s and all variable must follow a precise algorithm to be made of only abbreviation and a maximum length of 15.

Or for some, if it contains the value of a column in the db, it must have the same name as the column.

So yeah, instead of "UsualQuantityOrder", you get "UslQtyOrd" or "I_U_Q_O"... And you must maintain the comments to explain what the field is supposed to contain.

Mar 17 at 12:34pmWeb