Hacker News Reader

Comment by aequitas | Hacker News Reader

> The golden rule: value your reviewer’s time

This goes the other way around too. I've had some PR's that put copious amounts of time in to make them as complete and perfect as I could for the reviewer. Only to get them turned down for style reasons.

If style is that important, make sure you document the rules or automate it (3. Automate the easy stuff). So contributors have a chance at figuring this out for themselves, without having to go back and forth. If the style comes down to personal preference you can apply rule 12. Award all ties to your reviewer, in reverse too.

Nothing takes my motivation for contributing away as a reviewer that just must find something to complain about the code.

View on HN

I honestly love that Go has a formatter included, and the style is specified. Similarly with black[0] for Python. I used to care terribly about style, until I realized I just care about consistency and readability. Otherwise I don't give a shit, and having a tool available to everyone makes such a difference. This goes for other tools too, like static checkers, linters, even CI. I've been on projects where anyone could run the whole CI pipeline (which included the aforementioned static checkers, unit tests, even a doxygen run to check for undocumented things) on their dev machine before submitting, and not waste anyone's time with things better done by a computer. Wish every project could be like that.

[0] - https://black.readthedocs.io/en/stable/

> I used to care terribly about style, until I realized I just care about consistency and readability. Otherwise I don't give a shit, and having a tool available to everyone makes such a difference.

I had the same thing. Also consistency and automation is what's really important, as it extinguishes every need for style discussions. Because you can trivially convert from the project style to personal styles and vice versa using things like Git attributes[0], as long as it is consistent.

[0] https://git-scm.com/docs/gitattributes

yeah, same with me. That's why I love prettier. I care more about just having a pattern that's applied automatically, than I care about personal preferences.

Consistency for consistency's sake is not a virtue.

The best solution to all the style discussion nonsense is an automated, mandated style. The better solution is the team being mature enough to only care about real problems and not gate development with personal preferences. There's something to be said for feeling comfortable in your space. The company's codebase is not your space.

Consistency for readability is a virtue; did you miss that part?

And it's not about "gating" development with "personal preferences", what a strawman. If someone can't be bothered to run the formatting tool included in the repo on their code before submitting, then I can't be bothered to review their submission.

> The best solution to all the style discussion nonsense is an automated, mandated style.

Yes, exactly what I said. Again, did you miss that?

Consistency doesn't always serve readability.

In written English, you can convey the same basic facts with a hundred different sentences. A good author will choose the one that best conveys emphasis and subtle gradations of meaning to the attentive reader.

Code is also written for humans to read, so the best written code will use all the available facilities of the language to convey meaning to the reader.

Exactly Like english or any written language, generally books are typesetted against a bunch of general conventions. You dont choose if you get one or two spaces after a semicolon. Your book is not great because you wrote it in leetcase, you focus on the meaning, not the details.

You really do get to choose if you want 1 or 2 spaces or whatever formatting you want. Certain publishing houses might have style guides that they follow strictly and others less so, but all this just makes the formatting for a book a perfect metaphor to formatting code.

You don't. The consistency is within the same book (or maybe publisher). A book would look ridiculous if every paragraph was using a different number of spaces after punctuation.

But between two books its just personal preference whether you agree with the choice the author/publisher made.

I'm curious to see an example of how this plays out in code (not comments).

Using python, the choice between a list comprehension or a for loop is often a style choice.

I can't say this is common practice, but a list comprehension is great when the iteration is simple and quickly becomes hard for a human to parse if you're doing anything slightly complex. If you need any complex logic, you have to use a for-loop. So -- when written by a developer with these perspectives -- a for-loop signals complicated iteration logic and a list comprehension signals simple iteration logic. But you could do either with both if you chose. So, it's a style choice that can be used to convey meaning.

I failed a google interview for using a nested generator comprehension instead of for loops. It solved the problem, but the interviewer was utterly disgusted by it. He had some fair points about it being hard to inspect and modify and add exceptions.

I agree with automating it, but I also have seen automated styles going too far, more often than not.

Automated styling should be kept minimal, like checking names and indentation. Absolute rules does not work because then you cannot deviate from it even if it is appropriate in the right situation.

The standard JavaScript checkers nowadays are ridiculously strict and forces me to spend time writing a less readable code. For example yesterday my code was invalid because I did not write properties of an object alphabetically. So I had to convert this:

    {
        host: ,
        port: ,

        key: ,

        username: ,
        password: ,
    }

Into this mess:

    {
        host: ,
        key: ,
        password: ,
        port: ,
        username: ,
    }

With tslint I cannot even execute my code if the formatting is invalid. So if I write a console.log or forget to add a space between the slash and the code I commented, the compilation crashes.

Maybe that should not be an object then, but a URL or just several objects.

This is a very common pattern for a lot of third party packages.

And it is just an example to show that there are perfectly legitimate cases that are more readable when not alphabetically sorted.

It is consistent though, do you have a better generic system for grouping object keys than alphabetization? It’s pretty handy to approach a codebase knowing that it has enforced this convention.

My point is that having rules on this kind of detail is more harmful than beneficial, so there should not be one in the first place.

The point is that it's not generic: there is meaning conveyed by the grouping that can't be expressed as an easy script (which is why there's a human writing the code in the first place).

> The company's codebase is not your space

That’s one possible way to look at “the company’s code.” Another one is that you’re in “that” code six (or more) hours a day and it’s usually written by someone else. So if everyone codes to _their_ comfort, there’s a likelihood that no one is comfortable in the code ever. I find code style standards that a team as a whole has compromised upon (aka consistency) a reasonable approach to “average comfort.”

For JS/TS/etc, Prettier: https://prettier.io/

I don't agree with all the formatting, but I also just don't care enough. It's sooooo nice running `yarn format` on a huge legacy codebase after adding prettier.

ughh ... I've never seen HTML code uglier or harder to read than what came out of prettier on the code bases we have that it is used on.

The fact that gofmt removes whitespace between arithmetic operators drives me insane. Fortunately, rustfmt and black don't do that, and I've been largely able to avoid writing Go for the last year now.

How I managed this on a team:

If there is a style disagreement, documented style rules prevail. If there is a documented style rule, follow it. If there is not a documented style rule, you cannot block this PR with the style objection.

This avoids really annoying issues of "I don't like how this is written [for style reasons]" in PR feedback. I'm not a mind reader, I don't whether you prefer for..in over Array.foreach or whatever, my PR shouldn't be blocked because of that. If you want new rule, go propose one and we can discuss & vote on it later but it doesn't apply to this PR.

This avoids the issue of "style rules are determined by whoever has the greatest capacity for argument & is willing to block PRs over it." It goes to a discussion & vote and everyone can agree once then stop talking about it. :)

I have a rule that if stylistic decisions matter enough to reject a PR over, they also have to be enforced by a linter or code formatter. If they are that big a deal, we should enforce them via automation, not review. And if they're not that big a deal, do not put them in a code review.

It baffles me that this isn't the consensus.

I've waged wars over documenting styling rules (some too complicated to be covered by a formatter - which by itself is a red flag) because I know that people often forget to enforce rules that they themselves have come up with.

> Nothing takes my motivation for contributing away as a reviewer that just must find something to complain about the code.

Or the reviewer that always asks for more tests. (Once I got asked to write a test that was unnecessary and almost impossible to write a test for. Think timing issues. My options were to push back or spend a significant amount of time writing a test that was not needed. As a junior that already pushed back more than a typical junior I made the best choice I thought I could make at the time.)

What really helps in the case of testing is lead by example. If the project itself provides a good test framework and examples of properly implemented tests it is easy for contributors to add tests as well. But asking people to figure this stuff out themselves is often a really big hurdle. Also parametrized tests. I find no shame in asking people to add a case to a existing parameterized testsuite when submitting an issue.

Yeah... I have a lot of that discussion with my team, too.

It seems people (reviewers) need to find something or they think they are not doing good job.

Another irritating practice is treating opinions about style as commands -- basically commanding the author to change the style of the code to conform to the reviewer.

It should be understood that every problem has many potentially correct solutions. The job of the reviewer isn't to assure the best correct solution or the solution they like, it is to verify that the one selected by author is in the set of correct solutions. If this makes sense...

Exactly. Nothing I love more than working hard to make sure my PR meets the personal style of the colleague whom I expect will review it, only to have to have it reviewed by another colleague, with a mutually exclusive style, meaning I have to do it all over again, and then that reviewer goes on vacation and I get grilled by the expected reviewer on why I did it "that way". Sigh.

Reply on news.ycombinator.com