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 thread_kidlikeMar 17

I'm very surprised by these comments...

I regularly review code that is way more complicated that it should.

The last few days I was going back and forth on reviews on a function that had originally cyclomatic complexity of 23. Eventually I got it down to 8, but I had to call him into a pair programming session and show him how the complexity could be reduced.

Show threadzzrrtMar 17

I know the aggravation of getting a hairball of code to review, but I often hold my nose. At least find a better reason to send it back, like a specific bug.

If you're sure cyclomatic complexity should be minimized, I think you should put such rules in a pre-commit hook or something that runs before a reviewer ever sees the code. You should only have to help with that if someone can't figure out how to make it pass.

If you're not willing or politically able to implement that, you might be wasting time on your personal taste that the team doesn't agree with. Personally I'm pretty skeptical of cyclomatic complexity's usefulness as a metric.

Show thread_kidlike

I just used it here to approximately convey the scale.

the original function was full of mutable state (not required), full of special cases (not required), full of extra return statements (not required). Also had some private helper methods that were mocked in the tests (!!!).

All of this just for a "pure" function. Just immutable object in - immutable object out.

and yes, he was a junior.

Mar 17 at 6:00pmWeb