The core issue here is reviewer saying “if you change this, you also have to fix other pending issues in codebase”.
That should be the point of push back “Great feedback, and appreciate the direction to improve code quality, however the implications of changing Y means we need X, Y, Z approval and it will many days. I am making a tech debt task with what you described and will be done in a follow up PR based on priority and bandwidth.
Let’s focus on what it would take to get this surgical PR out.”
The biggest lesson I learned is to make focused PRs, and learn to pushback when reviewer asks for a scope creep. Mostly other engineers have been pragmatic.
It’s unrelated to number of lines as you can reformat while codebase but not change anything logically, or change some feature flags which could have a huge impact. But ONE FOCUSED CHANGE AT A TIME
> The core issue here is reviewer saying “if you change this, you also have to fix other pending issues in codebase”.
I don't agree. IMO, the worst issue being displayed here is that of the "6 days to change 1 line of code," almost half of them elapsed before an engineer ever looked at the issue. If this is such a high priority issue that the company would "need" (scare quotes intentional) to do a layoff if it's not implemented soon, those 2-3 days before anyone looked at it should never have happened. And that is apparently what passes for "the fast track" in this development process.
And then there's the 2 days at the end of this 6 day period where it looks like nothing happened, because the test plan was deemed insufficient.
"If you change this, you also have to fix other pending issues" only took up 2 hours here. There are at least 2 or 3 other things I could fill in here that I'd flag as core issues with this process before I'd even think about dealing with this part of the process.
You are correct. Orthogonal to the PR process is incompetence of management being unable to identify key stakeholders and communicating the urgency and importance.
It should have been some VP realizes how important this is. Gets the security folks, testing folks, Senior devs, team manager in a room and task them at figuring how how to solve this very specific problem before it impacts a layoff.
I didn’t really understand the difference between good and bad management until I had multiple different managers and VPs over a decade at different companies.
The difference between good and great is night and day once you witness an exemplary communicator who can get the right people together and give them focus.
Usually I avoid making any improvement that is not directly related to the tasks at hand. Even adding a missing semicolon risks bringing this to the attention of an overzealous reviewer that will then make me follow a rabbit hole of legacy fixes.
I’d rather create issues silently so I don’t forget instead of even adding a FIXME or TODO. I think this part of reviews is broken. Resolving tech debt should not be a requirement for completing a task, it should be planned.
Resolving tech debt should not be a requirement for completing a task, it should be planned.
Such tasks will never be planned because there's always higher-priority tasks to be done. I actively encourage my developers to clean up the code they'll be working on, otherwise you're just piling tech debt upon existing tech debt.
The only exception is repository-wide refactorings: if those are necessary to implement a feature, we block the feature and create a prerequisite task outlining the refactoring needed. But this is mostly to aid the review process, in my mind these two tasks are still one (and if I work on it myself, I usually already have the feature half-implemented so I can double-check that the refactoring does indeed make the feature easier to implement).
I agree this is ideal, but my point is, there were instances where you try to cleanup, and you end up pulling an infinite thread because the feature you're working on is stacked on top of 10 other features, each of which were duct taped together, and product forgot about this, and they think if it's working it must be very straightforward to just add this one thing. Your choice is to duct tape again or to refactor the 10 features. So you dare change one like of code that optimizes one of the previous features and the reviewer asks you to also optimize A, encapsulate B and replace method C with new and improved method D that does some of what you need but you now need to rework your solution to conform to that. So I often leave that semicolon alone.
> I actively encourage my developers to clean up the code they'll be working on, otherwise you're just piling tech debt upon existing tech debt.
While I broadly agree on the boy-scout rule, it can severely throw out (already poor) estimates.
It shouldn't be a rule ("always clean up the shit you see when working on something else"), it should be optional ("If it doesn't mess up your original goal, clean up as much as is possible within the estimated time").
I fundamentally disagree. If every minor fix needs to be planned, it never gets done. The boyscout rule wins, and if your reviewers are not pragmatic, then that's an issue with their attitude.
Score creepers have no idea the architectural damage they cause. When you get over invested in one block of code, people will try to route around it. Then you get layers of these and pretty soon your code is the moral equivalent of Atlanta, GA, which is notorious for the number of ring roads it has.
An even better solution, imho, is that rules should be automated.
New rules are added, and the automation adds rule override comments to every existing violation - which also lets you track them. When you need to rush code out that needs to violate a rule, you add an override comment as well, with your name assigned to fix it.
Then over time, you build a culture that wants to fix these rule violations separate from feature development.
“I am very sorry to hear but my job depends on making sure we follow our procedures. Please follow outlined procedures before your change is promoted to production. Thank you for your cooperation. Sincerely, your IT department.”
That is absolutely true. Most companies have code review process that is totally full of nitpicking and trivial comments. I once suggested replacing that with static analysis tools to remove such comments and make feedback faster in our org, and was told that such code review is necessary for everyone. It helps people get promoted, makes people feel they prevented code issues, keeps the code review metrics looking good for the top level overlords who sit and monitor number of reviewer comments.
> replacing that with static analysis tools to remove such comments
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".
I suppose it depends on what kind of tooling you specifically are talking about, but I strongly disagree in the case of Python. When you have such an "anything goes" language, it really helps to have a tool that enforces consistent style across a large group of developers.
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.
"Over-complicated comprehensions" and "linting" are mostly unrelated. As mentioned my metric on when to comment is "does this comment address anything that's objectively wrong with the code?" and for "over-complicated comprehensions" the answer is probably "yes".
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.
To me it sounds like you’re over indexing on a specific linter tool that you don’t like. Prettier, for example, does not hard wrap. You tell it an ideal width and it will let lines exceed it or wrap early (within its bounds) to ensure that the code readability is improved when wrapping.
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).
> 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.
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.
You're argueing something completely different. I never mentioned anything about autoformatters.
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.
I wouldn’t say they’re arguing something completely different. A large subset of linting rules are by nature purely formatting rules. You can enforce line length with either prettier or a linter and both can auto fix the issue.
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.
> You can enforce line length with either prettier or a linter and both can auto fix the issue.
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.
A human ever thinking about where to break a line is bad. Let the computers do that, in one standard way for a project, and never think or talk about it again. The amount of time people have wasted on trivia like this over the decades is astronomical. It doesn't matter what the answer is, pick one, make it automatic, and move on.
Both things are true. It's worth arguing about so infrequently it's never worth thinking about whether it's worth arguing about, let alone argue about; but automated tools still make decisions far worse than any human would make sometimes.
There's human factors reasons for e.g. web sites optimised for reading not using the full width of a modern screen though (including ones where the unused side space is not, in fact, filled with gunk but left empty with the text down the centre e.g. 40% of the screen even so), and it goes for code as well - the hardware might have changed but the liveware hasn't.
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.
But if all code looks like the same homogenized paste, then all expressivity from code is lost. Which is fine if you want to write the 68th CRUD app with developers who don't want to work on it, but seriously, it does wonders to productivity when everyone enjoys the tools they work with and the work itself. Forcing everyone to satisfy tools/code reviewers just kills all the joy from programming, so you get everyone to be mediocre.
But if all pipework looks like the same homogenized paste, then all expressivity from plumbing is lost. Which is fine if you want to install the 68th kitchen sink with plumbers who don't want to work on it, but seriously, it does wonders to productivity when everyone enjoys the tools they work with and the work itself. Forcing everyone to satisfy standards/inspectors just kills all the joy from plumbing, so you get everyone to be mediocre.
// 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
After code review:
void fire_missiles(); // fires the missiles
void fire_missives(); // posts a witty tweet
...until someone has to work on someone else's overly clever "masterpiece". It's not so black and white and a tricky thing to get right and almost impossible to satisfy everyone's needs.
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.
Almost always, the "overly clever "masterpiece"" is unreadable because there is no proper documentation, not because the program itself is horrible. If you can't understand it because the writer didn't document it properly, shout at them for the lack of documentation, not for the unorthodox code style....
Workaday code should be documented so another developer can skim it in n minutes rather than spend n3 close-reading it.
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.
Others also seem confused— it clearly wasn't the best wording. Maybe the usage is more common in my regional dialect or something.
'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.
I'm not saying not to document nor did I say that the program itself is inherently horrible. Often we're working on one problem which will having us jumping into a function we've never seen before. If I have suddenly switch context to read a bunch of documentation to understand someone's "creative" coding style, I will not be happy. Agree to disagree, it seems!
I do think this is a real consideration. In my view, the best case is that the code is as boring as possible, in service to an exciting project. Or to flip it around, I think the desire to find enjoyment in the code itself is more pronounced when working on boring projects.
Our python tooling enforces an 80 character limit and every time I run into it I make my code worse in some way to comply. Usually it's just less descriptive variable names, sometimes it's less readable autoformat. I've given up fighting it, just let it stink things up.
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.
Python is a shit show, absolutely. However, all you have to do to get around things like arbitrary line lengths making code objectively worse is to either, 1) increase the line length something more reasonable/modern, like 100 or even 120, or, 2) agree as an organization which lint rules to disable as part of your org's official "style guide".
> 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
> Enforcing 80 characters is way too short for any reasonably complex code
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.
Human reviewers can also demand your code be made worse. Sometimes, the choice is making your code worse to satisfy a dumb tool (software), or making your code worse to satisfy a dumb tool (person).
If your coworker demands you make the code worse, you'd have to imagine that it's a difference of style. They may believe you're making it better. At least in that case I'm satisfying the style of somebody else on my team. I view that as more valuable than satisfying some tool that nobody agrees with.
Linters are configurable, so the way they are supposed to be used is that you get your team to agree on one lint standard and shove it in the auto-linter so that reviewers can point to a higher authority instead of arguing in circles, pulling in other people onto their side, and wasting a bunch of time.
Thanks for the links -- I understand the concept; I was asking more about specific examples related to "clean code". When I think of "clean code", I think "programming to interfaces" and "keeping domain/business logic separate from implementation details about HTTP/gRPC and database connection management, etc. Maybe I don't fully grok "clean code"?
Usually many architects tend to go overboard with interfaces for everything, repositories, data layers, hexagonal architecture, and whatever else is fashionable at conferences.
This is exactly it. Most code reviewers make other people jump through hoops to force them to code 'their' way, whereas more often than not there are multiple ways to solve a problem neither of which is substantially better than the others. In the rare cases where there is a much better way of doing something there won't be anything to argue about, it will be obvious.
That makes sense. The problem is that it can be quite hard on large codebases to find such instances if they aren't routed through some library to avoid code duplication.
It doesn't have to be excessive - it has to be reasonable. But I agree, outside of the strictly-enforced automation, just let it go and stop going "nitpick!"
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.
Where I work, we have static analysis tools.. plus the ability to "override" (tell the tool to ignore this case) when we feel it makes sense ("I know what I'm doing here"). And then peer reviews to help make sure those overrides make sense.
I worked with a programmer who littered his code with the incantations that disabled the linter, compiler warnings, static analyzer flags, and so on. His code passed all the automatic tools with nary a blip. Was it good code?
Static analyzers are a tool. And peer reviews are a tool. And compilers are a tool. Being able to combine tools together (as I noted we did) helps you generate better code.
The main thing is that static analysis tools, strict code style and whatnot put both a cap and a ceiling on the code quality. It will be a bit harder to write complete garbage, but at the same time, it has a negative impact on code quality because many of those rules are completely nonsensical and decrease code readability. Also, it puts a severe limitation on expressivity, which is a negative thing if you want your programmers to actually enjoy their job (which increases productivity...)
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.
Working on codebases - like rails without rubocop - is not something I would like to consider. There is already too much variability in how people code.
Sometimes there's a real prisoner dilemma thing going on here too. As a senior reviewing a junior's PR there are things that could be better but sometimes it's just not important. Are their variable names a bit too wordy? Did they have uneven amounts of whitespace between methods? In an ideal world these are more a "feedback for next time if it becomes a pattern" type thing. But as a reviewer, someone is looking at your comments per PR as an indicator for how much guidance you're providing, or going "oh who let that get merged" so you make the comment anyway. And then as the reviewee, they're concerned that leaving comments unaddressed looks bad for their level of responsiveness to feedback, or they think the reviewer might give them bad feedback for pushing back, so they'll then go and make the changes. But now they need someone to approve the updated version and the latency cycle starts again.
I always make it very clear in my comments if something is a blocker (this can't be merged before this is fixed) or a note (this would better fit the style of the existing code), and then i make it very clear in person that I do not take it as a personal affront if they don't fix my note comments. I find that this works pretty well. It has the additional positive effect of opening me up to criticism. If they understand that not all my comments are always vital or "right" I imagine it's easier to dispute anything else I say too.
Good call, I use “minor:” as a comment prefix for this but just realized that I got into that habit awhile back and the team composition has changed since then so newer members don’t have that context.
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!
I've been known to outright use 'nitpick:' - and if it's a nitpick that matters to me personally will tend to supply a diff for them to grab. Keeping the effort required to resolve it in line with how much it matters to the codebase and/or other developers working on it tends to make people a lot happier about the suggestion.
I think for optional I'd just write "It may be worth ..." and I think I might prefer using 'Maybe:' as a tag because 'Optional:' implies to me "I'm sure this is a good idea but it's not strictly required" which is enough different to seem worth separating the two.
The FYI tag is a fantastic idea and I shall try and remember to use it in future.
Or instead imagine that you work in company where you actually can influence the settings of your repos so you can enable or disable these features whenever the team agrees on it. IMHO decisions like these should generally be up to the team, and I generally do turn off the "changes dismiss approval" setting because I trust my team mates not to abuse this.
I'm increasingly of the opinion that this should be the primary value of code review. The goal is to make sure that multiple people working on a codebase have at least some idea of what's going on in every corner of it. Obviously that's never going to be perfect, and you won't understand code as well by reading it as by working with it, but at least having some sort of familiarity is invaluable.
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.
I agree with you and the person you are agreeing with. I have to add that using analysis takes none of that away. Using analysis creates a culture of analysis tools. This also can make the revealing of performance surprises a shared learning experience from the grayest beard to the greenest horn.
One thing I really really miss from working in person on a small team was that we did all PR reviews in person, almost like a little “thesis defense” session. This was medical adjacent signal processing software so that rigor was necessary, but it really helped make sure we all knew exactly what was changing and why, as part of the review was going over any output changes in our test corpus and making sure they were all for the better and understood _why_ an output value changed.
You just can’t do that async inside a GitHub PR to the same level.
Completely agree, analysis tools go hand in hand with code review as a learning tool. Partly, they take more responsibility away from finding the obvious bugs, and partly, like you say, they help surface problems that people might not be looking out for. I'd love it if things like performance checking tools were more commonly used.
> The goal is to make sure that multiple people working on a codebase have at least some idea of what's going on in every corner of it.
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.
> > The goal is to make sure that multiple people working on a codebase have at least some idea of what's going on in every corner of it.
> 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.
My larger point is that a code review can only happen after the programmer has changed some code. For anything that is flagged during the review it’s fair to ask: Was this requirement known before the programmer started writing code, and if so, why was it not communicated earlier?
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.
This is why I think architectural discussions should happen first (and typically decisions should be made collectively rather than individually). At one of the best places I worked, we discussed every new feature as a team (four engineers and a PO) first, drawing stuff on whiteboards and figuring out if we had enough information to solve the problem first. When the solution was clear enough for whoever was implementing it, and we all agreed the rough plan made sense, then that person would do away and get that done.
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.
Nitpicking is definitely a real thing. Perhaps it's the feel that they need to find something wrong with the code.
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.
> Nitpicking is definitely a real thing. Perhaps it's the feel that they need to find something wrong with the code.
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
To:
if something:
# Happy case
else:
# Unhappy case
If you have 10 devs working on a project you're going to get 10 different ways of thinking. If you're working on a 10 year old code base with a million lines of code and you don't address things that could sometimes feel like nitpicking then you wind up with a lot of technical debt and it makes it harder to work on the project.
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.
It'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.
if not something:
# Unhappy case
else:
# Happy case
To:
if something:
# Happy case
else:
# Unhappy case
This is the exact kind of rule that some people feel needs to be applied globally and will run into conflicts with rules like "put the shorter case first" so people aren't having to keep multiple cases in their mind at once.
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.
And sometimes the unhappy case is simply more "important". It's nice when code reads like prose, and matches how you'd describe something using natural language. Perl has the `unless` statement for exactly this purpose. This pattern isn't inherently bad, sometimes it's just what you wanted to say.
I don't see anything wrong with giving a feedback such as "I would find it easier to understand if it was written lile this...". I'm just stating my preference and if the author agrees (which sometimes happens), or if they don't care, they can change it, otherwise they may also ignore it.
> test coverage which hints maybe the author didn't consider these cases
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.
Static analysis tools and peer reviews can catch different types of issues, though. Just like a statically compiled language can catch some bugs that a dynamic language would not; but not everything. I'm a big fan of peer reviews, and I tend to focus my comments on
- 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.
This is what I implemented prior to my team growing beyond 1 person. If linting passes, it’s valid style. If you think the style is problematic, that’s an issue with the linting and formatting and not the PR. In practice this hasn’t been a problem. We’ve made 2 or 3 linting PRs over the years.
It’s why opinionated tools like rustfmt are so good. They set and enforce a standard which is good enough that even perfectionists don’t feel like arguing them ad nauseam.
Many code bases that I've worked on suffered from too much, and badly engineered, code - not from not having enough code.
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.
> Many code bases that I've worked on suffered from too much, and badly engineered, code - not from not having enough code.
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).
But not every code base is in an early startup phase. Once the company is stable there isn't really any excuse to knock out underengineered garbage just to get something out of the door.
I've worked in spaces where static analysis tools were run automatically on every new PR. Trust me, it's not as good as it sounds. Static analyzers are fully incapable of detecting the more nuanced mistakes, so a human touch will still be necessary. Nearly all of the "bugs" found by the static analyzer won't actually be bugs, but you'll have to "fix" them anyway because the reviewer (again, you'll still need one) will demand that all the warnings be cleared before approving your code.
Build a culture that prefers succinct, non-nitpicky code reviews. Static analysis tools only give reviewers more crap to nitpick.
Plenty of C/C++ static analysis tools have pedantic rules that flag correct code. Effective use of them means going through which rules you want to disable to minimize the unproductive make-work of satisfying the machine.
It depends on your team. If you have good juniors, they will relish the feedback. Of course it needs to be substantive. Often junior code will be far more complex and difficult to read than senior code, and code reviews are a chance to get that feedback across.
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.
The thing is, almost all the trivial comments can be easily automated. That would both give the junior dev quicker feedback before even raising the review, and save time for everyone.
Junior code is not usually more complex because of bad naming conventions and inconsistent styling and related issues that tools can help with, although of course these don't help its readability overall. Rather it usually has control flow that's far too literal a translation of requirements leading to an exponential explosion of paths, insufficient and/or over-eager error handling (often both at the same time), poor representational choices leading to inefficiency or more special cases, and so on. None of these can be effectively automatically detected.
As an intern doing work in an unfamiliar framework, it was really valuable to get code review from seniors about how to better go about solving a problem structurally. Not something static analysis would've caught.
I _kind of_ understand in the sense that - it can feel bad/like wasted time to do code review when you don't leave a single comment.
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.
I’ve reached the point where my reviews tend to be either instant LGTM, or “please formalise the idea behind this into a design doc and submit for review to the TL forum” (I.e. a polite “I think you don’t know what you’re doing”). The usual “how about ${BETTER_NAME}” can be left optional, there’s very rarely anything actually bad by the time people submit for review. Is this a rare culture?
This definitely isn't my experience, but I work in C++, where having a second eye to look over things is... valuable.
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.
if it is "your" company but you cannot change it, I put that it is not your company. but that in fact you are just its employee and not the other way around.
Except that phrase is a pretty well-known aphorism. It's sort of the equivalent of using map and filter or reduce on a collection - just because a programmer hasn't seen map, filter, and reduce doesn't mean it's wrong. Perhaps the programmer needs more experience.
I am doing code reviews for a large manufacturing company with dozens of plants; that code is supposed to be deployed everywhere, so the entire company is at stake.
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.
I agree that formatters and static analysis tools are the best way to reduce trivial PR comments. However, I would say that the kinds of things addressed by formatters and static analysis tools, and similar issues that tools don't catch such as misspellings, are worth addressing in PRs. "Nitpicking" is in the eye of the beholder, and your company might have a particular culture problem that I haven't seen, but many of the things I've seen called "nitpicking" are legitimate readability issues.
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.
I have been told that we need 2 people to do code review, in a team of 3 people, and we can just “ask a member from another team who works on the same product”. We were told that outsiders can act as external bar raisers who will comment on “overall flow and code quality”.
Sounds like glorified and highly opinionated linters whom will just gatekeep progress because they “don’t get it”.
> keeps the code review metrics looking good for the top level overlords who sit and monitor number of reviewer comments
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.
Running SQL queries on the data warehouse and generating reports saying very few comments on reviews is much easier than bothering to look into the comments. I ran into a high level engineer making very high amounts of money who would just do such vague things all day and claim the org needed more comments.
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.
Hah, I have a very different experience where I work. At my org every time I submit a ticket requesting access for somebody to such-and-such a system it typically gets done within a couple minutes regardless of what priority I give. I’ve sometimes wondered if help desk staff snap them up as soon as they come in because they’re fast tickets to close and lets them improve their personal metrics.
It sucks when you say it like the title: "6 days to change 1 line of code",
But... The system improved in a few other ways:
1. The setting became configurable via the parameters table instead of being hard coded.
2. Auditing was introduced to track changes to that setting.
I'm not trying to defend bureaucracy, because I truly hate that aspect of large organizations, but just point out that additional value was created during those 6 days beyond what was originally set out to be achieved.
Incidentally, this is why you have to bake in a set amount of overhead into your estimates, and need to take this red tape into consideration if you do story pointing.
The only reason the parameters table would be useful is because making changes to the code had so many things blocking it. Similarly, auditing for this sounds unnecessary because it used to be in code which means that you had source control as your audit trail.
So your two wins were basically a "win" of not dealing with extra ceremony around code changes, followed by a "win" of recovering the functionality loss of the first "win" because future changes to this wouldn't go in the code.
Yes but it also did something that is potentially way more dangerous than the original ask. I would argue that pushing a parameter from a hard-coded value during an immediate outage or real production concern is stupid. There are way more potential pitfall there. IMO, This should have been handled with a:
OP: Please accept the 1 char PR, this is urgent. I just created a ticket to track the requested enhancements. Let's address production concern first then then I will get the rest after.
Reviewer: LGTM!
This is actually where seniority pays, if most of your engineering base can't navigate between rules and guidelines, the organization is crazy.
Step 1: determine true priority. How long can this take before it affects jobs? Everyone needs that in mind.
If a week won’t affect anyone’s jobs, follow the process or minimally alter it. If people are on furlough because of IT, you have everyone needed in the same room (physical or virtual) until the problem is solved.
We don’t have that context here.
But if Ed and the entire chain doesn’t have that context, That’s a system failure. The senior would likely just insist on a second ticket to fix it right after if he knew someone’s rent check was on the line. (And if not? That’s also a management issue to solve.)
The urgency of this problem was not communicated broadly enough and the impact of the simple fix was not communicated back up the organization so that it could be prioritized correctly.
It seems like Ed is just being "helpless" and blaming the organization when maybe he could have raised the issue back up to the president saying "my 1 line of code fix to your problem is being blocked by these people. Can you help?"
This was a communication problem as much as anything.
I think I agree with you, but to play devil's advocate:
> Which were not required
Sure they were not required by Ed, but they were required by other stakeholders who are responsible for testing and maintaining this system.
I agree though, it would be frustrating to get into this sort of "yak shaving" situation where you don't even care about all these other requirements that are being added.
Like I said in a sibling comment, this is really more of a communication problem on Ed's part, and maybe he could have neutralized all these problems with better communication.
Auditing requirements could have been covered by version history for the file containing that hardcoded value, I think? They would have other problems if they didn't have version control.
This is a story where a one line change of a hardcoded value actually went well.
I could image a scenerio where somebody stored the number of months of backlog as a 2 bit value. 0, 1, 2 or 3, you know, to be smart and clever. This may not appear as a problem during testing because it may be hidden many layers down, in some downstream service that is untested. Maybe in some low code automation service....
Changing it to 4 would mean the backlog is 0. Who knows what the consequences might be. Would that service go and cancel all jobs in the production queue?
Would it email all customers mentioning their stuff is cancelled?
I get that this is a seem gly easy change, but if a change of policy is expressed to the software team as an urgent problem, this seems like the management team needs better planning, and not randomly try to prioritize issues.....
The audit trail likely represents actual risk reduction of someone undoing or misunderstanding the change later, since the change has no meaning outside the context of the request.
"Fixing preexisting errors that violate new company policy" also arguably involves real risk reduction; you gotta do that work sometime, and if everyone in the company agrees the time is now, the best time is now.
Using Marge instead of Homer is not "risk reduction" but presumably testing accounting close is also critical.
Tony's request is also reasonable, unless you want to leave the next dev in the same shithole you were in re. the wiki state.
On the flip side, if nearby things are never updated to match changing understanding of the system, then very shortly the code will be cluttered with possibly dozens of different styles: multiple naming conventions, constants in some places, hard-coded values in others, and values read from a parameter file in others, and other kinds of variations. The result will be a chaotic scramble that has no clear structure, and requires programmers to know and understand multiple different ways of expressing the same business concept.
You said, "It's a bad idea to rush through refactors".
What constitutes "rushing through" a refactor, and what forces and context make it bad to do so? What can we do, if anything, to make it so that refactoring is as much a part of everyday development as the CI/CD process, and thus becomes just part of the work that's done, not something to be put off until the business decides there's nothing else with a higher priority?
In the linked article, the situation was that they absolutely needed to change some hard-coded MONTHS value from 3 to 4 in order to keep the factory running.
That change should be shipped in isolation. With manual testing to cope for the lack of coverage, presumably.
Refactoring doesn't have the same urgency as keeping the factory running, no matter how much we all believe in keeping the campground clean. It can wait until Monday in this particular case.
OK, but that's not the question I asked. What I want to know is how we can make refactoring as much a non-negotiable part of the process as code review, tests, CI/CD, or whatever you consider essential, non-skippable, even under a short timeframe?
In the context they were in, the answer to all of those questions is "shut the fuck up, we can talk about it later".
In the normal course of business, it's a different conversation. Even then, if you're making some poor dev refactor a bunch of code because they had the misfortune to touch it, maybe you should have done the work yourself a long time ago. Or written a linter and ticketed owners.
You don't want to make feature development some sort of pain lottery.
Unplanned work being arbitrarily scoped into sprint is pain. Doesn't matter the source.
In practice, your approach often turns into "junior engineer abuse" where they have to clean up a bunch of unrelated pre-existing stuff to make the seniors happy as a condition of shipping their feature.
There's all sorts of ways it could go wrong. Perhaps the real question is where blame will fall if it does. If the big boss says "I decided to take the risk and push this through, I accept this was a consequence of that", great. If the programmers get beatings, not so great.
the other thing i notice from the story was that an update on something considered mission Critical was not given an update on within 24 hours.
IT should have volunteered the info regarding how far back in the backlog this was classified as soon as that prioritization was made. "Behind 14" and with many people on the testing side occupied is obviously not going to help with "layoff level priority".
To me, the classification of "enhancement" just doesn't seem to capture the urgency.
I think the correct people and processes were followed, but they could have saved a great deal of time aligning on the importance and priority of the task by putting together a meeting with the leads.
For a time-sensitive and critical update to core functionality, the director of operations should have been aware of the mean time to deployment for the software and put together a team to fast track it, instead of entering it into the normal development pipeline with a high priority.
Code review starts out with good intentions. But some gatekeeper inevitably sets up shop and starts rejecting everything for trivial reasons. Says they're interested in keeping up 'code quality'. But nothing is worse than keeping buggy code around long after the fix is ready, or delaying features so nobody can try them.
I'd recommend a process where comments are allowed but reviewers cannot block commits. Each developer is trusted to be careful, make such changes as are pertinent to the task. Use CI as well and depending on the team it can all go very well.
Then an engineering leader needs to stop that person. Dysfunction can manifest in many ways, and overzealous reviewing is one of them. Changing the process so people can ignore pathological reviewers is a half measure at best.
I’m torn about blocking. I do get the sense that it’s frustrating to get that big red block, so in many cases people ask for changes without blocking - basically a “soft block”. But I’ve dealt with plenty of cases where a PR is so far off the rails (usually a jr. developer), I think it’s appropriate to send a message.
For me, it really depends on the other person. There are some devs that I trust to carefully consider my comments (even if they end up disagreeing), so unless something is catastrophically wrong, I will still approve.
But there are some other developers who seem content to just routinely (not just sometimes) ignore anything that is not phrased as a command (such as "I think we should test this better"), and I've adopted the habit of not approving such PRs immediately.
this works well when coverage/test quality is high. which is in itself not something that magically happens if you just let devs move as quickly as whatever manager thinks is necessary right now.
My comment is more a meta commentary on factory workers vs software developers.
This company leader is willing to lay off factory worker because of 10% under utilization, he can tweak some variables to increase productivity but ultimately the choice is either full utilization or unemployment. He can do this largely because, I assume, these workers are replaceable and can be rehired during busy season and because the profit generated per employee doesn't allow for any inefficiency.
I work as a software developer. Our under utilization has to get well over 90% before anyone is even considering getting rid of us. Many of us are putting in four hour work weeks. No one manages our minutes, our bathroom breaks, etc.
We are in a period of major capitalization of software. It won't last forever. Eventually the major infrastructure of the IT world will be built and the industry will shift to a maintenance mode. Most of us won't be needed, we will become replaceable and the profit we generate in maintenance mode will be insignificant to what we see today.
Factory workers are usually fired within minutes or hours of detections in low productivity of an individual worker. In our life time, I believe this will start happening with software developers.
> These workers are replaceable and can be rehired during busy season
That's exactly the difference. The factory is a system of processes designed to remove decision making and variation from each person.
You have to evaluate the degree to which that could be done with your skillset.
> in a period of major capitalization of software. It won't last forever.
I agree with your basic point-- not every company needs engineers developing novel software all the time. It's more of a boom and a bust creative venture, like producing a movie. If you choose to work in development over IT you should accept that risk. But I don't see why this has time has to be the peak.
Anecdotal experience: after some years in the team with formal code reviews, moved to the team/company without code reviews. Everyone is free to commit/merge to any branch. When joining the company, I had somewhat mixed feelings about this. Apparently, it felt so refreshing and empowering, I became productive within few days.
A while back, I also worked for a team that did "Catholic Code Reviews" (push and pray).
No code review worked very well given the objectives of the team. It was an R&D group who's primary objective was "demo the nifty new thing to the executives". That meant a lot of short notice requests but also a lot of throwaway code. We'd demo the thing, the exec would be like "looks great, no business case" and the repo would never be touched again. Of course, every now and then our thing WOULD get productized and the downstream team would be responsible for turning chicken-scratch code into something production worthy. Those guys hated us with a burning passion.
I've seen this work really well on small teams where there's high degree of trust, ~80% test coverage. It was a no PR process, just merge with master when tests pass, UX(if applicable) demoed successfully to stakeholders and you are satisfied with it. New team members were assigned a mentor that sat beside and frequently paired and reviewed their code for the first 2-3 months.
Was a 2.5 year project, went live at 20 months in, was on time and meet budget with more capability than originally scoped.
Many days had 2-3 hours of discussion at the whiteboard. It was informal, didn't always involve everyone.
Oddly, we had three different pms during this project. We had a hard rule of no email or contact outside of stand-up with them. Two of the three pms were able to 'work' with this setup. The head of airport IT eventually realized after two years of this that we didn't need a PM.
We had a rule that if you are doing anything novel in the code base you had to have a conversation about it with at least another dev.
We all sat within a few feet of each other in a large private office with a huge whiteboard. We used index cards taped to another dedicated whiteboard for stories and if you couldn't describe the essentials on that, you had to break the story into smaller pieces. We were allowed to build our own machines and use as many monitors as we wanted. Was an Airport billing and tariff system for a large international airport, the accounting department head, director and other users were a couple of office doors away. They rarely missed a stand-up and they had an open policy for any real time questions. Standups were informal discussions, demos, question and answers not typically for status. The status update just took one glance at the cards on the whiteboard.
The final system improved revenue by 8% in its first and every following month. The director of Accounting was brought before the Airport Authority Board to explain. Billing disputes and adjustments with airlines went from nine days a month to one. The monthly billing workload went from 18 days to 5. Instead of a senior accountant being the primary user, they were able to offload this to one junior accountant with three years experience.
Bug rate was six production bugs, zero incorrect invoices in the first year live. I don't have any data after that.
The previous rewrite attempt failed after a three year attempt.
Using your code review process to hold changes ransom until they meet some lofty ever-changing team ideals, is disfunctional.
"Upgrade as you go" policies leave long tails of half-completed conversions that make the codebase harder to ramp new devs on. You'll also never finish the conversion because there's no guarantee that product focus will walk every part of the codebase with any regularity. Some product areas get left alone for years and years and years.
It's either important enough to cut over to the new policy in one concerted project, or it's not important.
Reading this as a problem with code review is just wrong. The problem is that a company let its processes, which are made up internal barriers, get in the way of its principles.
Every process needs an escape hatch. Changing something which will prevent layoffs should trigger every escape hatch.
A few years ago, working on a comment team, I created a function to do an AB test using an AB test team system. Someone else does the implantation, not me. Someone didn't like the function, as they didn't use the company's cache system (the AB team previously explained all the reasons why the cache is not used, etc).
I started to participate in several meetings to explain that it does not use the cache system because bla bla bla.
A week or two after a few meetings (and a lot of money spent), I got it deployed.
It's the same company that created a new meeting to discuss wasted time in all the other meetings....
At the time I thought: This is abnormal, but it is actually an extremely common scenario in many companies.
Shirley's review was accurate for a non-urgent MR, but was bogging an urgent ticket. Ignoring them would not have increased risk, but the policy is important to manage technical debt. A simple resolution in this case is to move the comments to technical tickets, assign them to Ed for later, and approve the MR.
yet during interviews you're expected to write 50 lines or 100 lines of code with tricky algorithms under pressure in 30-45 minutes from design to implementation to unit tests and corner case considerations, also you must keep talking while coding with your interviewer so you behave like a team player and is collaborating with your future team mate on the fly.
all these have nothing to do with real life coding, it's more like a coding monkey stress test to me, you can only perform well by doing daily leetcode for months, it's geared towards young people who has those times to practice.
I recall the days I spent two weeks to add 50 lines of code to linux kernel, 95% of the time was to figure out how that kernel subsystem works and where to hook the code, but that matters 0 to nowadays FAANG interview process, the 5% of time for 50 lines code is all that matters.
Im still unclear how people get bit by this kind of thing.
The way ive always operated is that nothing in a Code Review can stop a merge unless its a rule written down before the review or its just a blatantly obvious incorrect miss of requirements or functionality.
I love seeing peoples comments about how things can be improved in reviews and learning new approaches or context, but if there's not reference to a written rule I should have known about beforehand anyway, I'm not changing it if I don't agree with it.
> The way ive always operated is that nothing in a Code Review can stop a merge unless its a rule written down before the review or its just a blatantly obvious incorrect miss of requirements or functionality.
I've never worked on a team where code reviews worked that way for anyone besides maybe the team lead. Even if nobody uses the GitHub feature to add a blocking review comment, there's always this expectation that nearly every review comment has to be treated as "right" by default, even if it's ultimately based on opinion, and your job as the submitter is to either debate or accept every opinion before merging; this of course means a few changes lines can take days to get merged, unless everyone jumps on a call to have a synchronous code review, which nobody wants to do every single day.
The reality is most organizations don't write anything down, and even when they occasionally do, there's a high probability that said documentation is outdated. So while I like the idea of the written rule principle, I don't see it being widely adopted, and there are many more developers who actually believe it's better that every decision just lives in people's heads.
If you have need unanimous consent to merge then yea you'd probably need to address every single comment. Otherwise you can always get the one or two approvals you need from people you know are smart. If they're good with it and nobody is pointing out rule violations, just nonsense, I've never felt at any fortune 500 company, that I couldnt merge.
I do see people who seem to believe that, but they're always junior and it's all in their heads. Nobody ever said that you need to listen to every single comment.
Write good code, incorporate good suggestions, ship stuff fast, test well, deliver working software that solves customer problems. That's your job.
I disagree with this. For example, lets say an engineer implements a 1000 line function and another engineer says, 'A similar function already exists. Just add a parameter and change these 2 lines'.
I don't think we need to over index on written rules for common sense recommendations like this. I'd be happy to see the first engineer blocking another's PR on the request. In the long run it isn't viable to optimize for shipping, I think a lot of the optimization has to be directed towards maintenance.
Common sense is never common. I'm sure you can find something written to block nonsense like that - is it fully unit tested? You do have something written that requires unit tests right?
But at the end of the day, if you're on a team with somebody like that then you NEED written rules to deal with that nonsense and the Code Review process is the first step to identify it.
The nonsense gets merged the first time, but not before you add a written rule against it going forward. Problem solved.
There's another failure mode not described here: "product management doesn't understand the change".
I'm in the (perhaps enviable, perhaps unenviable) position of getting a small subset of my marching orders directly from our CTO. Engineering at my company is organized in such a way that, while engineers report to the CTO, product management (part of engineering) does not. When the CTO asks me to priority-override a new feature, or even a small change, I've sometimes gotten into arguments with product management over how this wasn't planned for, and how they need time to adjust schedules and collect requirements before I can begin work. Unfortunately, I can't use the "talk to your boss" excuse, due to that quirk of the reporting structure.
One memorable example of this was a feature request which came in hot in mid-June, which I had code complete later that day. We actually shipped it a few days ago. To product management's credit, they _did_ come up with requirements that were not met by my initial change, but I question whether releasing fast and iterating, or releasing correct after four weeks, was better for the customer.
Does your CTO (and whoever PMs ultimately report to) intend for these types of friction to occur every single time a CTO request appears? Does your CTO notice that these small changes takes extra time to occur? Does your CTO even care?
My point is that the product manager also has a boss, and both the CTO and that boss have a shared boss (the CEO).
If the CTO and product boss are constantly stepping on each other's toes (or whatever), that's a problem that they either need to solve, just acknowledge as just the cost of doing business. And that ought to be communicated and acknowledged by everyone in the line.
Not sure I see the point, rules are in place for a reason and it's not like changing 2 lines would have taken 12 days.
This seems like it changes something with potentially super-high impact, 6 days from request by the CEO to release to production is a pretty good release time.
It is not a good release time for the described scenario. The company badly needed a change to survive so fixing unrelated issues in the same change doesn't make sense.
The problem (and solution) isn't necessarily to change the process. Assuming good faith and competence when designing the process, all (or similar) of the downsides encountered in the scenario were considered when designing the process, and the cost-benefit came out ahead to add the extra safeguards the layers.
As demonstrated by the end of the scenario, the 'solution' was the president to more directly intervene.
The failure in this scenario isn't necessarily that the process got in the way, it was that when the President and IT Director kicked off the action, they were not fully aware of the efforts and overrides required to escalate to 'do this now'. A converse failure is that the intent of the President and IT Director was not properly communicated to all stakeholders in the process.
Yes, such rules and processes are often made in reaction to something bad happening.
I like the description Jason Fried gave of policies being "organizational scar tissue" in response to a "cut" (i.e. something bad happening). He suggests that we don't scar on the first cut:
Yep. Nine months to change a single buggy line of cache line invalidation in the translation look-aside table handling IMUL opcode in QEMU 4.0.
imul eax, [ecx+0x41], 0x10
Entails hundreds of hours of single-stepping through that opcode in Linux kernel using an indirect operand pointing toward its own opcode (self-modifying code).
Even the extraordinaire Fabrice Bellard (author of QEMU) admitted that it is broke and did a total rewrite, which fixed tons of other issues.
What procedure mitigating the supposed risk of this change was skipped due to management pressure according to TFA? The way I read it, the original change of the constant "3" to "4" was tested, it was observed to have the desired effect, and none of the delays before the coding and the testing by the developer or after it did anything to reduce the risk further.
All deadlines are "artificial" and some have a lot of $$$ attached to them (due to regulatory/market/customer/supplier issues)
But I know, a blame culture begets a CYA culture, which begets the opposite of "help me help you"
(hence why such a parameter is hardcoded in the code - the issue here is not stopping the ~~orphan~~ developer crushing machine but why does it exist in the first place)
This conflates two kinds of slowdowns. Post-implementation stuff like thorough peer reviews and automated testing followed by manual QA is usually a net gain on a long enough timeline. The damning and soul-draining part is pre-implementation -- how many people need to agree that the line of code will be changed and how many people must opine on the change before it can be shot over the Dev | QA wall.
The worst place I ever worked at had mandatory "ticket refinement" - a feature or a bug couldn't be worked on until it had been through a "refinement session", in which everyone had to chime in, and the ticket wasn't considered refined until everyone on the team fully agreed with the implementation. Naturally over time, this devolved first into writing pseudo-code in Jira tickets and then into near-production code in a google doc overflowing with comments. Leaving that company I told my manager "this is a team of 4 'senior' engineers, I'd suggest you get one proper senior engineer and 3 typists". Nowadays, you could probably shove minutes from those meetings into copilot and call it a day.
Of course, the flipside of this was that comp was about on par with the market, and the perks were top-tier. If you're okay with doing the bare minimum while spending ~20-30 hours a week on total nonsense, it's probably the dream job.
The president orders a massively impactful change and various processes in the company work to make sure it is safe before launching it. Sounds fine to me.
If the president were told "this is risky, we have to verify that nothing will catch fire if we store it for more than 3 months" (or similar) I'm sure they would have agreed to proceed with caution, but still treat the matter as high priority.
This is not about safety, this is about ticking boxes. Boxes that make sense, but which are irrelevant compared to the potential impact of the change.
For example, replacing the constant with a file parameter is important, but completely out of scope of the PR, and could lead to unexpected impacts. Sounds like there are enough things to QA with this small change that there’s no need to add other sources of potential bugs.
The naming of the configuration key is also completely ridiculous and a huge waste of time.
I’m sympathetic to the pain for the programmer, but I still have so many questions:
The months of backlog to build against was hard coded? Changing it requires rebuilding and redeploying the entire software stack that runs the factory? This must be some custom in-house MES? The testing requirements to redeploy that must be extensive right? If that is hard coded, who knows what else is. And if the software doesn’t work right, you end up buying or making the wrong parts?
I take issue with increasing a backlog to increase utilisation of resources.
When you try to make every part of a process operate at 100% capacity, you risk creating bottlenecks. There's always some variability in how long tasks take. If every part of your system is running at 100% capacity, then any small disruption or variation can cause a backup. That backup then propagates throughout the system, causing delays everywhere.
This is a culture issue. If people get paid to look busy, they will find ways to look busy. There's no incentive to close tickets quickly, because it will only result in more work.
The example situation is a bad representation. The process is needed for large changes and the downside is that it happens for single line changes too.
Very little of this seemed unreasonable, with the exception of:
> Pissed off hours spent on Hacker News: 14.
I hope it was an exaggeration, but also well within the realm of possibility.
This is the only bit that looked like a problem to me:
David: It's for Philip. It we don't do this right away, we'll have to have a layoff.
Judy: OK, then I'll fill out that section myself and put this on the fast track.
2 days later.
If humans understand a change that's in-progress to avoid layoffs, then there's no reason to effect layoffs dogmatically because of a policy which is relying on a laggy process.
There's good stuff in here too:
David: Forget the queue. Mark it urgent and send it to Ed immediately.
1 hour later.
Ed (programmer): On line 1252 of Module ORP572, I changed the hard-coded variable MonthsOfBacklog from "3" to "4"...
Shirley (Code Review): It is now against company policy to have any hard-coded variables. You will have to make this a record in the Parameters file. Also, there are 2 old Debug commands, an unassigned variable warning message, and a hard-coded Employee ID that will all have to be fixed before this module can be moved to production.
Ed: I don't have access to Marge.
Julie: Then contact Joe in IT Security. He'll get you permissions.
2 hours later.
Shirley: Your new Parameters record "MonthsOfDemand" needs a better name. The offshore programmers won't understand what this means. Also, it should have an audit trail of changes.
And more bad stuff
Ed: What policy is that?
Shirley: It's not exactly written down anywhere. The offshore team is 3 months late updating the wiki, but I assure you, all new Parameter records must satisfy new naming requirements and keep audit trails.
1 day later:
Why would a variable rename take 1 day, perhaps that disgruntled time on HN?
This is bad on both the process-side as well as execution. It shouldn't take 2 days to write a test plan as unnecessary as it may seem. Perhaps that's another day spent on HN?
Tony (IT Testing): I see 129281 on Marge, but I have no Test Plan.
Ed: Just run it the old way and the new way and note the increase in the total on the WorkOrdersHours report.
Tony: That's your test plan? No. This affects everything in the factory. I have to have user selected Test Cases, Expected Results, documented Test Runs, and user sign-off.
2 days later:
I've read about bad processes and have even worked in worse environments. This story is nowhere near as bad as it gets. At multiple points problems were solved in <= 2 hours by humans, presumably overriding any silly processes.
> Why would a variable rename take 1 day, perhaps that disgruntled time on HN?
When I work in an environment without bullshit I love my job and get lots done.
When I work in an environment like this I hate my job and have no motivation to finish two ticket in the time I could finish one, because it would means twice as much bullshit.
The thing is, there's very little pure bullshit here: only some unexpected requirements (which do make sense), a bunch of bumps that did have human workarounds/shortcuts, etc. The urgency of the whole thing was synthetic though, which upon recognizing why it's being done could wait until it gets done--waiting some extra days shouldn't necessitate firing people you want to keep.
The other strange part is the "6 days to change 1 line of code" - yes, this one line of code took six days to change. But the actual time investment of each person, even the programmer involved, is much less than those six days. Probably lots of other lines got changed in those days by those same people, including the author, so you're not really offering any useful velocity measurement by pointing to this one line that takes six days to change. (And presumably that's not even true if Ed touched some other lines in ORP572.)
The people here commenting that this is outrageous have missed the boat a little bit.
This 'one line change' is an existential and fundamental effect on the company, it may very well affect operations otherwise.
'One line of code' can easily blow up a system, and the checks are not there to for the 99% of time time we are 'all good' it's for the 1% of the time when there is sketchiness.
Someone complaining about 'hard coded variables' is perfectly right to do so.
Every single element in this situation is in place for very good reason.
Imagine someone wanting to replace the lock in your Airline door, mid flight - oh, we'll just use a 'regular screwdriver' instead of the one designed for the door, who cares!? It's just an airplane!
The gripes are misplaced: the solution for this situation probably is to have a war room/crunch room situation to move the issue quickly and correctly through the hurdles so it can be done within the timeframe needed.
All of those 'delays in between' were the problem - they did not respect the urgency of the situation.
That should be the point of push back “Great feedback, and appreciate the direction to improve code quality, however the implications of changing Y means we need X, Y, Z approval and it will many days. I am making a tech debt task with what you described and will be done in a follow up PR based on priority and bandwidth.
Let’s focus on what it would take to get this surgical PR out.”
The biggest lesson I learned is to make focused PRs, and learn to pushback when reviewer asks for a scope creep. Mostly other engineers have been pragmatic.
It’s unrelated to number of lines as you can reformat while codebase but not change anything logically, or change some feature flags which could have a huge impact. But ONE FOCUSED CHANGE AT A TIME