Hacker News Reader

Comment by markus_zhang | Hacker News Reader

Story from friend but I can relate:

Some teams and code are pretty much unreviewable and the best thing is to add CI for simple tests and lgtm if there is no glaring mistakes.

My team only has 3 including the manager, so, eh, each one holds a lot of knowledge that only he or she knows. Documentation? Yeah that’s a good idea, but I don’t have time to read them because “We want to ship as fast as possible”. So I just put up a precommit for testing and plug in the same tests for the CI and call it a day. If you pass the CI I’ll take a cursory look and LGTM.

Some code is unreviewable. I work as a DE and it’s all business logic entangled. Why is there a specific id excluded? What is the purpose of this complex join? I mean, the reviewer is not supposed to know everything, right? So the best thing, again, is to only look at the technical things (is the join done properly), let the CI figure out the weird stuffs and LGTM.

View on HN

"Why is the specific id excluded?" is exactly the kind of thing we can capture in code, and that a good code review will flag.

It takes two seconds to write `EVIL_IDS_THAT_STOLE_OUR_LUNCH_MONEY=[1] [...] NOT IN EVIL_IDS_THAT_STOLE_OUR_LUNCH_MONEY` instead of `NOT IN [1]` and then not hate your past self six months from now when you have to figure out why something has been excluded.

If some code can't be understood today, it's not going to be able to be understood when someone comes to modify it. Maybe in your domain all the code is write-once-read-never, but for those of us maintaining enterprise software that isn't an option.

We absolutely expect our reviewers to fully understand the code. If you want a quick shortcut to make that cultural change, have any bugs in code go to the code reviewer instead of the original author. You will find people start taking code reviews a whole lot more seriously.

> We absolutely expect our reviewers to fully understand the code.

I suspect you don’t work as a DE or your company has top notch culture. I have never worked in any companies (including some big ones) that encourage a full understanding of any code submitted to PR review.

more or less the entire point of code review is to ensure that the reviewer fully understands the code that they review

i know that lots of orgs don't do it this way, but it's really important to make this point very clear: those orgs are wrong, and pathological, no matter how many of them there are

I want to agree with you. But I'd argue that for some career paths it is just impossible, and it is definitely impossible for my company. That's why I never consider myself as an engineer -- and I'm fine with it, because I don't love what I do. I take my side projects more engineer-ish.

I mean, the reviewer is not supposed to know everything, right?

Uhm he kind of is, if you want a proper review...

Reply on news.ycombinator.com