Hacker News Reader

Comment by ozim | Hacker News Reader

Code style and “are tests passing” should not even be there.

Code style should be automated same as running tests when someone creates PR.

Your relatively short comment caught my interest and I'd like to learn more about your thoughts.

As, IMHO it depends (as so often). Code review is commonly the place that puts the current style under test (hence automated otherwise you don't have the results during review) and under review.

E.g. Code review finds out if a code-style is missing, incomplete or even outright wrong.

If that approach is taken, passing tests and code-style must be in there, otherwise it is not useful to argue about violations. From my understanding I would therefore see those results be available during review.

It may also be that there is code-style for automated test-code, and that code part of the review. Without taken this into context of the code-review, I miss a bit the boundary of your comment.

Could you elaborate a bit where you draw the line and why? E.g. I can imagine there is a benefit to keep a distinct context for code-reviews so that they are still practically feasible and those parts that need further adoption are put into a different phase, like steps before (preparation of the current increment) or after (preparation of next increments) the code-review itself.

Style formatting should be automated on level of IDE or other CI tooling.

Discussing or fixing code style is huge waste of time and styling can be automated. Yes it will be broken in some edge cases but for me that is acceptable because my goal is to deliver the feature and not to make "perfectly aligned code". Aligning code by hand is waste of time, thinking about aligning code is waste of time. Formatting should be in configuration and you format whole file at once and never do stuff by hand.

Passing tests is also easy to automate when creating PR, CI should run tests and tell that to person creating PR - hey tests failed, fix it and then after they pass you can open PR, not a fellow developer because that is waste of time for everyone.

I did not write "tests should not be there" only "are all tests passing?" is not part of code review for me, it is something that is there before code review even begins.

So checking if there are proper tests and if tests make sense is part of code review.

Thanks, that better helped me to understand your comment. Additionally I was not aware if that was with pull-requests or more with regular code-reviews as part of the development process. That was an additional insight, thanks for sharing.

Nevertheless, things can be different, e.g. if CI passes, it merges already, no need to have a request for merge, which are often a blocker to keep a steady development flow. Naturally, this benefits from tests that are already run during development, if not driving the development.

And I didn't read you're against testing nor aligning white space and comments at all, more the localization. Actually the points you raise are looking important to me, because if alignment comes in late, this can cause a lot of erosion, which can hinder any review if not even provoke merge conflicts which are stopping the process quite early and require re-iteration.

Your approach should also work towards non-release blocking code reviews, a property I personally like, as I've seen teams struggle with code reviews, becoming more and more of a burden, even after practicing it has found its way. But that is only a subjective comment of mine, every project is different, which makes it an interesting topic for me.

I don't care about code style. Granted, I work in a small team, but I work on a project with someone with a different style. All I ask is that we respect each other's style. And that he puts braces around single statement branches.

What's surprising to me in the article is the pyramid style. In all business school and psychology texts, the top of the pyramid is the most important one. Here it's the least important.

There should be no "someone with other style working on project" - project should have its style and dev should adjust to that style.

Same with joining new team, one might have preferences but you leave them at the door and format code with whatever already is there.

There is tooling where you can also have formatting configuration per repo so if you use IDE it would take that config and format it for you in "project style".

I do not agree. Everyone can have their own style, as long as it's consistent enough and readable.

I consider code and project property of a company I work for. If they have dress code or code style it is for them to set it up. I might propose some changes that fit me but most of the time "when in Rome do as the Romans do".

Sure, but we are the Romans. The code base was a shambles. At least things runs without failing now. And management considers features and selling more important than maintenance, so the best we can do is keep it simple.

> If they have dress code or code style it is for them to set it up.

I also chose this company because they don't. A dress code, what power-hungry lunacy is that?

When you have hundreds of engineers working on the same codebase, it becomes very jarring to read code with different spacing, bracket formatting etc…

The benefit of automated linting is that the codebase becomes more coherent and easier to read.

The down side is that you can’t add your personal flare to your contributions… but in my opinion that’s not too important

You'll get used to reading diverse code in a fairly short time. "Coherent" and "easier to read" also has the downside of lowering the attention you have when reading the code, so you'll have a harder time catching obvious mistakes because "it looks correct".

And again, being against personal flair only makes sense if you are a manager trying to treat programmers interchangeably, but as a programmer, your interests are increasing personal flavour in code so you are less replaceable.

I was brought in to repair a codebase built by a programmer who wanted to make himself "less replaceable" for 4 years before he quit.

It took me 6 months to fully decode all his personal mnemonics, abbreviations, style decisions and bizarre architecture plans.

There were several times during that time when I considered trying to find him to tell him what I thought about him to his face.

You are going to be replaced at some point, unless you think this company is going to fold before you retire.

The real problem here is the lack of documentation though - like, if you use non-standard tooling or style or decisions, the burden should be on you to document them. Sounds like in this case this hasn't happened - which is really sad, but not much different from awful (but standards-conforming) codebases with zero documentation.

If he actually documented his abbreviations, mnemonics, style and architecture, you would have most likely appreciated it instead of cursing it, I think.

This is true, but if he had documented anything then he would have become significantly more replaceable.

So replaceable in fact that the company wouldn't have had to hire me at great expense and could have got a normal developer in to take over and continue.

Maybe I'm just extremely jaded since my job is fixing messes left by people with more style than sense but I see striving for uniqueness to be a flaw in an engineer.

Fair enough. But for him (and you) it worked out great! He got some job security, and you got a job by fixing the mess. If there wasn't a mess, you wouldn't have this job:)

In Maslov's the bottom is the most important.

Granted, I wrote that sloppily. The bottom forms the basics, the rest is (supposed to be) built upon that and we're supposed to strive for the top. That's Maslov's idea. But Code Style isn't built on top of unit tests, nor is it code review perfection. A pyramid simply isn't the right graphic for OP's ideas.

Formatting is not the only component of code style, it's just the part that can easily be automated. Anytime there is more than one way to do something, and both options are correct and produce the same result, the choice between them is likely to be a matter of taste or style. For example, using a loop instead of using a map function. You can automate some of that stuff, but never all of it.

Reply on news.ycombinator.com