Comment by terrortrain | Hacker News Reader

Comment by terrortrain | Hacker News Reader

This list is awesome, i love everything except:

12. Award all ties to your reviewer

A true "tie" is really rare IMO. If it really is a tie, we should all should agree that person putting work into it should be able to decide their own code. Not only does it cut down on discussion, but it cuts down on work and communication needed to get the PR past the finish line.

Allowing ties to go to the reviewer can lead to all kinds of discussions about meaningless stuff, like:

``` We should use forEach(...) instead of for loops, because that is how a most of other code is written. ```

In that example (from a real code review), the code is practically the same, just different style. So now the coder has to go back, re-write, re-test, signal for a re-review, re-discuss etc... All for something that is a tie, and doesn't actually matter either way.

Alternatively, letting non-issues through without a duel every time, saves a bunch of time and energy on every ones part.

This is one spot where I strongly prefer the standard from the Google guidelines[1]. "Reviewers should not require the author to polish every tiny piece of a CL before granting approval. Rather, the reviewer should balance out the need to make forward progress compared to the importance of the changes they are suggesting." Getting caught up in all the little details rarely serves this purpose; more likely it's being done in the service of something anti-productive like a personality conflict.

[1]: https://google.github.io/eng-practices/review/reviewer/stand...

Moreover, Google’s style guides contain a very specific clause at the beginning. Here’s the example for Java.

> This document serves as the complete definition of Google's coding standards for source code in the Java™ Programming Language. A Java source file is described as being in Google Style if and only if it adheres to the rules herein.

https://google.github.io/styleguide/javaguide.html

Google requires all code to go through code review, so to avoid time being wasted by style disputes or reviews being sent back for style-only changes, style disputes are essentially banned. Anything that can be enforced by an auto-formatter or linter is so enforced. For anything else reviewers are expected to be able to cite the style guide to back up their requested change.

This practice seems to have spread into the community due to the popularity of Gofmt, and now other languages like Rust have a community-standardized formatter (rustfmt) and linter (Clippy). Many thousands of hours have probably been saved by this point by treating format akin to language syntax.

That's the ideal, but there's a kind of magical "nits of the gaps" effect where some folks will find gaps in what can be automatically handled, and zero in on them.

I once had a colleague who loved to nitpick variable names. Should that flag be named "continue" or "shouldContinue"? Or maybe the i variable should be renamed to index. Or maybe "row" and "column" should be renamed to "i" and "j". Sometimes "buffer" should be renamed to "data" and sometimes "data" should be renamed to "buffer".

It does not make sense to say that variable names don't matter just because there's no way to automate a style guide on the semantic content of variable names. Sometimes variable names matter quite a lot.

So interpret that particular part of their code review guide as being less about the value of automation, and more about how to guard against some of these social headaches. A culture of breaking ties toward the requester discourages nitpicking. A culture of breaking ties toward the reviewer encourages it. Even in the presence of linters.

The two most difficult things in programming.. caching and naming

The two most difficult things in programming: caching, naming and off-by-one errors.

The three most difficult things are naming, cache invalidation, off-by-one errors.

And latency.

Latency is not difficult.

It is just lack of knowledge or focus to understand the total execution path and remove unnecessary elements from it.

I have worked in algorithmic trading where I did research and I designed framework to respond to stock exchange messages within 5 microseconds, consistently.

5 microseconds is difficult and requires some special techniques but 100 microseconds is quite easy and requires just understanding what is execution path and removing all unnecessary operations from it.

If you look at contemporary developers and web frameworks it doesn't seem anybody puts particular care into understanding those things and only try to twiddle couple of things and see how that transfers to performance.

Latency is (where I am used to it) a hard physical limit imposed on the upper end by the speed of light. When you have a system that literally spans a planet, there's only so much clever algorithms (and hardware) can do to speed up the time difference between "things happen in one end" to "things happen on the other".

Yeah, it is hard problem when you are hitting physics.

But then it really isn't a problem -- it is a limitation. Nothing you can do much about.

There are of course types of applications where latency is really tough topic. For one, low-latency trading is where companies are in arms race against other low-latency market players and there is always improvements form improving your product's latency.

But in a typical situation like websites and corporate services, the latencies required are so large that it is quite easy to reach them -- just don't do anything stupid.

My last boss had a problem with this. Guy literally told me to just accept any change suggested to ease the review process and it's not worth debating any comments. We had very different opinions.

He always claimed he was "not nit-picky", but then he let reviews turn into exactly what you said, while loops versus for loops and what-not, and allowed the rest of the team to comment on low-value things like style rather than answering the important questions.

Why pushback at using a built-in function that does the same thing as some little helper your team wrote 8 years ago, when your helper function doesn't even handle special cases? There's literally no point other than someone has NIH syndrome. Why are the source files all 5,000 lines long and not broken into a logical project structure? Why is the back-end data-gathering and transforming code inter-mangled with the front-end templates (Django) so far as to be written as template filters and template functions?

It did not take me long to realize the development culture there was not aligned with my values. The lead developers came up in a different world and refused to see the positives and benefits of any modern development workflow involving a separate back-end API and front-end application.

I was going to say something similar.

When I'm the reviewer, I try to decided for myself whether something is truly important, or just a style choice. If it's the latter, I will comment on it, but then add "Your call." to make it clear that I expect ties to go to the author.

This should be included in a "How to Make Your Code Reviewee Fall in Love with You" companion article, if one doesn't exist yet.

I wrote a list of code review suggestions a few years ago that focus on things the reviewer should do:

https://mtlynch.io/human-code-reviews-1/

I wrote a "what makes for a good change" doc a while ago, that touches on this.

https://github.com/vatine/sressays/blob/master/change-reques...

My preferred approach is: the ties are awarded to the individual whose name will show up in `git blame` after this change is merged.

> agree that person putting work into it should be able to decide their own code.

I take the opposite of this approach. If my reviewer felt it was worth their time to put in the work to provide feedback, then I accept their opinion. 99.99% pr feedback is not worth escalating to a ping-pong or face to face meeting.

I mean, consistent code style is just good practice, so maybe that's not a great example.

I don't think I've ever seen style so divergent that it actually hampered my ability to change the code. Add a tool to automatically apply some stylistic choices if you want, but as soon as it becomes extra work to adhere to a specific code style, it quickly deteriorates to causing more work than it saves.

Yeah we have tools where I work that deals with 90% of style errors, and reviewers call out the other 10%. After a few weeks it's all second nature so you don't have to do much to adhere to the guidelines and just do it while writing the code in the first place.

To me ignoring style is kind of like slipping a PR/diff/cl through without unit tests. Sure, that individual CL won't be the downfall of the codebase, but repeated offenses will destroy the codebase without an easy way to fix it all. Of course, without unit tests you end up with an unmaintainable fragile codebase, and without style you end up with an ugly, hard to read codebase.

It's exactly the same: if I think a style change is going to lead to "destroying the codebase" with repeated offences (rare - and should've been a transparent autofix if so) I'll call it out, and the same holds for a unit test. If you have a good reason to skip a unit test (e.g. isolated all stateless code, where the remaining code is almost trivially correct but hard to test) then that's fine (if it's noted as such), but otherwise I'll call it out.

It's just that skipping unit tests without good reason is far more likely to "destroy the codebase" (or at least to cause more work down the road than it saves now, without there being a good reason to skip that work now) than not adhering to some style guide will, IME.

Agree; rule #1 of reviewing in my opinion is "trust your colleague whose code you are reviewing".

Reply on news.ycombinator.com