6 days to change 1 line of code (2015)

> 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.

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.

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.

> The biggest lesson I learned is to make focused PRs, and learn to pushback when reviewer asks for a scope creep.

Whenever that happens just add a TODO ticket. Unblock production and the system is not worse for it

“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.”

it is not about code quality, it is about power lmao

> 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".

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.

In some environments, that is true. But the process of review can also help build shared knowledge and understanding of the changes and the code base.

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.

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.

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.

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.

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.

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.

This week during code reviews, I caught:

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?

If you can't change the Company, change your company.

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.

Agreed. We don’t need human linters: https://bower.sh/human-linting

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.

I have given up on review process.

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.

Unless it’s them getting a P1 security alert because you ran “npm install”

our security team actually responds even faster. they simply deny all requests automatically, but immediately.

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 takes multiple weeks to get someone added to the AD group required for wiki edit permissions.

2 programmer hours, obviously

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.)

> It sucks when you say it like the title: "6 days to change 1 line of code"

Which just states the fact.

> The system improved in a few other ways

Which were not required.

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.

And it just cost 6 x 10% x $MM worth of daily products! /s

None of the requested changes involved more testing or risk-reduction.

They actually increased risk by insisting on refactoring a bunch of nearby things as the "cost" of the change.

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.

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.

Knight Capital!

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.

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.

I abhor “every code change needs a reviewer” rule. They are a massive distraction and don’t necessarily lead to better code.

> 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.

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.

sounds like a security and audit nightmare to be honest but I guess it’s an agency or similar with smaller projects

Yea it’s atrocious as management is basically derelict in planning.

They are relying on random unrelated work tasks to trigger these unplanned work time bombs all over the code base .

Either some new standard is important and the code should be updated, or not. Just relying on randomness and slowing down urgent work ain’t a plan.

> It's the same company that created a new meeting to discuss wasted time in all the other meetings....

I've been in this kind of meeting. It was a 30-minute meeting instituting a rule that meetings should be 25 minutes by default.

I worked for a company with a few thousand employees. Everyone had to spend about 6 hours in meetings about how to have more effective meetings.

VPs and managers droned on about how important it was.

Rule 1 in the training was to have a detailed agenda.

As far as I saw nobody ever followed rule 1 except myself and one other person.

I’m glad I work at a small company now.

You might be surprised at how common this is.

Real world programming and interview programming are like 2 different disciplines at this point.

> 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.

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.

Why can't you use the talk to your boss excuse?

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?

Sounds like a horrible place to work.

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.

Exactly. 6 days latency is great for a complex system. In heavily regulated industries this can easily be 6 weeks or 6 months.

When I see rule or process my first thought is 'what happened that led to this rule or process?'

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.

Yes

Because, in this case, the buck stops with him

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 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 disagree, you can fork and apply whatever change is blocking you to the OSS project.

> 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 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.)