[HN Gopher] 6 days to change 1 line of code (2015)
___________________________________________________________________
6 days to change 1 line of code (2015)
Author : tempodox
Score : 309 points
Date : 2023-07-16 12:01 UTC (11 hours ago)
(HTM) web link (edw519.posthaven.com)
(TXT) w3m dump (edw519.posthaven.com)
| short_throw wrote:
| I was just waiting to hear that the parameter loading wasn't
| available in some edge case scenario and code review changes took
| down the system.
|
| But actually this is a story about the process working.
| willsmith72 wrote:
| 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.
| hmaarrfk wrote:
| 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.....
| twic wrote:
| 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.
| hmaarrfk wrote:
| 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.
| qxmat wrote:
| Knight Capital!
| hmaarrfk wrote:
| https://www.bugsnag.com/blog/bug-day-460m-loss/
|
| It made me laugh! And cry inside
| woooooo wrote:
| 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.
| morelisp wrote:
| 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.
| cratermoon wrote:
| 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.
|
| Now _that_ is truly increased risk.
| woooooo wrote:
| Long term vs short term. It's a bad idea to rush through
| refactors.
| stasmo wrote:
| 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.
| arcbyte wrote:
| 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.
| tyleo wrote:
| 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.
| arcbyte wrote:
| 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.
| ravenstine wrote:
| > 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.
| arcbyte wrote:
| 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.
| synergy20 wrote:
| 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.
| crop_rotation wrote:
| Real world programming and interview programming are like 2
| different disciplines at this point.
| guy98238710 wrote:
| It's so sad and also terrifying when people call this real
| world programming.
| qsantos wrote:
| 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.
| qxmat wrote:
| 6 days is pretty reasonable to me because I'm an avid supporter
| of the release train (miss the release train, wait for the next
| one).
|
| Governance is an important aspect of the software development
| cycle past the PoC stage. Releases shouldn't lead to an
| existential crisis.
| hermannj314 wrote:
| 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.
| starcraft2wol wrote:
| > 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.
| zac23or wrote:
| 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.
| duxup wrote:
| 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.
| crop_rotation wrote:
| You might be surprised at how common this is.
| jawns wrote:
| > 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.
| scruple wrote:
| I sat through that one, too.
| gaff33 wrote:
| Only 6 days? That's impressive!
| iLoveOncall wrote:
| 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.
| crop_rotation wrote:
| 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.
| moralestapia wrote:
| >The company badly needed a change to survive [...]
|
| Lol, nah. This is fiction, no company IRL is going to start
| laying off people after a few days of noticing a small dip in
| production.
|
| The whole premise of "We own this company but if this code
| change doesn't go through in X days we will fire people
| instead" is completely stupid.
|
| But it is an entertaining story and it gets the point through
| that internal bureaucracy gets in the way of many things.
| iLoveOncall wrote:
| I could somewhat agree with that, however it seems like this
| only added a couple of hours out of the 6 days.
| icegreentea2 wrote:
| 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.
| sz4kerto wrote:
| Exactly. 6 days latency is great for a complex system. In
| heavily regulated industries this can easily be 6 weeks or 6
| months.
| 0x445442 wrote:
| This is true and why I'll never work in the banking sector
| again.
| matwood wrote:
| When I see rule or process my first thought is 'what happened
| that led to this rule or process?'
| ReflectedImage wrote:
| That's hardly relevant. The bureaucracy will increase until
| the company goes bankrupt.
| justin_oaks wrote:
| 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:
|
| https://m.signalvnoise.com/dont-scar-on-the-first-cut/
| shpx wrote:
| Better than waiting a year or more for an open source project to
| make a release bumping a dependency because no one cares.
| tbarbugli wrote:
| I disagree, you can fork and apply whatever change is blocking
| you to the OSS project.
| bsuvc wrote:
| 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.
| yuliyp wrote:
| 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.
| axpy wrote:
| 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.
| mesarvagya wrote:
| This. If it is a production issue that needs to be fixed, fix
| it fast with priority and create another ticket to take care
| of reviewers comments.
| pif wrote:
| > 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.
| bsuvc wrote:
| 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.
| idlewords wrote:
| Sounds like Philip should have laid off a few people in IT
| management.
| omoikane wrote:
| 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.
| cjalmeida wrote:
| And it just cost 6 x 10% x $MM worth of daily products! /s
| ebiester wrote:
| 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.)
| bsuvc wrote:
| That's a good point.
|
| 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.
| crop_rotation wrote:
| 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.
| ryukoposting wrote:
| 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.
| NavinF wrote:
| > Nearly all of the "bugs" found by the static analyzer won't
| actually be bugs
|
| Which static analyzer is this? Every tool I've used only
| finds bugs the are provable so the false positive rate is
| essential zero
| kevin_thibedeau wrote:
| 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.
| qudat wrote:
| Agreed. We don't need human linters: https://bower.sh/human-
| linting
| swader999 wrote:
| If you can't change the Company, change your company.
| BSEdlMMldESB wrote:
| 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.
| swader999 wrote:
| Good catch. I rephrased it more to what I intended. (Parent
| replied to my initial comment: "If you can't change your
| Company, change your Company.")
| mikestew wrote:
| If I am catching your meaning correctly, I would also
| lower-case the second use of "company" to drive home a
| different usage of the word.
|
| I nitpick only because it's a clever turn of a word that
| deserves understanding.
| swader999 wrote:
| Fixed!
| cratermoon wrote:
| Did we just witness a nit-picky "code" review, right here
| in the comments? I think we did.
| depressedpanda wrote:
| It wasn't nit picky as I wouldn't have understood what
| the author meant before the reviewers came in and
| suggested improvements.
| cratermoon wrote:
| 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.
| avidiax wrote:
| > 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.
| crop_rotation wrote:
| 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.
| Macha wrote:
| 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.
| delusional wrote:
| 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.
| ryanschneider wrote:
| 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!
| mst wrote:
| 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.
| sibit wrote:
| We use the following:
|
| Nit: This is a minor thing. Technically you should do it,
| but it won't hugely impact things.
|
| Optional: I think this may be a good idea, but it's not
| strictly required.
|
| FYI: I don't expect you to do this in this PR, but you
| may find this interesting to think about for the future.
|
| None of these block a merge and anything beyond these
| type of comments will be a in-person discussion.
| Macha wrote:
| > 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.
|
| This is all well and good until your company brings in
| automation to enforce the setting that all changes
| dismiss reviews on all repositories.
|
| Combine it with the requirement that all branches must be
| up to date with master before merging for the double
| whammy of rebase/review/repeat hell.
| Tainnor wrote:
| 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.
| arp242 wrote:
| > 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".
| jacquesm wrote:
| 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.
| gtirloni wrote:
| I will often follow the codebase's way of doing things,
| even if I know a better way.
|
| I'll only push for the better way if I, or someone else, is
| willing to refactor the whole codebase.
|
| Last thing I want is to work on a codebase with N different
| ways to solve the same problem. That decreases productivity
| too much.
| jacquesm wrote:
| 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.
| Pannoniae wrote:
| 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.
| flippinburgers wrote:
| 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.
| renegade-otter wrote:
| 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.
| a_cardboard_box wrote:
| 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).
| delusional wrote:
| 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.
| bobthepanda wrote:
| 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.
| arp242 wrote:
| At least when it's a person it's someone you can talk to,
| reason with, compromise with, reasonably disagree with.
| Can't do any of that with software.
| kilotaras wrote:
| Most tools have an escape hatch, see e.g. [0]
|
| [0] https://github.com/search?q=clang-
| format+off&type=code
| pjmlp wrote:
| Example, clean code architecture astronauts.
| ddejohn wrote:
| Can you elaborate?
| pjmlp wrote:
| Some examples,
|
| https://serverlessfirst.com/emails/the-serverless-
| architectu...
|
| https://stackoverflow.com/questions/404210/design-
| patterns-a...
|
| https://www.google.com/amp/s/blog.codinghorror.com/it-
| came-f...
| ddejohn wrote:
| 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"?
| pjmlp wrote:
| In that case,
|
| https://qntm.org/clean
|
| Usually many architects tend to go overboard with
| interfaces for everything, repositories, data layers,
| hexagonal architecture, and whatever else is fashionable
| at conferences.
| RHSeeger wrote:
| 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.
| cratermoon wrote:
| 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?
| dharmab wrote:
| I use these tools but make sure to turn off any subjective
| checks in their configurations.
| ddejohn wrote:
| 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.
| YokoZar wrote:
| 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.
| Pannoniae wrote:
| 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.
| sanderjd wrote:
| 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.
| sodapopcan wrote:
| ...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.
| Pannoniae wrote:
| 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....
| sodapopcan wrote:
| 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!
| chefandy wrote:
| Workaday code should be documented so another developer
| can skim it in _n_ minutes rather than spend _n_ 3 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.
| sodapopcan wrote:
| I really like the term "workday code". I always struggle
| for a concise term to use for this and now I have one!
| Thanks :)
|
| Also, agreed on documentation though I didn't feel like
| diving into that one in my response.
| jiveturkey wrote:
| you misread workaday
| sodapopcan wrote:
| I don't think I did but I wouldn't know since you
| neglected to explain how.
|
| EDIT: Ohhhh I see now :)
| ddejohn wrote:
| I googled it but didn't really see anything obvious --
| what is "workaday code"?
| gtirloni wrote:
| Not your side project.
| chefandy wrote:
| 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.
| jameshart wrote:
| 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.
| hoosieree wrote:
| Before code review: // 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
|
| Code is more uniform, zero downsides!
| 20after4 wrote:
| TL;DR: PEP8 can go to hell.
|
| 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.
| ddejohn wrote:
| 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
| mst wrote:
| > 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.
| a1o wrote:
| Make sure to disable thing in flake like E721 or things
| that are simply incorrect for your project, don't trust it
| blindly.
| arp242 wrote:
| "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.
| lukevp wrote:
| 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).
| arp242 wrote:
| 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.
| jaktet wrote:
| 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.
| arp242 wrote:
| > 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.
| sanderjd wrote:
| 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.
| morelisp wrote:
| 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.
| TillE wrote:
| Yeah I'm always delighted to use clang-format rather than
| worry about where I should put the brackets.
|
| Code style is the epitome of bikeshedding. Anything
| consistent is fine, and using a tool to automate that is
| a huge productivity win.
| web3-is-a-scam wrote:
| > 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.
| gjvc wrote:
| we have wider screens nowadays.
| troupo wrote:
| And I prefer to have three-four columns of code in those
| wider screens than obnoxiously long lines.
| mst wrote:
| 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.
| sanderjd wrote:
| Disagree. It's better to have an automatic tool do stuff
| like this so that humans just never think about trivia
| like line length at all.
| [deleted]
| natded wrote:
| [dead]
| Philip-J-Fry wrote:
| 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.
| nickjj wrote:
| > 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.
| Macha wrote:
| On this example: 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.
| Tainnor wrote:
| 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.
| RadiozRadioz wrote:
| 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.
| morelisp wrote:
| Check out this guy who thinks it's objectively bad to
| have 1000 line methods.
| cratermoon wrote:
| > 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.
| nicwolff wrote:
| 11 ways of thinking! if not something:
| # Unhappy case return # Happy case
| Izkata wrote:
| This one specifically has a name, that if statement
| (including the early return) is a guard clause.
| nickjj wrote:
| Yep, personally I think that's ok too.
|
| 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.
| GrinningFool wrote:
| 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.
| MrJohz wrote:
| 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.
| jt2190 wrote:
| > 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.
| MrJohz wrote:
| > > 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.
| jt2190 wrote:
| 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.
| MrJohz wrote:
| 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.
| detourdog wrote:
| 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.
| MrJohz wrote:
| 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.
| ryanschneider wrote:
| 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.
| [deleted]
| AdrianB1 wrote:
| 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.
| Waterluvian wrote:
| 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.
| mtsr wrote:
| 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.
| Waterluvian wrote:
| Yeah! Any one opinion may not be the best for any one
| person, but everyone being consistent is the best for
| everyone.
| hereonout2 wrote:
| Introducing linting in the test stage (flake8 with python)
| solves so many problems.
|
| By completely removing any personal preferences and
| disagreements on style, the whole team just gets to hate on
| and berate an inanimate tool instead.
|
| We all just seem to equally dislike flake8 and move on with
| our lives.
| pjmlp wrote:
| 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.
| RHSeeger wrote:
| 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.
| xwdv wrote:
| [flagged]
| PartiallyTyped wrote:
| 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".
| Tainnor wrote:
| 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.
| ReaLNero wrote:
| > 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).
| Tainnor wrote:
| 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.
| Forricide wrote:
| 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.
| lrem wrote:
| 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?
| Forricide wrote:
| 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.
| javajosh wrote:
| 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.
| crop_rotation wrote:
| 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.
| javajosh wrote:
| _> Of course it needs to be substantive_
| morelisp wrote:
| 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.
| ghqst wrote:
| 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.
| jpollock wrote:
| 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?
| regularfry wrote:
| Possibly (6), depending on the nature of the duplication.
| okibry wrote:
| I can relate that.
| swader999 wrote:
| This is a O(1) operation at this company. Did I pass the
| interview? I still don't want to work there.
| hkon wrote:
| 6 days. Hey, that could be considered fast most places!
| karmakaze wrote:
| 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.
| add-sub-mul-div wrote:
| > 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.
| karmakaze wrote:
| 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.
| morelisp wrote:
| 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.)
| don-code wrote:
| 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.
| throwaway2990 wrote:
| Sounds like a horrible place to work.
| icegreentea2 wrote:
| 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?
| jstanley wrote:
| Because the CTO is not the product manager's boss.
| icegreentea2 wrote:
| 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.
| zerr wrote:
| 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.
| anthomtb wrote:
| 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.
| jefozabuss wrote:
| sounds like a security and audit nightmare to be honest but I
| guess it's an agency or similar with smaller projects
| swader999 wrote:
| 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.
| jacknews wrote:
| Or in other words, artificial/arbitrary management
| metrics/deadlines force through a possibly dangerous change to
| factory operations?
| raverbashing wrote:
| 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)
| [deleted]
| jacknews wrote:
| A CEO is _entirely_ responsible for the culture of an
| organization.
| yosefk wrote:
| 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.
| jacknews wrote:
| That would be the (non-existent) tests that should perhaps be
| run because "This affects everything in the factory."
|
| Probably the code still works with this change, but does the
| factory?
| bluedino wrote:
| A modern version would involve three cloud teams, DevSecInfraOps,
| and discussions about which resource groups something is in.
|
| Also there were 1,280 man-hours of meetings involved here
| [deleted]
| jasmer wrote:
| 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.
| [deleted]
| politelemon wrote:
| > Julie: Then contact Joe in IT Security. He'll get you
| permissions. > 2 hours later.
|
| This is completely unrealistic, security teams would never
| respond so quickly.
| fargle wrote:
| our security team actually responds even faster. they simply
| deny all requests automatically, but immediately.
| aezart wrote:
| It takes multiple weeks to get someone added to the AD group
| required for wiki edit permissions.
| seanthemon wrote:
| 2 programmer hours, obviously
| 542458 wrote:
| 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.
| switch007 wrote:
| Unless it's them getting a P1 security alert because you ran
| "npm install"
| [deleted]
| matt3210 wrote:
| 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.
| richk449 wrote:
| 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?
| LispSporks22 wrote:
| Wow that is Discovery's engineering teams. The whole mess of it
| is called "Engineering Excellence". I hated it.
| nojvek wrote:
| 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
| mabbo wrote:
| 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.
| foogazi wrote:
| > 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
| Paul-Craft wrote:
| > 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.
| figassis wrote:
| 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.
| Tainnor wrote:
| 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.
| hinkley wrote:
| 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.
| Jabbles wrote:
| 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.
| thiht wrote:
| 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.
| azhenley wrote:
| This bureaucracy grows proportional to company size. It gets
| much, much worse.
| activitypea wrote:
| 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.
| ravila4 wrote:
| 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.
| habosa wrote:
| 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.
| a13o wrote:
| 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.
| steveBK123 wrote:
| 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.
| JoeAltmaier wrote:
| 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.
| qudat wrote:
| I abhor "every code change needs a reviewer" rule. They are a
| massive distraction and don't necessarily lead to better code.
| willseth wrote:
| 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.
| Tainnor wrote:
| 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.
| black_puppydog wrote:
| 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.
___________________________________________________________________
(page generated 2023-07-16 23:01 UTC)