I dislike excessive usage of these kind of tools because not infrequently you end up making your code worse just to satisfy some dumb tool.
The real solution is just accepting that not all code needs to be like you would have written it yourself, and asking yourself "does this comment address anything that's objectively wrong with the code?" A lot of times the answer to that will be "no".
The less communication between devs during code reviews - the better. It's exhausting to be diplomatic to your coworkers asking them to change something. It's a minefield full of feelings and egos.
Usually many architects tend to go overboard with interfaces for everything, repositories, data layers, hexagonal architecture, and whatever else is fashionable at conferences.
I'll only push for the better way if I, or someone else, is willing to refactor the whole codebase.
Last thing I want is to work on a codebase with N different ways to solve the same problem. That decreases productivity too much.
Unpopular opinion: I think individualism showing in code is a bad thing. Code should be textbook boring with as little personality showing as possible. This, of course, is due to my experience working in a department with ~40 developers. I work so much faster when I don't have to decipher "clever" code or somebody's personal style for how to line break their over-complicated comprehensions. It slows everybody down when there are 40 different dialects being "spoken". Homogenizing our code with linters and formatters means that any part of the codebase I jump into will look completely familiar and all I have to do is focus on the logic.
In an ideal world, developers care more about how their program works more than how it is written. But ya, unfortunately most people are working on applications they don't care about, so they get overly creative with code and tools.
If workaday code needs documentation for other developers to understand it to begin with, it needs a rewrite. Even if it runs great as code, people aren't computers and it needs to communicate it's purpose to both. Finding a showstopping bug under pressure in code like that is enough to make you want to put pencils though your eyes.
Also, agreed on documentation though I didn't feel like diving into that one in my response.
EDIT: Ohhhh I see now :)
'Workaday' technically has two definitions: the first is something work related, and the second is something ordinary, common, or routine. Both definitions carry connotations of the other though. In this context, I'm referring to the everyday code that we write to solve most problems. I'd juxtapose this with unusual code working around really weird constraints or making up for a fundamental shortcoming or bug in a language or critical library which might require janky counterintuitive code.
// fires the missiles.
// Spongebob case (and extra underscores) makes it hard to accidentally type.
void FiRE___MIsSiLeS(); // !!!!
// be careful!
void fire_missives(); // posts a witty tweet
void fire_missiles(); // fires the missiles
void fire_missives(); // posts a witty tweet
I don't do much Python, but when I do I've often run in to line length "linters" for example. I (strongly) prefer to hard-wrap at 80, but if your code happens to end up at 81 or 82 then that's fine too, and sometimes even 90 or 100 is fine, depending on the context. Unconditionally "forcing" a line break by a linter is stupid and makes the code worse 99% of the time. It's stuff like this that serve little point.
And if you need to constantly tell people to hard-wrap at ~80, employing common sense, then those people are morons or assholes who should be fired. It's a simple instruction to follow. (and yes, I've worked with people like this, and they either were or should have been fired).
Actually I found that the number of linters is directly proportional to the quality of the code: the more linters, the worse it is (on average, there are also excellent projects with many linters). The first reason is that people will make their code worse just to make the linter happy, and the second reason is that I found that linters are often introduced after a number of bad devs wrote bad code, and people think a "linter" will somehow fix that. It won't: the code will still be bad, and the developers who wrote it are still writing bad code.
Linters/static analysis that catch outright mistakes should be added as much as possible by the way (e.g. "go vet": almost everything that reports is a mistake, barring some false positives). I'm a big fan of these tools. But stylistic things: meh.
The real advantage of linters is that code comprehension becomes easier and authoring that code becomes simpler. The code always follows the same style in every project that team has, and everyone can configure the linter to reformat on save. It also completely removes the conversation on style from code reviews.
Haven’t you ever gotten a big PR from someone because their editor settings were different and they auto formatted a file differently than it was written before? This makes PRs a nightmare to differentiate between style and substantive changes.
The auto formatting is honestly one of the biggest productivity gains as a developer that I’ve seen. No longer do you have to ensure your code is indented when typing. A lot of times, I will even write multiple lines of code on a single line and let the formatter take over. And the end result is that my code is consistent and matches the style of all the surrounding code. It takes a cognitive burden off of everyone, when writing the code and when reading it (PR or otherwise).
That said, any autoformatter will need to strike a careful balance or it will produce nonsense: too little enforcement makes the tool useless, and too much will just crapify things. gofmt mostly gets it right, but I've never been able to get clang-format to produce something reasonable. I don't know about Python, as I don't do much Python.
Because of this things like [`eslint-config-prettier`](https://github.com/prettier/eslint-config-prettier) exist to ensure conflicting eslint formatting rules are disabled if they can be handled by prettier.
It's not about whether it's automatic or not, it's about the code being worse (that is: harder to read). Breaking lines because it's 1 character over some arbitrary limit is almost always bad, whether it's automatic or manual.
Code style is the epitome of bikeshedding. Anything consistent is fine, and using a tool to automate that is a huge productivity win.
I immediately reject those PRs. If your editor can’t format only changed lines you need a better editor. If you’re working on a task, changed lines should only be for what is necessary to implement it - nothing else.
132 is fairly reasonable but I can still skim-read 80 column code noticeably faster (especially when I'm looking at a side-by-side diff).
You're welcome to have different preferences, of course, but I came to using 80 columns from having previously written much wider code myself - and the width of the -screen- I was using had nothing to do with my choice to switch.
I can only agree with this if the lint rules are paired with 100% automatic reformatting, and not simply a lint rule that rejects the changes. It's a huge waste of time to force the author to go back and manually fix line lengths to meet someone's arbitrary preference for how long a line should be. Line length is clearly a preference and forcing everyone to conform to a single standard doesn't solve legibility.
The language should make line breaking obvious or automatic. IMO, The fact that it's an issue at all seems like a weakness in Python's syntax. Compared to the languages dominated by curly braces and the lisps with their parenthesis. Python saves my pinky fingers from typing braces and parenthesis but it demands more attention to the placement of line-breaks. Combine that with very short line-length standards and it leads to significant annoyance.
> Line length is clearly a preference and forcing everyone to conform to a single standard doesn't solve legibility.
I fully disagree here. Enforcing 80 characters is way too short for any reasonably complex code and leads to exactly the issue you're complaining about: tools doing automatic line breaking which make the code worse, AND/OR developers making their code more golfy in order to fit inside the line length limit whenever possible.
What you're arguing for (individual developers taking responsibility for their own line breaking style) would only work if every developer cared about such things, and now you have business/product complaining to project managers about how much time we're wasting formatting our own code by hand instead of letting automation remove that variable from the equation.
Again, all of these opinions are with the perspective of working with dozens of developers on a very large and complex codebase. If I personally disagree with somebody's "style" for line breaking, and I happen to find myself working in their code, what am I supposed to do if I personally find their code more difficult to read? Do I do re-work to make the format align with how I best read code? What happens when the original author comes back?
Developer ego is a huge problem. Automated formatting and linting help to reduce that problem.
EDIT: I was actually responding more to arp242... whoops
Making it a hard requirement is a bad idea, making it 'required unless you can give a really good reason' (and notice that arp242 made clear that lengthening some lines a bit was a perfectly reasonable thing to do) is usually* workable.
My usual coding style tends to fairly naturally fit into 80 columns, -especially- in the case of complex code because that tends to get broken up vertically for clarity, and an '80 columns unless you have a good reason' limit seems to work out fine for e.g. PostgreSQL whose source code I personally find -extremely- readable.
I do agree that a lot of the time having some sort of autoformatter that everybody runs is a net win, especially since with a little practice you can usually predict what the autoformatter is going to do and code such that the output is decently clear to read as well as consistent with the rest of the codebase.
(*) Enterprise java style codebases where every identifier name is a miniature essay less so.
Of course, if all you care about is the fungibility of your programmers, then having those overly strict rules completely makes sense, but if you want to produce a quality product, not so much.
I wish there was something like semantic commits “fix/feat/chore” for review feedback, specifically a keyword for these little things that are more future advice than must fixes.
I also try to be clear when I don’t expect to do a second review if they make the minor changes, that the green check mark carries forward.
More. broadly I’ve been trying to favor “rough consensus and running code” in my PR reviews, even if I don’t fully agree with an approach I’ll give the writer the benefit of the doubt since they’ve most likely spent more time thinking about the problem than I have. Or at least that’s my hope!
This is all well and good until your company brings in automation to enforce the setting that all changes dismiss reviews on all repositories.
Combine it with the requirement that all branches must be up to date with master before merging for the double whammy of rebase/review/repeat hell.
Nit: This is a minor thing. Technically you should do it, but it won’t hugely impact things.
Optional: I think this may be a good idea, but it’s not strictly required.
FYI: I don’t expect you to do this in this PR, but you may find this interesting to think about for the future.
None of these block a merge and anything beyond these type of comments will be a in-person discussion.
The FYI tag is a fantastic idea and I shall try and remember to use it in future.
The other important benefit is catching stupid mistakes - formatting, missing code, > vs >=, etc - dumb mistakes that everyone makes. But ideally most of those mistakes are caught by types, tests, and linting, so the code review just makes sure that a different pair of eyes has sanity checked things. If code reviews become mainly about this sanity check, then it's often a sign that the types, tests, or linting are inadequate (or that someone is being too picky).
The other problem is when reviews too often focus on the big problems - architectural discussions about how to implement a feature - in which case there's usually a need to have more architectural discussion before implementation starts.
You just can’t do that async inside a GitHub PR to the same level.
If that is truly the goal, shouldn’t the programmer have that information before they start writing code?
> The other important benefit is catching stupid mistakes - formatting, missing code, > versus >=, etc - dumb mistakes everyone makes.
I’d love this to be true, but I’ve now seen so many bugs missed in PRs because the reviewers get bogged down in “code style” feedback (which is this generation’s commas versus tabs debate I swear) that they completely miss the more severe issues. Additionally, the reviewers often only have shallow knowledge of the business requirements and can’t know whether > or >= is appropriate.
> The other problem is when reviews too often focus on the big problems - in which case there’s usually a need to have more architectural discussion before implementation starts.
Totally agree here.
To be clear, I support the idea of feedback from other developers during the development process, but I feel that little thought is given to (a) what kind of feedback is truly valuable, (b) when the feedback should happen, and (c) who is in a good position to provide that feedback.
> If that is truly the goal, shouldn’t the programmer have that information before they start writing code?
Could you clarify that a bit?
I agree that programmers would ideally start out with some knowledge of the codebases they work on, but by working on the codebases, that typically make changes and so that knowledge is quickly out of date! ;)
That's the value of code review as knowledge sharing - as new things are implemented, or old parts refactored, or new technologies introduced, or even parts deleted, multiple people are involved in those changes and so gain that knowledge.
EDIT: and I completely agree with the rest of your comment! Giving good feedback is a learned skill, and too often we don't put the time in to learn (or teach) it properly.
Again, this isn’t the “fault” of the code review, it’s that there are insufficient or missing pieces of the software development process overall. Yes, having a code review is a good thing, but we often try to make it do all the things.
One of the big things this achieved was that we rarely had to rewrite stuff, or come up with a solution that was missing some vital detail. Because everyone was involved in the initial plan (UI, backend, and business), we had a good grasp of different people's needs and could challenge ideas that would cause problems in some niche use-case.
To me, implementation and code review both come after this architectural discussion. That's not to say that there aren't still decisions to make, and sometimes I'd still sit down with the other UI guy and discuss how we might best structure our code - but again (ideally) before and during implementation, rather than after it when the branch is ready to be merged.
Rather, I see code review as a chance to learn about the minor details of implementation. For example, a function to search through a list of objects might have a dozen different potential signatures, invariants, and usages, depending on who implements it and how they prefer to work. Having a review makes sure that both (or all) developers know how to use this function, and have seen it if they need to use it somewhere else in the codebase.
And sometimes none of this works out, you think you've planned everything, you write your function to search through objects, and your colleague points out that a standard library function can do everything you've written but simpler and faster. And then you'll still have to rewrite your code, but now at least you've learned something new.
But there's also very real issues that one person thinks is a nitpick that really isn't. Perhaps because they can't see the issue with their own eyes, they don't understand the issue or they lack the ability to leave feelings aside and rethink what they've written.
We've all been attached to code we've written. We probably think it's the most elegant code ever written. But sometimes you've just gotta admit you were wrong, it's not readable or it's flawed and it will be worse off for the codebase.
I've also been called a nitpicker by someone more senior than me for pointing out some very real and potentially problematic race conditions in their code. To me, a race condition is a fundamental issue with what has been written and should be fixed. But to them it was acceptable because they hadn't seen it break naturally yet.
You can use code formatting and linting tools to automate the low hanging fruit but there's still opportunities to offer feedback that could be considered nitpicking but IMO isn't.
For example variable names, breaking up long functions, not enough test coverage which hints maybe the author didn't consider these cases or changing this type of code:
if not something:
# Unhappy case
else:
# Happy case
if something:
# Happy case
else:
# Unhappy case
I like when people comment on my code and find ways to improve it in any way shape or form. I don't care if they have 5, 10 or 15 years less experience. I treat it like a gift because I'm getting to learn something new or think about something from a perspective I haven't thought of.
In my reviews, when I look at the tests, I put on a test engineer hat. Testers like to do things like give an empty value where one is supposed to be required by the business logic, a negative or other out-of-range value, an emoji or other wonky unicode in text. Just basic stuff to a test engineer, but lots of programmers only write tests for the expected happy path, and maybe one or two cases for common alternate paths. The most common failures I find are in handling dates and date comparisons.
I've been wanting to add more fuzz testing to the tests I write, but I haven't quite gotten my head around how to do it effectively.
if not something:
# Unhappy case
else:
# Happy case
if something:
# Happy case
else:
# Unhappy case
In short it's a preference masked as a best practice, and putting it under the same kind of "oh we should change it for the boy scout rule" logic as things like "Maybe let's not have 1000 line methods" is the exact kind of performative review being complained about.
if not something:
# Unhappy case
return
# Happy caseIt's mainly avoiding the "if not else" pattern in 1 condition. I haven't met anyone who fully agrees that it's easier to read than "if happy else unhappy" or returning early with whatever condition makes sense for the function.
- This won't do what you appear to be expecting it to
- This will prevent <some other feature we're likely to need> from being implemented; at least without a much higher cost
- This will work, but is very hard to understand; it will negatively impact maintenance. Consider doing it this other way, or adding an explanation about what's going on
- This code is ok, but might read/work better <this other way>. I'm not failing the review because of it, but it's worth keeping in mind for future code.
By completely removing any personal preferences and disagreements on style, the whole team just gets to hate on and berate an inanimate tool instead.
We all just seem to equally dislike flake8 and move on with our lives.
Yes, you can go to extremes, and some people are annoyingly nitpicky when it doesn't matter. But generally, code is often a liability, stuff that is hard to understand (or even potentially subtly wrong) will cause issues down the line, and so on. Code review helps mitigate these issues, something which I think is one of the few empirical results we actually have. Plus, it also spreads knowledge about the code base.
The submission is in any case not pointing out that code review itself is bad (or even that having to adapt to a new code style is the problem), but rather the whole bureaucracy around it. Code reviews are good when the turnaround is swift.
Because the projects that weren't built fast enough were eventually thrown away, meaning they never require maintenance, which is the source of the sampling bias you are observing. I've seen it happen in some cases (engineer with the wrong personality leading an R&D project).
Build a culture that prefers succinct, non-nitpicky code reviews. Static analysis tools only give reviewers more crap to nitpick.
Which static analyzer is this? Every tool I've used only finds bugs the are provable so the false positive rate is essential zero
That said, often teams don't give credit for code reviews, or take into account the (sometimes considerable) time required to write them. To some extent, tools are also to blame - I think all teams would do well to pick a system that integrates with the dominant IDE. Doing code reviews in the same space you write code just makes sense; requiring that you use a friggin' webapp is HORRIBLE.
But of course, it's very very important to ignore that feeling and do the sensible thing, which is be happy that the code review is smooth and the code can be merged. I can't imagine the existential dread of realizing that half your job is pointing out the same common errors every single time. Tools like clang-tidy, clang-format, etc are so vital for staying sane.
That said, I think culture for this kind of thing varies wildly between different companies, and between teams in those companies; many (most?) teams are comprised of small numbers of people, so even one team/technical lead having a preference for a certain style could change things, and... well, everyone's different.
1) O(N^2) in the hottest code we have.
2) O(N^3) from someone else in the same area.
3) Filling a cache on first access - this causes the system to page (high latency!).
4) Hunting for data themselves and creating null pointer errors.
5) Hunting for data themselves and missing all the fun edge cases.
6) Duplicating a feature that already existed.
I'm guessing (4) could have been caught by static analysis?
I nitpick only because it’s a clever turn of a word that deserves understanding.
I insist on doing code review in areas where static analysis tools are completely useless, like in SQL code: without context, any SELECT can be good enough to pass static analysis, but crash in production due to performance problems.
Nobody was ever promoted in my company for doing code reviews. None of the people that were ever promoted know how to do any sort or code review. I can say that the correlation between promotion and code reviews is negative.
For example, a field named "customers" that holds a single customer can be a major readability hurdle for someone who is looking at code for the first time. Even if you discount readers' annoyance and discomfort, which I don't think you should, a single field named "cusotmer" can become a drag on readability if the misspelling gets amplified in the codebase or if some programmers on the project aren't native English speakers. (It can be much harder to mentally gloss over language errors that aren't in your native language.)
Something I learned in school is that it's extremely hard to read something you've written as another person would read it. I had an English teacher that would split the class into pairs every time we had a paper to turn in, and in each pair, you would read the other person's paper out loud to them. The results were comical, and to most students entirely unexpected. Their whole school careers, they had thought teachers were being mean and nit-picky, until they got to see their friends struggle to read what they had written. Then they started to understand that a wrong word, a misspelling, a wrong verb tense, an ambiguous pronoun, could stop a reader in their tracks or send them down the wrong path trying to understand what the writer meant. They also learned that they could not see these issues in their own writing. When they read somebody else's writing, they would stumble over errors, but in their own writing, they had to make a special effort to see them, and even then, readers would effortlessly "discover" errors that the author had missed.
For many programmers, PRs are their first and only opportunity to develop this empathy for readers, and if they reject the feedback, they're going to remain forever in the dark about how other people experience their code.
Sounds like glorified and highly opinionated linters whom will just gatekeep progress because they “don’t get it”.
Wouldn't the top level overlords want better code review metrics? Padding your stats with comments that can be handled by static analysis makes such noisy metrics even more noisy.
Just implement whatever clean code guidelines everyone is keen in having, with their dependency injection guidelines of an interface for every single class, and move on with the actual work instead of burning discussion cycles in pull request reviews.