1. 34

  2. 9

    Ask yourself “is this my opinion?” – This is so important. Engineers that keep making comments based on unproved opinions can slow down the whole project, and bring no benefits.

    1. 2

      Yes and no. Sometimes something is an opinion but ultimately you should still pick A or B and not a mixture of the two. Is there any actual evidence that brace position affects correctness or readability? No. But inconsistent brace position probably does, and while it might be based on someone’s opinion, picking one or the other way of putting in curly braces needs to be done.

      What really frustrates me is that most code review systems (including the GitHub pull request model) have poor support for ‘this looks good I’ll just clean up the style issues with it before I merge it’. The effort to just go and fix the issues immediately is so much lower than marking the style issues, asking for them to be fixed, the person fixing them, them pushing again, me pointing out one they missed because I didn’t spend half an hour going through and marking every little instance, them doing it (probably annoyed themselves by now), them pushing, me finally approving and merging.

      1. 1

        but things like brace position consistency should be part of the agreed upon style guide. And it’s fine to point that a code is breaking the style guide.

        And while the rules of the style guide are arbitrary, they are there simply so everyone agrees on a common pattern.

        And this is covered in the article, linters and code style guides are super important to avoid this type of conversation. But then when someone keeps arguing that a class is more readable because they like organizing methods in some other way, that the team has not agreed as an standard, or that they think that some pattern is preferable to another, without concrete arguments beyond “I find more simple”, it’s when it boils down to personal style and trying to deny a PR simply because that’s not how the reviewer would had written the code, is very problematic and self centered. Which is not what you’re referring too, I think preferring consistency is necessary, and should be agreed with everyone in a living document.