9 ms·

Agreed. I mainly manage and review code at this point in my career. I find many bugs, every once in a while finding something that would have caused an outage o

Agreed. I mainly manage and review code at this point in my career. I find many bugs, every once in a while finding something that would have caused an outage or notable problem for users. What I find more though is code that isn't thought through. Tech debt and code smell are real, and they affect the performance of a team. Nipping that in the bud takes quality PR reviews and time to meet with submitters around issues you find. Knock on wood but working at the company I do now where I, along with my team, have made quality PR reviews normal.. our codebase is now enjoyable and fun to work on. I highly recommend it! One key aspect is being “kind, not nice”. Be helpful when leaving comments in PRs, but don’t be nice for the sake of avoiding conflict. Also if you find code reviews to be a waste of time I can reccomend one thing I do often - give warnings. I approve and give comments around things I’d like to be fixed in the future for similar PRs. I don’t hold up the merge for little things, but at the same time I won’t let the little things slide forever

saulpw 2y ago
How do you phrase these warnings? "Next time.."? I have a hard time being serious with my own warnings if it's fine enough for now.
- audiodude 2y ago
  "This is okay for now, but we should think about how we want to serialize these objects. Feel free to remove the N^2 algorithm in a follow up."
  - est31 2y ago
    That works great in a setting where you are both employees of the same company, and you respect each other, but it often doesn't work in the open source world, people just disappear and you never hear from them again. It is possible that they do file follow-ups, but in my experience it's rare.
    - sfink 2y ago
      Yes, even within a company my threshold for accepting a change can vary pretty widely depending on my experience and relationship with the author. For an external contributor or someone I've never collaborated with (by reviewing code or having my code reviewed), I don't accept the code until almost everything is worked out to my satisfaction. With someone I work with regularly, it's not uncommon to accept a change with a comment like "this is all good, but you need to take X into account which will change almost everything in this patch" (I exaggerate, but only slightly). I know whether an update could be problematic and whether it is necessary to see it again. Sometimes there are a couple of obvious ways that something could be done, they picked one but weren't tied to it if I had a reason for picking a different one, I picked a different one for $REASON. Most are somewhere in between. Though in some ways it works the other way around. For an unfamiliar open source contributor, I need to be confident that the change is worthwhile. I will be lenient on stylistic things, and I'll just land their patch and then fix it up afterwards. For someone I've worked with a bunch (whether a familiar contributor or a coworker), I will trust their opinion on the underlying quality of a change, but be less tolerant of unnecessary stylistic differences since they should have already come into alignment on those and it's more likely to be an oversight if they missed something. (Plus, I don't want to be fixing up their changes after the fact, given that >90% of patches will come from regular contributors.)
    - seadan83 2y ago
      To add to this, "no time like now" is real. Follow-ups rarely happen, tracking them is a pain. Making someone accountable for it is unfair, particularly if they disagree with the follow up (and presumably they do, otherwise they would have done the thing in the first place).
- elcomet 2y ago
  We prefix our comments with "minor:", and all employees know that this means it's something that would be nice but not necessary to merge
- seadan83 2y ago
  Use the delete key. (My apologies for the snark, but I'm kinda serious, don't send those type of review comments at all). If it's fine for now, it's fine. If it's not fine - then speak up. If you do have such concerns, "fine for now" - that is something for an offline conversation - outside of the context of a CR. Which is also a good litmus test for how important a comment is. Would you schedule a 15 minute meeting to discuss the issue with your colleague? If not, then why is it okay to do that to someone during a CR?
jellyfishbeaver 2y ago
I am a new manager and I am struggling to get my team to understand the value in code reviews. I have been through so many rewrites and re-re-writes of spaghetti code, I am much more critical now reviewing code, and I am trying to promote this culture on my team. Do you have any suggestions? - The same people leave detailed comments on others' merge requests, but get discouraged when nobody else puts in the same amount of effort for theirs. - People blindly accept suggestions with no resistance or discussion to get the review over with. - People send their MRs to side channels or other teams to get their changes merged in without resistance or back and forth. (I've had to revert several of these).
- spankalee 2y ago
  Culture is made, it's not accidental. I would raise all these issues and more in group meetings. Try to get people to understand the many different benefits review brings to both the committer and reviewer - by having them state the benefits they want or could see getting. Talk about various kinds of comments (clear bugs, performance, style, robustness, factoring and organization, etc.), and the various priorities from no-action-required to nits to blockers. Talk about the priority of reviews themselves. Reducing the latency of reviews has a huge positive effect, ime.
  - seadan83 2y ago
    People are smart - how about listen to their concerns? Do that before hitting them over the head with a list of 'best practices' and 'desired outcomes'. I'm starting to come to the opinion that nits are entirely and outright counterproductive in code reviews. Since I learned about "Ship/Show/Ask" - it's drastically changed my view of code reviews: https://martinfowler.com/articles/ship-show-ask.html https://martinfowler.com/articles/ship-show-ask.html; I no longer believe reviewing every change is healthy, seeing the difference of other models I can tell it's actively harmful now. Another good approach IMO is allowing for post-merge reviews, 'ship/show/ask' is better, but why is it written in stone that a code review MUST happen before merge?
- ecshafer 2y ago
  Good PR culture is definitely something that has to be built from the ground up, and supported top down. At Shopify, who I think has a really good PR culture we have a few things that I think help (beyond a good CICD, and static analysis tools): 1. PRs are supposed to wait for 2 acceptances, can be shipped with 1, and can be emergency shipped with 0. So the barrier is low, but the culture supports more. We are expected to get 2 reviewers from our team to okay. 2. Depending on the code project, we have to fill out a template for the PR, what is in it, what it changes, what to look for when we test the code, etc. 3. Some areas have code owners that might require an additional review from a specific team. 4. We are expected to check out, and test branches when we review them. So a quick read and LGTM is really discouraged outside of a few small cases. I have seen a lot of places that do the blind PR acceptance, and its tough because without this really being enforced and encouraged that culture is hard to change.
closeparen 2y ago
>time to meet with submitters around issues you find. What! I would be livid if someone scheduled a meeting with me about a PR. We have way too many meetings already, this is one of the only processes that is mercifully async.
- hyperadvanced 2y ago
  It doesn’t need to be formal or very long. I personally enjoy a PR meeting where we can poke at the code and understand it over someone dumping 2,000 lines of code in my lap at lunch time and hoping to get their spaghetti to prod by dinner
- fiddlerwoaroof 2y ago
  Me too, I really dislike meetings that could have been handled by asking me three or four questions in slack or in the PR and then waiting fifteen minutes or so for me to answer.
- danielmarkbruce 2y ago
  Livid? About a meeting to discuss work? Some comments in slack etc sound worse than intended, and people are aware of that and sometimes go out of their way to say it in a way to it's received as intended.
- willio58 2y ago
  To be clear I make time to meet if the submitter wants to talk through things. I don’t require meeting on every PR. I meet as needed on PRs, pretty infrequently as people get up to speed
- seadan83 2y ago
  "in-person" review can be the best I find. If a CR is going to have a dozen comments about everything, it's a disaster to do that async - the author is going to feel attacked. The dialog to resolve the conflict & disagreements is probably not going to play out well. Personally I like it when teams start with in-person CRs if they are not used to them. Only after healthy relationships and conflict resolution mechanisms established, do the PRs go async. I also like a rule that there should never be more than 2 round trips on communications in a CR, otherwise it should be done at the same time. Delaying small changes to allow for such round-trip times is not feasible in a lot of environments. There's too much to be done. Not worth it. When projects do fail, it's very rarely that some manager slaps a big "failed" label on it. I bring that up to say that projects fail all the damn time because the team is moving too slow - and individuals often don't appreciate when they actually did need to move faster (the feedback loop is often missing, of, "oh, we actually were too slow, we needed to ship faster than we actually did. The JIRA, planning, estimation, CR, best practices, time spent fixing linting - it all doesn't matter cause this project actually failed - it took too long.")
mikepurvis 2y ago
"Tech debt and code smell are real" I think what I struggle most with is that often times there's a valid business reason to "just ship it ASAP", but the missing piece is the accountability around the conditions attached to that. Like, okay, if we don't want to fix this now because it needs to be in the next release then we can merge it as-is, but you can't document this externally, it can't because part of an API, and there can't be any further development in this direction until X, Y, and Z have been rewritten to be in like with ABC. I find it profoundly hard to get buy-in for those types of discussions. Everyone is happy to smile and nod and the appropriate tickets are filed with deadlines attached, but then the next release rolls around and there's new business-imperative stuff that's the focus and the cleanup tickets are quietly moved to backlog with the deadlines removed. Seeing this repeated over a number of years has left me with kind of a cynicism about the process, where it feels like code review is at least partly an exercise in frustration; I don't have the backing required to insist on doing it right upfront, so instead I'm really just getting a preview of what is going to land in my lap a year or two from now.
- klooney 2y ago
  The previews are valuable though, it makes you look like a wizard when you already know how something broke.
  - patrick451 2y ago
    Or incompetent. If you knew this was going to break, why did you approve it? Your only defense is "there was a lot pressure to get into the release xyz". There's not much sympathy for that defense. The animal spirits that thought the broken feature were the most important thing ever are long gone, and frustrations about the new outage caused by the previously most important feature ever have taken over.
    - klooney 2y ago
      Don't approve it! That's actually a thing you can do.
  - 2y ago
danielmarkbruce 2y ago
Never realized this was a debated topic. Are there smart people who believe in not having code reviews? What's the best argument against code reviews?
- bakje 2y ago
  I’ve spoken with a CTO who was against them because they add too much overhead, partly because they’re too late in the process. He encouraged his team to discuss an approach beforehand or to work on something together. Other than that they had a lot of tests and a very structured codebase, I guess it worked for them.
- dkdbejwi383 2y ago
  I worked in a team that didn't do reviews because _everything_ including spikes, research, etc, was done by two engineers pairing. This was remote, cameras on, all day. I found it utterly exhausting. I was somehow working at 100% capacity but producing output at 50% because so much of my cognitive bandwidth was taken up with the pairing process.
  - JackMorgan 2y ago
    I've been on full time pairing teams but noticed 2-5x more throughput because there's fewer meetings, code review, and surfing the web. It's exhausting because suddenly I'm actually writing code 7+ hours a day and that's really hard. Every other team I've been on without pairing, writing code is at most 3 hours a day. The rest of the time just gets chewed up by other communication channels.
- seadan83 2y ago
  I'd guess I've done somewhere in the ballpark of 1000 to 2000 code reviews. After that time, my opinion of them has drastically changed. I can give something of a long laundry list (my apologies for it being kinda ranty...): CR slows things down & often unnecessarily. Overly time consuming Time lost with needless explanations. Somewhat impossible when reviewer pool is one or two people. A bigger reviewer pool is probably not going to have to have context. Creates a bias to not ship. "waiting for CR" can be a big impediment for shipping. Perhaps that tonight was the good time for you to get something into prod, not tomorrow - but you're waiting for a CR. It's an example of process over people (AKA: cargo-culting). The context of when/where/how/why CR is important and is situational. CR best practices are going to have different results in different situations. Often CR is just done because it is a best practice, blindly. It would be preferable to think deeply about what is important & why - rather than just a "oh yeah, here is another hoop we jump through because #AmazonDidIt" Stratifies the team. The senior/elite/favored people invariably get easier times during review. CR can get political. Scrum masters & managers scrambling to unblock a team member and get something reviewed. Which is great for that one time, but reveals an utterly broken process otherwise. When a CR is "escalated", what's the chance the reviewer will actually spend the time needed and the back-and-forths to get things "properly" reviewed? Conducive to nitting, which are are useless and counter-productive. Time spent on nits is waste and draining to keep coming back to something to then tweak "is it good enough yet?" Drive by reviews without context Coming to agreement during CR is difficult. Not everyone is able to observe/experience and/or resolve conflict. CR is late in the code development process; it's one of the worst times to get feedback after something has been done, polished, tested, made production ready - and suddenly then someone thinks it should be done differently (which is a lot easier to see when something is already done and working). It's akin to writing code 3 times, once to get it to right, a second time to get it nice, and a third time for whatever the hell the reviewer wants. Shows lack of trust in team. It is gatekeeping. Does not scale well. I was once told by a reviewer that I write code faster than they could read it. (At the time I reviewed about 6 PRs every day, and sent out about 3. I was doing 80% of reviews, it was a shit-show; the reviews I received were slow and useless - the team members were focused on trying to deliver their over-stretched projects; I was too streched to give proper feedback and not work 100 hours a week). Better options exist, namely the "ship/show/ask" strategy: https://martinfowler.com/articles/ship-show-ask.html https://martinfowler.com/articles/ship-show-ask.html That latter branching strategy puts it up to the code author to decide, "does this comment typo fix need a review? Does this fix in this code that I authored originally and know everything about - really need a blocking review?" In that last case, if a person can just merge a bunch of precursor refactors right away, they can get that knocked out - the PR they send out for review is then clean & interesting; and there is no time lost managing branches or pestering someone A second option to help make things better is to allow "after-merge" reviews too. A few teams I've been on, we did that enough where we learned what kinds of things are good to ship on their own. To another extent, there wasn't the bandwidth to review everything. It was not a bad thing.
giancarlostoro 2y ago
> “kind, not nice” Always when joining a team the first thing I tell devs is "I don't care how critical you are, just be honest" I think setting expectations early on is very critical. I think people not feeling attacked / too defensive of code is a good step forward. People who vehemently defend their code are bad developers imho.
- 01HNNWZ0MV43FF 2y ago
  It sounds like radical candor and I like it
- rty32 2y ago
  Well, I have a colleague who spends more time defending his code by giving a list of wrong reasons, rather than actually fixing them. Happened more than once. I just move on. At some time he will realize how bad his code is.

saulpw 2y ago

How do you phrase these warnings? "Next time.."? I have a hard time being serious with my own warnings if it's fine enough for now.

audiodude 2y ago
"This is okay for now, but we should think about how we want to serialize these objects. Feel free to remove the N^2 algorithm in a follow up."
- est31 2y ago
  That works great in a setting where you are both employees of the same company, and you respect each other, but it often doesn't work in the open source world, people just disappear and you never hear from them again. It is possible that they do file follow-ups, but in my experience it's rare.
  - sfink 2y ago
    Yes, even within a company my threshold for accepting a change can vary pretty widely depending on my experience and relationship with the author. For an external contributor or someone I've never collaborated with (by reviewing code or having my code reviewed), I don't accept the code until almost everything is worked out to my satisfaction. With someone I work with regularly, it's not uncommon to accept a change with a comment like "this is all good, but you need to take X into account which will change almost everything in this patch" (I exaggerate, but only slightly). I know whether an update could be problematic and whether it is necessary to see it again. Sometimes there are a couple of obvious ways that something could be done, they picked one but weren't tied to it if I had a reason for picking a different one, I picked a different one for $REASON. Most are somewhere in between. Though in some ways it works the other way around. For an unfamiliar open source contributor, I need to be confident that the change is worthwhile. I will be lenient on stylistic things, and I'll just land their patch and then fix it up afterwards. For someone I've worked with a bunch (whether a familiar contributor or a coworker), I will trust their opinion on the underlying quality of a change, but be less tolerant of unnecessary stylistic differences since they should have already come into alignment on those and it's more likely to be an oversight if they missed something. (Plus, I don't want to be fixing up their changes after the fact, given that >90% of patches will come from regular contributors.)
  - seadan83 2y ago
    To add to this, "no time like now" is real. Follow-ups rarely happen, tracking them is a pain. Making someone accountable for it is unfair, particularly if they disagree with the follow up (and presumably they do, otherwise they would have done the thing in the first place).
elcomet 2y ago
We prefix our comments with "minor:", and all employees know that this means it's something that would be nice but not necessary to merge
seadan83 2y ago
Use the delete key. (My apologies for the snark, but I'm kinda serious, don't send those type of review comments at all). If it's fine for now, it's fine. If it's not fine - then speak up. If you do have such concerns, "fine for now" - that is something for an offline conversation - outside of the context of a CR. Which is also a good litmus test for how important a comment is. Would you schedule a 15 minute meeting to discuss the issue with your colleague? If not, then why is it okay to do that to someone during a CR?

jellyfishbeaver 2y ago

I am a new manager and I am struggling to get my team to understand the value in code reviews. I have been through so many rewrites and re-re-writes of spaghetti code, I am much more critical now reviewing code, and I am trying to promote this culture on my team. Do you have any suggestions? - The same people leave detailed comments on others' merge requests, but get discouraged when nobody else puts in the same amount of effort for theirs. - People blindly accept suggestions with no resistance or discussion to get the review over with. - People send their MRs to side channels or other teams to get their changes merged in without resistance or back and forth. (I've had to revert several of these).

spankalee 2y ago
Culture is made, it's not accidental. I would raise all these issues and more in group meetings. Try to get people to understand the many different benefits review brings to both the committer and reviewer - by having them state the benefits they want or could see getting. Talk about various kinds of comments (clear bugs, performance, style, robustness, factoring and organization, etc.), and the various priorities from no-action-required to nits to blockers. Talk about the priority of reviews themselves. Reducing the latency of reviews has a huge positive effect, ime.
- seadan83 2y ago
  People are smart - how about listen to their concerns? Do that before hitting them over the head with a list of 'best practices' and 'desired outcomes'. I'm starting to come to the opinion that nits are entirely and outright counterproductive in code reviews. Since I learned about "Ship/Show/Ask" - it's drastically changed my view of code reviews: https://martinfowler.com/articles/ship-show-ask.html https://martinfowler.com/articles/ship-show-ask.html; I no longer believe reviewing every change is healthy, seeing the difference of other models I can tell it's actively harmful now. Another good approach IMO is allowing for post-merge reviews, 'ship/show/ask' is better, but why is it written in stone that a code review MUST happen before merge?
ecshafer 2y ago
Good PR culture is definitely something that has to be built from the ground up, and supported top down. At Shopify, who I think has a really good PR culture we have a few things that I think help (beyond a good CICD, and static analysis tools): 1. PRs are supposed to wait for 2 acceptances, can be shipped with 1, and can be emergency shipped with 0. So the barrier is low, but the culture supports more. We are expected to get 2 reviewers from our team to okay. 2. Depending on the code project, we have to fill out a template for the PR, what is in it, what it changes, what to look for when we test the code, etc. 3. Some areas have code owners that might require an additional review from a specific team. 4. We are expected to check out, and test branches when we review them. So a quick read and LGTM is really discouraged outside of a few small cases. I have seen a lot of places that do the blind PR acceptance, and its tough because without this really being enforced and encouraged that culture is hard to change.

closeparen 2y ago

>time to meet with submitters around issues you find. What! I would be livid if someone scheduled a meeting with me about a PR. We have way too many meetings already, this is one of the only processes that is mercifully async.

hyperadvanced 2y ago
It doesn’t need to be formal or very long. I personally enjoy a PR meeting where we can poke at the code and understand it over someone dumping 2,000 lines of code in my lap at lunch time and hoping to get their spaghetti to prod by dinner
fiddlerwoaroof 2y ago
Me too, I really dislike meetings that could have been handled by asking me three or four questions in slack or in the PR and then waiting fifteen minutes or so for me to answer.
danielmarkbruce 2y ago
Livid? About a meeting to discuss work? Some comments in slack etc sound worse than intended, and people are aware of that and sometimes go out of their way to say it in a way to it's received as intended.
willio58 2y ago
To be clear I make time to meet if the submitter wants to talk through things. I don’t require meeting on every PR. I meet as needed on PRs, pretty infrequently as people get up to speed
seadan83 2y ago
"in-person" review can be the best I find. If a CR is going to have a dozen comments about everything, it's a disaster to do that async - the author is going to feel attacked. The dialog to resolve the conflict & disagreements is probably not going to play out well. Personally I like it when teams start with in-person CRs if they are not used to them. Only after healthy relationships and conflict resolution mechanisms established, do the PRs go async. I also like a rule that there should never be more than 2 round trips on communications in a CR, otherwise it should be done at the same time. Delaying small changes to allow for such round-trip times is not feasible in a lot of environments. There's too much to be done. Not worth it. When projects do fail, it's very rarely that some manager slaps a big "failed" label on it. I bring that up to say that projects fail all the damn time because the team is moving too slow - and individuals often don't appreciate when they actually did need to move faster (the feedback loop is often missing, of, "oh, we actually were too slow, we needed to ship faster than we actually did. The JIRA, planning, estimation, CR, best practices, time spent fixing linting - it all doesn't matter cause this project actually failed - it took too long.")

mikepurvis 2y ago

"Tech debt and code smell are real" I think what I struggle most with is that often times there's a valid business reason to "just ship it ASAP", but the missing piece is the accountability around the conditions attached to that. Like, okay, if we don't want to fix this now because it needs to be in the next release then we can merge it as-is, but you can't document this externally, it can't because part of an API, and there can't be any further development in this direction until X, Y, and Z have been rewritten to be in like with ABC. I find it profoundly hard to get buy-in for those types of discussions. Everyone is happy to smile and nod and the appropriate tickets are filed with deadlines attached, but then the next release rolls around and there's new business-imperative stuff that's the focus and the cleanup tickets are quietly moved to backlog with the deadlines removed. Seeing this repeated over a number of years has left me with kind of a cynicism about the process, where it feels like code review is at least partly an exercise in frustration; I don't have the backing required to insist on doing it right upfront, so instead I'm really just getting a preview of what is going to land in my lap a year or two from now.

klooney 2y ago
The previews are valuable though, it makes you look like a wizard when you already know how something broke.
- patrick451 2y ago
  Or incompetent. If you knew this was going to break, why did you approve it? Your only defense is "there was a lot pressure to get into the release xyz". There's not much sympathy for that defense. The animal spirits that thought the broken feature were the most important thing ever are long gone, and frustrations about the new outage caused by the previously most important feature ever have taken over.
  - klooney 2y ago
    Don't approve it! That's actually a thing you can do.
- 2y ago

danielmarkbruce 2y ago

Never realized this was a debated topic. Are there smart people who believe in not having code reviews? What's the best argument against code reviews?

bakje 2y ago
I’ve spoken with a CTO who was against them because they add too much overhead, partly because they’re too late in the process. He encouraged his team to discuss an approach beforehand or to work on something together. Other than that they had a lot of tests and a very structured codebase, I guess it worked for them.
dkdbejwi383 2y ago
I worked in a team that didn't do reviews because _everything_ including spikes, research, etc, was done by two engineers pairing. This was remote, cameras on, all day. I found it utterly exhausting. I was somehow working at 100% capacity but producing output at 50% because so much of my cognitive bandwidth was taken up with the pairing process.
- JackMorgan 2y ago
  I've been on full time pairing teams but noticed 2-5x more throughput because there's fewer meetings, code review, and surfing the web. It's exhausting because suddenly I'm actually writing code 7+ hours a day and that's really hard. Every other team I've been on without pairing, writing code is at most 3 hours a day. The rest of the time just gets chewed up by other communication channels.
seadan83 2y ago
I'd guess I've done somewhere in the ballpark of 1000 to 2000 code reviews. After that time, my opinion of them has drastically changed. I can give something of a long laundry list (my apologies for it being kinda ranty...): CR slows things down & often unnecessarily. Overly time consuming Time lost with needless explanations. Somewhat impossible when reviewer pool is one or two people. A bigger reviewer pool is probably not going to have to have context. Creates a bias to not ship. "waiting for CR" can be a big impediment for shipping. Perhaps that tonight was the good time for you to get something into prod, not tomorrow - but you're waiting for a CR. It's an example of process over people (AKA: cargo-culting). The context of when/where/how/why CR is important and is situational. CR best practices are going to have different results in different situations. Often CR is just done because it is a best practice, blindly. It would be preferable to think deeply about what is important & why - rather than just a "oh yeah, here is another hoop we jump through because #AmazonDidIt" Stratifies the team. The senior/elite/favored people invariably get easier times during review. CR can get political. Scrum masters & managers scrambling to unblock a team member and get something reviewed. Which is great for that one time, but reveals an utterly broken process otherwise. When a CR is "escalated", what's the chance the reviewer will actually spend the time needed and the back-and-forths to get things "properly" reviewed? Conducive to nitting, which are are useless and counter-productive. Time spent on nits is waste and draining to keep coming back to something to then tweak "is it good enough yet?" Drive by reviews without context Coming to agreement during CR is difficult. Not everyone is able to observe/experience and/or resolve conflict. CR is late in the code development process; it's one of the worst times to get feedback after something has been done, polished, tested, made production ready - and suddenly then someone thinks it should be done differently (which is a lot easier to see when something is already done and working). It's akin to writing code 3 times, once to get it to right, a second time to get it nice, and a third time for whatever the hell the reviewer wants. Shows lack of trust in team. It is gatekeeping. Does not scale well. I was once told by a reviewer that I write code faster than they could read it. (At the time I reviewed about 6 PRs every day, and sent out about 3. I was doing 80% of reviews, it was a shit-show; the reviews I received were slow and useless - the team members were focused on trying to deliver their over-stretched projects; I was too streched to give proper feedback and not work 100 hours a week). Better options exist, namely the "ship/show/ask" strategy: https://martinfowler.com/articles/ship-show-ask.html https://martinfowler.com/articles/ship-show-ask.html That latter branching strategy puts it up to the code author to decide, "does this comment typo fix need a review? Does this fix in this code that I authored originally and know everything about - really need a blocking review?" In that last case, if a person can just merge a bunch of precursor refactors right away, they can get that knocked out - the PR they send out for review is then clean & interesting; and there is no time lost managing branches or pestering someone A second option to help make things better is to allow "after-merge" reviews too. A few teams I've been on, we did that enough where we learned what kinds of things are good to ship on their own. To another extent, there wasn't the bandwidth to review everything. It was not a bad thing.

giancarlostoro 2y ago

> “kind, not nice” Always when joining a team the first thing I tell devs is "I don't care how critical you are, just be honest" I think setting expectations early on is very critical. I think people not feeling attacked / too defensive of code is a good step forward. People who vehemently defend their code are bad developers imho.

01HNNWZ0MV43FF 2y ago
It sounds like radical candor and I like it
rty32 2y ago
Well, I have a colleague who spends more time defending his code by giving a list of wrong reasons, rather than actually fixing them. Happened more than once. I just move on. At some time he will realize how bad his code is.