11 ms·

This is the bane of my existence at the moment after ~20 years into my career, and it frustrates me when I run into these situations when trying to get certain

This is the bane of my existence at the moment after ~20 years into my career, and it frustrates me when I run into these situations when trying to get certain people to review pull requests (because I'm being kind, and adhering to a process, and there is really valuable feedback at times). But on the whole it's like being dragged back down to working at a snails pace. - Can't refactor code because it changes too many files and too many lines. - Can't commit large chunks of well tested code that 'Does feature X', because... too many files and too many lines. - Have to split everything down into a long sequence of consecutive pull requests that become a process nightmare in its own right - The documentation comments gets nitpicked to death with mostly useless comments about not having periods at the ends of lines - End up having to explain every little detail throughout the function as if I'm trying to produce a lecture, things like `/* loop until not valid */ while (!valid) {...` seemed to be what they wanted, but to me it made no sense what so ever to even have that comment This can turn a ~50 line function into a 3 day process, a couple of hundred lines into a multi-week process, and a thousand or two line refactor (while retaining full test coverage) into a multi-month process. At one point I just downed tools and quit the company, the absurdity of it all completely drained my motivation, killed progress & flow and lead to features not being shipped. Meanwhile with projects I'm managing I have a fairly good handle on 'ok this code isnt the best, but it does work, it is fairly well tested, and it will be shipped as the beta', so as to not be obstinate.

NotBoolean 2y ago
I don’t have your experience but I personally think some of this feedback can be warranted. > Can't refactor code because it changes too many files and too many lines. This really depends on the change. If you are just doing a mass rename like updating a function signature, fair enough but if you changing a lot of code it’s very hard to review it. Lots of cognitive load on the reviewer who might not have the same understanding of codebase as you. > Can't commit large chunks of well tested code that 'Does feature X', because... too many files and too many lines. Same as the above, reviewing is hard and more code means people get lazy and bored. Just because the code is tested doesn’t mean it’s correct, just means it passes tests. > Have to split everything down into a long sequence of consecutive pull requests that become a process nightmare in its own right This is planning issue, if you correctly size tickets you aren’t going to end up in messy situations as often. > The documentation comments gets nitpicked to death with mostly useless comments about not having periods at the ends of lines Having correctly written documentation is important. It can live a long time and if you don’t keep an eye on it can becomes a mess. Ideally you should review it before you submitting it to avoid these issues. > End up having to explain every little detail throughout the function as if I'm trying to produce a lecture, things like `/* loop until not valid */ while (!valid) {...` seemed to be what they wanted, but to me it made no sense what so ever to even have that comment I definitely agree with this one. Superfluous comments are a waste of time. Obviously this is just my option and you can take things too far but I do think that making code reviewable (by making it small) goes a long way. No one wants to review 1000s lines of code at once. It’s too much to process and people will do a worse job. Happy to hear your thoughts.
- quesomaster9000 2y ago
  I do object to the notion of something being a planning issue when you're talking about a days worth of work. Implement X, needs Y and Z, ok that was straightforward, also discovered U and V on the way and sorted that out, here's a pull request that neatly wraps it up. Which subsequently gets turned into a multi-week process, going back & forth almost every day, meaning I can't move on to the next thing, meanwhile I'm looking at the cumulative hourly wages of everybody involved and the cost is... shocking. Death by process IHMO.
  - bspammer 2y ago
    > Implement X, needs Y and Z, ok that was straightforward, also discovered U and V on the way and sorted that out, here's a pull request that neatly wraps it up This sounds very difficult to review to be honest. At a minimum unrelated changes should be in their own pull request (U and V in your example).
    - pbh101 2y ago
      Agree. Another item here that is contextual: what is the cost of a bug? Does it cost millions, do we find that out immediately, or does it take months? Or does it not really matter, and when we’ll find the big it will be cheap? The OP joining a new company might not have the context that existing employees have about why we’re being cautious/clear about what we’re changing as opposed to smuggling in refactors in the same PR as a feature change. I’m going to be the guy that is asking for a refactor to be in a separate commit/PR from the feature and clearly marked. It doesn’t justify everything else he mentioned (especially the comments piece) but once you get used to this it doesn’t need to extend timelines.
    - shakna 2y ago
      How do you rework a core process, then? If you rework a major unit that touches just about everything... Sharding something like that can break the actual improvement it is trying to deliver. Like... Increase the performance of a central VM. You'll touch every part of the code, but probably also build a new compiler analysis system. The system is seperate to existing code, but useless without the core changes. Seperating the two can ruin the optimisation meant to be delivered, because the context is no longer front and center. Allowing more quibling to degrade the changes.
nosefurhairdo 2y ago
That's rough. Of course some amount of thoughtfulness towards "smallest reasonable change" is valuable, but if you're not shipping then something is wrong. As for the "comments on every detail" thing... I would fight that until I win or have to leave. What a completely asinine practice to leave comments on typical lines of code.
SeptiumMMX 2y ago
You always need to look at the track record of the team. If they were not producing solid consistent results before you joined them, it's a very good indicator that something's fishy. All that "they are working on something else that we can't tell you" is BS. If they were, and you were the only one treated like that, hiring you was a decision forced upon the team, so they got rid of you in a rather efficient way.
lifeisstillgood 2y ago
I am trying my best to build in an inordinate amount of upfront linting and automated checks just to avoid such things - and then I still need to do a roadshow, or lots of explanations- but that’s probably good. But the good idea is to say “we all have the same brutal linting standards (including full stops in docs!) - so hopefully the human linger will actually start reading the code for what it is, not what it says”
- whstl 2y ago
  I'm also a fan of linting everything. Custom linter rules ftw. This and documenting non-lintable standards so that people are on the same page ("we do controllers like this"). This is how I like to build and run my teams. This makes juniors so much more confident because they can ship stuff from the get go without going through a lengthy nitpicky brutal review process. And more senior devs need to actually look at code and business rules rather than nitpicking silly shit.
  - t-writescode 2y ago
    > This makes juniors so much more confident because they can ship stuff from the get go without going through a lengthy nitpicky brutal review process. I had not considered that linters could greatly help new developers in this way, especially if you make it a one-button linting process for all established development environments. Thanks for the insight! I will use this for the future.
- justatdotin 2y ago
  if a colleague wants to argue over placement of a curly boy, I'll fight to the death. if it's a linter, I shrug and move on.
shinycode 2y ago
No wonder why software development used to be expensive if 50 lines of code takes multiples days for several people …
- LtWorf 2y ago
  Well maybe they do critical systems.
  - DavidPiper 2y ago
    Narrator: "They don't." (Glib, but in my experience, mostly true.)
  - shinycode 2y ago
    Valid point, it’s even mandatory in this case. Sometimes people do it for the sake of it. Maybe because there nothing else to make them feel important ? In critical systems I hope it’s the case though
spion 2y ago
Indeed, cognitive load is not the only thing that matters. Non-cognitive toil is also a problem and often enough it doesn't get sufficient attention even when things get really bad. We do need better code review tools though. We also need to approach that process as a mechanism of effectively building good shared understanding about the (new) code, not just "code review".
spockz 2y ago
This sounds more like a case where you need a “break-the-glass” like procedure where some checks don’t apply. Or the checks should be non blocking anyway.
notShabu 2y ago
there is huge incentive for people who don't know how to code/create/do-stuff to slow things down like this b/c it allows them many years of runway at the company. they are almost always cloaked in virtue signals. almost every established company you join will already have had this process going for a long time. doing stuff successfully at such a company is dangerous to the hierarchy and incurs an immune response to shut down or ostracize the doing-of-stuff successfully so the only way to survive or climb is to do stuff unsuccessfully (so they look good)
epolanski 2y ago
You seem to be describing a company where bureaucracy is a feature not a bug. Been there. Left, live thousands times better.
flakes 2y ago
> The documentation comments gets nitpicked to death with mostly useless comments about not having periods at the ends of lines > End up having to explain every little detail throughout the function For these cases I like to use the ‘suggest an edit’ feature on gitlab/github. Can have the change queued up in the comments and batch commit together, and takes almost no additional time/effort for the author. I typically add these suggestion comments and give an approve at the same time for small nitpicks, so no slow down in the PR process.
- F-W-M 2y ago
  I good process would be to just push the proposal to the branch in review.
  - flakes 2y ago
    I still want to let the author have the final say on if they decide to accept or reject the change, or modify it further. Editing the branch directly might cause some rebasing/merge conflicts if they’re addressing other peoples comments too, so I don't typically edit their working branch directly unless they ask me to.
MarkMarine 2y ago
I’m 15 years in and I feel basically the same. I end up making a feature or change, then going back and trying to split it into chunks that are digestible to my colleagues. I’ve got thousands of lines of staged changes that I’m waiting to drip out to people at a digestible pace. I yearn for the early stage startup where every commit is a big change and my colleagues are used to reviewing this, and I can execute at my actual pace. It’s really changed the way I think about software in general, I’ve come around to Rich Hickey’s radically simple language Clojure, because types bloat the refactors I’m doing. I’d love to have more of you where I work, is there some way I can see your work and send some job descriptions and see if you’re interested?
- withinboredom 2y ago
  > I end up making a feature or change, then going back and trying to split it into chunks that are digestible to my colleagues. If you are doing this AFTER you've written the code, it is probably way easier to do it as you go. It's one thing if you have no idea what the code will look like from the beginning -- just go ahead and open the big PR and EXPLAIN WHY. I know that I'm more than happy to review a big PR if I understand why it has to be big. I will be annoyed if I see a PR that is a mix of refactoring, bug fixes, and new features. You can (and should) have done those all as separate PRs (and tickets). If you need to refactor something, refactor it, and open a PR. It doesn't take that long and there's no need to wait until your huge PR is ready.
  - quesomaster9000 2y ago
    Solving creative problems is often iterative, and one things I'm very concerned about when doing engineering management is maintaining momentum and flow. Looking at latency hierarchies is a really good example, you have registers, then cache, then memory, SSD, network etc. and consulting with another human asynchronously is like sending a message to Jupiter (in the best case). So, with an iterative process, the more times you introduce (at best) hour long delays, you end up sitting on your arse twiddling your thumbs doing nothing, until the response comes back. The concept of making PRs as you go fails to capture one of the aspects of low-latency problem solving, which is that you catch a problem, you correct it and you revise it locally, without exiting that loop. Which is problematic because not only have you put yourself in a situation where you're waiting for a response, but you've stopped half-way through an unfinished idea. This comes back to 'is it done', a gut feel that it's an appropriate time to break the loop and incur the latency cost, which for every developer will be different and is something that I have grown to deeply trust and and adjust to for everybody I work with. What I'm getting at is the iterative problem solving process often can't be neatly dissected into discrete units while it's happening, and after we've reached the 'doneness' point it takes much more work to undo part of your work and re-do it than it took to do originally, so not only do you have the async overhead of every interaction, but you have the cognitive burden of untangling what was previously a cohesive unit of thought - which again is another big time killer
jschrf 2y ago
Aye. Sign of the times. You're 20+ years in, so I'm preaching to the choir and old-man-yelling-at-cloud here. Cargo culting + AI are the culprits. Sucks to say, but engineering is going downhill fast. First wave of the shitularity. Architects? Naw, prompt engineers. Barf. Why write good code when a glorified chatbot could do it shittier and faster? Sign of our times. Cardboard cutout code rather than stonemasonry. Shrinkflation of thought. Peep this purified downvote fuel: Everything is bad because everyone is lazy and cargo cults. Web specifically. Full-stop. AI sucks at coding and is making things recursively worse in the long run. LLMs are nothing more than recursive echo chambers of copypasta code that doesn't keep up with API flux. A great example of this is the original PHP docs, which so, so many of us copypasta'd from, leading to an untold amount of SQL injections. Oopsies. Simalarily and hunting for downvotes, React is a templating framework that is useful but does not even meet its original value proposition, which is state management in UI. Hilariously tragic. See: original example of message desync state issue on FB. Unsolved for years by the purported solution. The NoSQL flash is another tragic comedy. Rebuilding the wheel when there is a faster, better wheel already carefully made. Postgres with JSONB. GraphQL is another example of Stuff We Don't Need But Use Because People Say It's Good. Devs: you don't need it. Just write a query. - You mention a hugely important KPI in code. How many files, tools, commands, etc must I touch to do the simplest thing? Did something take me a day when it should have taken 30s? This is rife today, we should all pay attention. Pad left. Look no further than hooks and contexts in React land for an example. Flawed to begin with, simply because "class is a yucky keyword". I keep seeing this in "fast moving" startups: the diaspora of business logic spread through a codebase, when simplicity and unity is key, which you touch on. Absolute waste of electricity and runway, all thanks to opiniation. Burnt runways abound. Sometimes I can't help but think engineering needs a turn it off and then on again moment in safe mode without fads and chatbots.
- sgarland 2y ago
  > Everything is bad because everyone is lazy and cargo cults. It’s an interesting series of events that led to this (personal theory). Brilliant people who deeply understood fundamentals built abstractions because they were lazy, in a good way. Some people adopted those abstractions without fully comprehending what was being hidden, and some of those people built additional abstractions. Eventually, you wind up with people building solutions to problems which wouldn’t exist if, generations above, the original problem had been better understood.
gre 2y ago
The process is introducing more room for bugs to somehow creep in. Damn.
- quesomaster9000 2y ago
  This is a big problem with reviews where the author is capitulating because they, with gritted teeth, acknowledge it's the only way to get the desired result (jumping over a hurdle). So you blindly accept an ill-informed suggestion because that's the only way you can complete the process.
jesse__ 2y ago
I've had a similar experience several times over the years. Even at companies with no working product that ostensibly wanted to 'move fast and break things'. And I do the same thing; quit and move on. I'm pretty convinced people like that more-or-less can't be reasoned with. My question is .. is this getting more common as time goes on, or do I just feel like it is..
sarchertech 2y ago
After 20 years of doing this, I’m convinced that required PR reviews aren’t worth the cost. In the thousands of pull requests I’ve merged across many companies, I have never once had a reviewer catch a major bug (a bug that is severe enough that if discovered after hours, would require an oncall engineer to push a hot fix rather than wait for the normal deployment process to fix it). I’ve pushed a few major bugs to production, but I’ve never had a PR reviewer catch one. I’ve had reviewers make excellent suggestions, but it’s almost never anything that really matters. Certainly not worth all the time I’ve spent on the process. That being said, I’m certainly not against collaboration, but I think required PR reviews aren’t the way to do it.
- kevmo314 2y ago
  Wow someone who finally has this same unpopular opinion as I do. I'm a huge fan of review-optional PRs. Let it be up to the author to make that call and if it were really important to enforce it would be more foolproof to do so with automation. Unfortunately every time I've proposed this it's received like it's sacrilegious but nobody could tell me why PR reviews are really necessary to be required. The most ironic part is that I once caught a production-breaking bug in a PR while at FAANG and the author pushed back. Ultimately I decided it wasn't worth the argument and just let it go through. Unsurprisingly, it broke production but we fixed it very quickly after we were all finally aligned that it was actually a problem.
  - sarchertech 2y ago
    >Unfortunately every time I've proposed this it's received like it's sacrilegious but nobody could tell me why PR reviews are really necessary to be required. Obvious signs of cargoculting in my opinion.
    - deleted 2y ago
      [deleted]
  - mattmanser 2y ago
    I'll bite. To catch stupid mistakes like an extra file, an accidental debug flag, a missing compiler hint that has to be added to migration scripts etc. To ensure someone who doesn't quite understand the difference between dev and production build pipelines doesn't break it. To ensure a certain direction is being followed when numerous contractors are working on the code. For example a vague consistency in API designs, API param names, ordering, etc. To check obvious misunderstandings by juniors and new hires. To nix architect astronauts before their 'elegant' solution for saving a string to a database in 500 lines gets added. To check the code is actually trying to solve the ticket instead of a wrong interpretation of the ticket. To get introduced to parts of the codebase you haven't worked on much. But as with anything you get from it what you put in.
charlie0 2y ago
I'm one of the rare individuals who really tries to review code and leave helpful comments. I've been on the receiving end of really big PRs and can say I understand why you're being told to break things up into smaller chunks. Most of the devs who submit large PRs just don't have a good grasp of organizing things well enough. I've seen this over and over again and it's due to not spending enough time planning out a feature. There will be exceptions to this, but when devs keep doing it over and over, it's the reviewer's job to reject it and send it back with helpful feedback. I also understand most people don't like the friction this can create and so you end you with 80% of PRs being rubber stamped and bugs getting into production because the reviewers just give up on trying to make people better devs.
- sneak 2y ago
  The reviewer's job is primarily to ensure business continuity, and only marginally to make people better devs.
  - fmbb 2y ago
    When I review code I never think I am there to make people better devs. I’m reviewing the code because I don’t want shit code merged into the code base I am responsible for operating. I’m going to be the one debugging that. Don’t just merge shit you feel like merging.
romellem 2y ago
> mostly useless comments about not having periods at the ends of lines Oh my god, this sounds like a nightmare. I definitely would not be able to tolerate this for long. Did you try to get them to change? Were you just not in a senior enough position for anyone to listen?
tdiff 2y ago
As a reviewer I've seen numerous examples of PRs that were basically out of sync with the rest of the project, did not solve the problem they were supposed to solve, or added buggy or unmaintainable code. Arguments like "but it works in majority of cases" are a way to delegate fixing issues to somebody else later. Unless noone will be using that code at all, in which case it should not be merged either.

NotBoolean 2y ago

I don’t have your experience but I personally think some of this feedback can be warranted. > Can't refactor code because it changes too many files and too many lines. This really depends on the change. If you are just doing a mass rename like updating a function signature, fair enough but if you changing a lot of code it’s very hard to review it. Lots of cognitive load on the reviewer who might not have the same understanding of codebase as you. > Can't commit large chunks of well tested code that 'Does feature X', because... too many files and too many lines. Same as the above, reviewing is hard and more code means people get lazy and bored. Just because the code is tested doesn’t mean it’s correct, just means it passes tests. > Have to split everything down into a long sequence of consecutive pull requests that become a process nightmare in its own right This is planning issue, if you correctly size tickets you aren’t going to end up in messy situations as often. > The documentation comments gets nitpicked to death with mostly useless comments about not having periods at the ends of lines Having correctly written documentation is important. It can live a long time and if you don’t keep an eye on it can becomes a mess. Ideally you should review it before you submitting it to avoid these issues. > End up having to explain every little detail throughout the function as if I'm trying to produce a lecture, things like `/* loop until not valid */ while (!valid) {...` seemed to be what they wanted, but to me it made no sense what so ever to even have that comment I definitely agree with this one. Superfluous comments are a waste of time. Obviously this is just my option and you can take things too far but I do think that making code reviewable (by making it small) goes a long way. No one wants to review 1000s lines of code at once. It’s too much to process and people will do a worse job. Happy to hear your thoughts.

quesomaster9000 2y ago
I do object to the notion of something being a planning issue when you're talking about a days worth of work. Implement X, needs Y and Z, ok that was straightforward, also discovered U and V on the way and sorted that out, here's a pull request that neatly wraps it up. Which subsequently gets turned into a multi-week process, going back & forth almost every day, meaning I can't move on to the next thing, meanwhile I'm looking at the cumulative hourly wages of everybody involved and the cost is... shocking. Death by process IHMO.
- bspammer 2y ago
  > Implement X, needs Y and Z, ok that was straightforward, also discovered U and V on the way and sorted that out, here's a pull request that neatly wraps it up This sounds very difficult to review to be honest. At a minimum unrelated changes should be in their own pull request (U and V in your example).
  - pbh101 2y ago
    Agree. Another item here that is contextual: what is the cost of a bug? Does it cost millions, do we find that out immediately, or does it take months? Or does it not really matter, and when we’ll find the big it will be cheap? The OP joining a new company might not have the context that existing employees have about why we’re being cautious/clear about what we’re changing as opposed to smuggling in refactors in the same PR as a feature change. I’m going to be the guy that is asking for a refactor to be in a separate commit/PR from the feature and clearly marked. It doesn’t justify everything else he mentioned (especially the comments piece) but once you get used to this it doesn’t need to extend timelines.
  - shakna 2y ago
    How do you rework a core process, then? If you rework a major unit that touches just about everything... Sharding something like that can break the actual improvement it is trying to deliver. Like... Increase the performance of a central VM. You'll touch every part of the code, but probably also build a new compiler analysis system. The system is seperate to existing code, but useless without the core changes. Seperating the two can ruin the optimisation meant to be delivered, because the context is no longer front and center. Allowing more quibling to degrade the changes.

nosefurhairdo 2y ago

That's rough. Of course some amount of thoughtfulness towards "smallest reasonable change" is valuable, but if you're not shipping then something is wrong. As for the "comments on every detail" thing... I would fight that until I win or have to leave. What a completely asinine practice to leave comments on typical lines of code.

SeptiumMMX 2y ago

You always need to look at the track record of the team. If they were not producing solid consistent results before you joined them, it's a very good indicator that something's fishy. All that "they are working on something else that we can't tell you" is BS. If they were, and you were the only one treated like that, hiring you was a decision forced upon the team, so they got rid of you in a rather efficient way.

lifeisstillgood 2y ago

I am trying my best to build in an inordinate amount of upfront linting and automated checks just to avoid such things - and then I still need to do a roadshow, or lots of explanations- but that’s probably good. But the good idea is to say “we all have the same brutal linting standards (including full stops in docs!) - so hopefully the human linger will actually start reading the code for what it is, not what it says”

whstl 2y ago
I'm also a fan of linting everything. Custom linter rules ftw. This and documenting non-lintable standards so that people are on the same page ("we do controllers like this"). This is how I like to build and run my teams. This makes juniors so much more confident because they can ship stuff from the get go without going through a lengthy nitpicky brutal review process. And more senior devs need to actually look at code and business rules rather than nitpicking silly shit.
- t-writescode 2y ago
  > This makes juniors so much more confident because they can ship stuff from the get go without going through a lengthy nitpicky brutal review process. I had not considered that linters could greatly help new developers in this way, especially if you make it a one-button linting process for all established development environments. Thanks for the insight! I will use this for the future.
justatdotin 2y ago
if a colleague wants to argue over placement of a curly boy, I'll fight to the death. if it's a linter, I shrug and move on.

shinycode 2y ago

No wonder why software development used to be expensive if 50 lines of code takes multiples days for several people …

LtWorf 2y ago
Well maybe they do critical systems.
- DavidPiper 2y ago
  Narrator: "They don't." (Glib, but in my experience, mostly true.)
- shinycode 2y ago
  Valid point, it’s even mandatory in this case. Sometimes people do it for the sake of it. Maybe because there nothing else to make them feel important ? In critical systems I hope it’s the case though

spion 2y ago

Indeed, cognitive load is not the only thing that matters. Non-cognitive toil is also a problem and often enough it doesn't get sufficient attention even when things get really bad. We do need better code review tools though. We also need to approach that process as a mechanism of effectively building good shared understanding about the (new) code, not just "code review".

spockz 2y ago

This sounds more like a case where you need a “break-the-glass” like procedure where some checks don’t apply. Or the checks should be non blocking anyway.

notShabu 2y ago

there is huge incentive for people who don't know how to code/create/do-stuff to slow things down like this b/c it allows them many years of runway at the company. they are almost always cloaked in virtue signals. almost every established company you join will already have had this process going for a long time. doing stuff successfully at such a company is dangerous to the hierarchy and incurs an immune response to shut down or ostracize the doing-of-stuff successfully so the only way to survive or climb is to do stuff unsuccessfully (so they look good)

epolanski 2y ago

You seem to be describing a company where bureaucracy is a feature not a bug. Been there. Left, live thousands times better.

flakes 2y ago

> The documentation comments gets nitpicked to death with mostly useless comments about not having periods at the ends of lines > End up having to explain every little detail throughout the function For these cases I like to use the ‘suggest an edit’ feature on gitlab/github. Can have the change queued up in the comments and batch commit together, and takes almost no additional time/effort for the author. I typically add these suggestion comments and give an approve at the same time for small nitpicks, so no slow down in the PR process.

F-W-M 2y ago
I good process would be to just push the proposal to the branch in review.
- flakes 2y ago
  I still want to let the author have the final say on if they decide to accept or reject the change, or modify it further. Editing the branch directly might cause some rebasing/merge conflicts if they’re addressing other peoples comments too, so I don't typically edit their working branch directly unless they ask me to.

MarkMarine 2y ago

I’m 15 years in and I feel basically the same. I end up making a feature or change, then going back and trying to split it into chunks that are digestible to my colleagues. I’ve got thousands of lines of staged changes that I’m waiting to drip out to people at a digestible pace. I yearn for the early stage startup where every commit is a big change and my colleagues are used to reviewing this, and I can execute at my actual pace. It’s really changed the way I think about software in general, I’ve come around to Rich Hickey’s radically simple language Clojure, because types bloat the refactors I’m doing. I’d love to have more of you where I work, is there some way I can see your work and send some job descriptions and see if you’re interested?

withinboredom 2y ago
> I end up making a feature or change, then going back and trying to split it into chunks that are digestible to my colleagues. If you are doing this AFTER you've written the code, it is probably way easier to do it as you go. It's one thing if you have no idea what the code will look like from the beginning -- just go ahead and open the big PR and EXPLAIN WHY. I know that I'm more than happy to review a big PR if I understand why it has to be big. I will be annoyed if I see a PR that is a mix of refactoring, bug fixes, and new features. You can (and should) have done those all as separate PRs (and tickets). If you need to refactor something, refactor it, and open a PR. It doesn't take that long and there's no need to wait until your huge PR is ready.
- quesomaster9000 2y ago
  Solving creative problems is often iterative, and one things I'm very concerned about when doing engineering management is maintaining momentum and flow. Looking at latency hierarchies is a really good example, you have registers, then cache, then memory, SSD, network etc. and consulting with another human asynchronously is like sending a message to Jupiter (in the best case). So, with an iterative process, the more times you introduce (at best) hour long delays, you end up sitting on your arse twiddling your thumbs doing nothing, until the response comes back. The concept of making PRs as you go fails to capture one of the aspects of low-latency problem solving, which is that you catch a problem, you correct it and you revise it locally, without exiting that loop. Which is problematic because not only have you put yourself in a situation where you're waiting for a response, but you've stopped half-way through an unfinished idea. This comes back to 'is it done', a gut feel that it's an appropriate time to break the loop and incur the latency cost, which for every developer will be different and is something that I have grown to deeply trust and and adjust to for everybody I work with. What I'm getting at is the iterative problem solving process often can't be neatly dissected into discrete units while it's happening, and after we've reached the 'doneness' point it takes much more work to undo part of your work and re-do it than it took to do originally, so not only do you have the async overhead of every interaction, but you have the cognitive burden of untangling what was previously a cohesive unit of thought - which again is another big time killer

jschrf 2y ago

Aye. Sign of the times. You're 20+ years in, so I'm preaching to the choir and old-man-yelling-at-cloud here. Cargo culting + AI are the culprits. Sucks to say, but engineering is going downhill fast. First wave of the shitularity. Architects? Naw, prompt engineers. Barf. Why write good code when a glorified chatbot could do it shittier and faster? Sign of our times. Cardboard cutout code rather than stonemasonry. Shrinkflation of thought. Peep this purified downvote fuel: Everything is bad because everyone is lazy and cargo cults. Web specifically. Full-stop. AI sucks at coding and is making things recursively worse in the long run. LLMs are nothing more than recursive echo chambers of copypasta code that doesn't keep up with API flux. A great example of this is the original PHP docs, which so, so many of us copypasta'd from, leading to an untold amount of SQL injections. Oopsies. Simalarily and hunting for downvotes, React is a templating framework that is useful but does not even meet its original value proposition, which is state management in UI. Hilariously tragic. See: original example of message desync state issue on FB. Unsolved for years by the purported solution. The NoSQL flash is another tragic comedy. Rebuilding the wheel when there is a faster, better wheel already carefully made. Postgres with JSONB. GraphQL is another example of Stuff We Don't Need But Use Because People Say It's Good. Devs: you don't need it. Just write a query. - You mention a hugely important KPI in code. How many files, tools, commands, etc must I touch to do the simplest thing? Did something take me a day when it should have taken 30s? This is rife today, we should all pay attention. Pad left. Look no further than hooks and contexts in React land for an example. Flawed to begin with, simply because "class is a yucky keyword". I keep seeing this in "fast moving" startups: the diaspora of business logic spread through a codebase, when simplicity and unity is key, which you touch on. Absolute waste of electricity and runway, all thanks to opiniation. Burnt runways abound. Sometimes I can't help but think engineering needs a turn it off and then on again moment in safe mode without fads and chatbots.

sgarland 2y ago
> Everything is bad because everyone is lazy and cargo cults. It’s an interesting series of events that led to this (personal theory). Brilliant people who deeply understood fundamentals built abstractions because they were lazy, in a good way. Some people adopted those abstractions without fully comprehending what was being hidden, and some of those people built additional abstractions. Eventually, you wind up with people building solutions to problems which wouldn’t exist if, generations above, the original problem had been better understood.

gre 2y ago

The process is introducing more room for bugs to somehow creep in. Damn.

quesomaster9000 2y ago
This is a big problem with reviews where the author is capitulating because they, with gritted teeth, acknowledge it's the only way to get the desired result (jumping over a hurdle). So you blindly accept an ill-informed suggestion because that's the only way you can complete the process.

jesse__ 2y ago

I've had a similar experience several times over the years. Even at companies with no working product that ostensibly wanted to 'move fast and break things'. And I do the same thing; quit and move on. I'm pretty convinced people like that more-or-less can't be reasoned with. My question is .. is this getting more common as time goes on, or do I just feel like it is..

sarchertech 2y ago

After 20 years of doing this, I’m convinced that required PR reviews aren’t worth the cost. In the thousands of pull requests I’ve merged across many companies, I have never once had a reviewer catch a major bug (a bug that is severe enough that if discovered after hours, would require an oncall engineer to push a hot fix rather than wait for the normal deployment process to fix it). I’ve pushed a few major bugs to production, but I’ve never had a PR reviewer catch one. I’ve had reviewers make excellent suggestions, but it’s almost never anything that really matters. Certainly not worth all the time I’ve spent on the process. That being said, I’m certainly not against collaboration, but I think required PR reviews aren’t the way to do it.

kevmo314 2y ago
Wow someone who finally has this same unpopular opinion as I do. I'm a huge fan of review-optional PRs. Let it be up to the author to make that call and if it were really important to enforce it would be more foolproof to do so with automation. Unfortunately every time I've proposed this it's received like it's sacrilegious but nobody could tell me why PR reviews are really necessary to be required. The most ironic part is that I once caught a production-breaking bug in a PR while at FAANG and the author pushed back. Ultimately I decided it wasn't worth the argument and just let it go through. Unsurprisingly, it broke production but we fixed it very quickly after we were all finally aligned that it was actually a problem.
- sarchertech 2y ago
  >Unfortunately every time I've proposed this it's received like it's sacrilegious but nobody could tell me why PR reviews are really necessary to be required. Obvious signs of cargoculting in my opinion.
  - deleted 2y ago
    [deleted]
- mattmanser 2y ago
  I'll bite. To catch stupid mistakes like an extra file, an accidental debug flag, a missing compiler hint that has to be added to migration scripts etc. To ensure someone who doesn't quite understand the difference between dev and production build pipelines doesn't break it. To ensure a certain direction is being followed when numerous contractors are working on the code. For example a vague consistency in API designs, API param names, ordering, etc. To check obvious misunderstandings by juniors and new hires. To nix architect astronauts before their 'elegant' solution for saving a string to a database in 500 lines gets added. To check the code is actually trying to solve the ticket instead of a wrong interpretation of the ticket. To get introduced to parts of the codebase you haven't worked on much. But as with anything you get from it what you put in.

charlie0 2y ago

I'm one of the rare individuals who really tries to review code and leave helpful comments. I've been on the receiving end of really big PRs and can say I understand why you're being told to break things up into smaller chunks. Most of the devs who submit large PRs just don't have a good grasp of organizing things well enough. I've seen this over and over again and it's due to not spending enough time planning out a feature. There will be exceptions to this, but when devs keep doing it over and over, it's the reviewer's job to reject it and send it back with helpful feedback. I also understand most people don't like the friction this can create and so you end you with 80% of PRs being rubber stamped and bugs getting into production because the reviewers just give up on trying to make people better devs.

sneak 2y ago
The reviewer's job is primarily to ensure business continuity, and only marginally to make people better devs.
- fmbb 2y ago
  When I review code I never think I am there to make people better devs. I’m reviewing the code because I don’t want shit code merged into the code base I am responsible for operating. I’m going to be the one debugging that. Don’t just merge shit you feel like merging.

romellem 2y ago

> mostly useless comments about not having periods at the ends of lines Oh my god, this sounds like a nightmare. I definitely would not be able to tolerate this for long. Did you try to get them to change? Were you just not in a senior enough position for anyone to listen?

tdiff 2y ago

As a reviewer I've seen numerous examples of PRs that were basically out of sync with the rest of the project, did not solve the problem they were supposed to solve, or added buggy or unmaintainable code. Arguments like "but it works in majority of cases" are a way to delegate fixing issues to somebody else later. Unless noone will be using that code at all, in which case it should not be merged either.