|
I had a bunch of guys who would request changes on the other's PRs as revenge for requested changes on their own PR. This slowed down the whole team since they were both very senior and working on big and important things. I told them to knock it off but we'll never know how I would have ended since the company was full of this kind of attitude at the top too and folded shortly after.
|
#
?
Mar 7, 2023 23:32
|
|
|
|
| # ? May 27, 2024 13:28 |
|
|
Leaving one line "do this" comments in code review is bad, and if I had to deal with that in my career I'd be very annoyed. That said, (and maybe you're already doing this) it's probably better to think of it less as "pushing back" and more as "hey can you help me understand why you made this suggestion?". Maybe they'll reflect and think "why am I making this suggestion? Does it really matter?" Maybe they'll share an insightful story about a time they did something similar to what you're doing and regretted it. Or maybe they'll just say "because I said so. I don't have time to explain". I've heard it happen before. That's the signal to find a better team.
|
|
#
?
Mar 8, 2023 00:29
|
|
|
Illusive gently caress Man posted:Leaving one line "do this" comments in code review is bad, and if I had to deal with that in my career I'd be very annoyed.
|
|
#
?
Mar 8, 2023 13:21
|
|
|
One time I asked in a PR why the person did something the way they did and they responded with "ugh fine I'll do it the other way" and I had to pull in my boss for a second opinion on whether I'd phrased my question rudely or something
|
|
#
?
Mar 8, 2023 16:10
|
|
|
Elaborating on this from last night a little bit: There are several kinds of norms that go into establishing any kind of code or design review process, and people tend to conflate them, possibly because they were never taught how to talk to coworkers, or maybe because they have absentee management. First is what people are supposed to do with feedback. I take a hard-line stance on this: we are all adults and there is no such thing as blocking a teammate from merging. Part of being an adult is understanding "you break it, you bought it". Your coworkers are there to give you their perspectives, advice, and feedback on your change. As an adult, you make a decision based on the inputs and information you're given. If you think your coworker is giving you good advice, and you have time, make the change. If you think your coworker is giving you bad or inapplicable advice, ask clarifying questions, within reason. But if your coworker is giving you advice and you think it would be better implemented as a fast-follow instead of holding up a fix or feature, be a grown-up and backlog the improvement. Throw it in the active sprint if you want (you would have done the work anyway if you had made the fix inline with your change). If your coworker is being unhelpful, you don't have to listen to them. Merge your change anyway, move on with your day. And if something goes wrong for the business because you were arrogant about your approach or you made a bad call to take a shortcut, at least now we can focus performance conversations on your decision-making instead of your velocity. Second is what kind of feedback you're supposed to give. And I really recommend that people just lay it out there. Optimize for being hyper-communicative, because we're all people and we can give feedback during retros on what kind of feedback turned out to be useful or not. It's not on your coworker to round-trip half a dozen times on your feedback to figure out what you would change, so lay out all the context you can like an executive summary and let your coworker make an informed decision. If your teammates tell you something is unhelpful or counterproductive, and you don't stop doing it, there's another performance conversation.
|
|
#
?
Mar 8, 2023 16:30
|
|
|
Illusive gently caress Man posted:Leaving one line "do this" comments in code review is bad, and if I had to deal with that in my career I'd be very annoyed. Yeah, the comments I got were (verbatim): quote:This should be a single test function. quote:Add field to this struct, add test case 1 and test case 2 to these tests, and remove the second test body. Which is like jesus, dude, either do it yourself or at least say the magic word. If you really want this specific implementation, then you could have also been more specific in the ticket you filed which was basically merge these two tests together. On the other hand, theres an argument to be had that if two tests are being merged then they should have as little duplicate code as possible. But then we get into the reason that Im somewhat frustrated about this, which is that Ive gotten burned out somewhat by this project and have started to care less than Id be proud of. So I had a working and acceptable but not completely DRY solution, which is what promoted the comments. quote:That said, (and maybe you're already doing this) it's probably better to think of it less as "pushing back" and more as "hey can you help me understand why you made this suggestion?". Maybe they'll reflect and think "why am I making this suggestion? Does it really matter?" Maybe they'll share an insightful story about a time they did something similar to what you're doing and regretted it. Its usually the latter, either because he has trouble explaining why he wants particular things, because he assumes everyone thinks the same way he does, or just because hes also just as burnt out as I am. Vulture Culture posted:As an adult, you make a decision based on the inputs and information you're given. If you think your coworker is giving you good advice, and you have time, make the change. If you think your coworker is giving you bad or inapplicable advice, ask clarifying questions, within reason. But if your coworker is giving you advice and you think it would be better implemented as a fast-follow instead of holding up a fix or feature, be a grown-up and backlog the improvement. Throw it in the active sprint if you want (you would have done the work anyway if you had made the fix inline with your change). If your coworker is being unhelpful, you don't have to listen to them. Merge your change anyway, move on with your day. As a permanent child, I find it so much easier to just not argue over it. These kinds of things are ultimately very minor and I dont really want to litigate poo poo like why did you break this hard-to-read 276-line chunk of test setup into smaller functions for the sake of clarity and ease of understanding instead of keeping all the logic inline cuz none of it is repeated anywhere. I just wanna get my work done. Id love to improve the codebase and the way we work, but everyone on the team has to buy into it or its pointless. quote:Second is what kind of feedback you're supposed to give. And I really recommend that people just lay it out there. Optimize for being hyper-communicative, because we're all people and we can give feedback during retros on what kind of feedback turned out to be useful or not. It's not on your coworker to round-trip half a dozen times on your feedback to figure out what you would change, so lay out all the context you can like an executive summary and let your coworker make an informed decision. If your teammates tell you something is unhelpful or counterproductive, and you don't stop doing it, there's another performance conversation. Yeah, Im actually going to undo the breakdown of the large function into smaller chunks because said coworker has previously expressed annoyance at it, and Ive already gotten the message that what I prefer to see in code is just that, a preference. Im on a team and if the team prefers this Ill roll with it. Pollyanna posted:Ill do you one better - I decided to push back on this one. Ill let you know how it goes. This ended up being a hallway conversation as we walked back from a meeting that basically boiled down to quote:why do you think we shouldnt do this So yeah Im just sick of debating minutiae with this dude and thus I cave quickly because whatever, our team is the pariah of the org anyway so what I do doesnt matter. Pollyanna fucked around with this message at 19:28 on Mar 8, 2023 |
|
#
?
Mar 8, 2023 19:17
|
|
|
Harriet Carker posted:Yep. Amazon too. I took a 10% pay cut moving from CA to WA even though I was fully remote in both places. Pretty lame. Amazon doesn't reduce pay for remote work. Were you by chance in the Bay Area? There are 4 location-based pay bands in the US and you likely moved down a band. In-office vs. remote wouldn't have mattered, though.
|
|
#
?
Mar 8, 2023 19:45
|
|
|
Not really relevant to your actual problem but I am kinda coming around to long functions lately vs. breaking things up into pieces. If the various blocks are commented it can be easier to follow the flow if it's inline rather than having to jump around, and also not having clarity on if the sub functions are used in multiple places or not. I dunno I go back and forth on code style stuff all the time, I certainly wouldn't clog up the PR process demanding someone change it but I might lay out the pros and cons of each approach to a junior.
|
|
#
?
Mar 8, 2023 19:55
|
|
|
It ultimately doesnt matter and I only broke it up on my local branch as a way to pick apart and analyze the program flow because anything like a 267 long chunk makes my eyes glaze over. Clearly, I am a bad programmer. (But also seriously small functions are not at all hard to reason about.)
|
|
#
?
Mar 8, 2023 19:57
|
|
|
prom candy posted:Not really relevant to your actual problem but I am kinda coming around to long functions lately vs. breaking things up into pieces. If the various blocks are commented it can be easier to follow the flow if it's inline rather than having to jump around, and also not having clarity on if the sub functions are used in multiple places or not. I dunno I go back and forth on code style stuff all the time, I certainly wouldn't clog up the PR process demanding someone change it but I might lay out the pros and cons of each approach to a junior. Studies seem to indicate longer functions are easier to work with. Lower time to resolve issues and bug counts. The best place is probably in some middle ground. The two obvious extremes, entire program in main and every line a separate function, are obviously both terrible. Smaller functions are definitely not better or virtuous by default.
|
|
#
?
Mar 8, 2023 19:59
|
|
|
I can work with that.leper khan posted:The two obvious extremes, entire program in main and every line a separate function, are obviously both terrible. Though this particular case is close to the former, and very far from the latter. But eh, Im letting it go - if it works it works.
|
|
#
?
Mar 8, 2023 20:01
|
|
|
For my personal stuff, things i might otherwise write a function for only to be used in that one place and to improve readability I just throw in its own scope block with a comment
|
|
#
?
Mar 8, 2023 20:02
|
|
|
If its only ever you, sure, but if you have a large team with some newbie engineers and you have a big chonker everyones gonna groan and mentally deprioritize understanding the whole thing. Honestly it doesnt matter, whatever.
|
|
#
?
Mar 8, 2023 20:04
|
|
|
Pollyanna posted:If it’s only ever you, sure, but if you have a large team with some newbie engineers and you have a big chonker everyone’s gonna groan and mentally deprioritize understanding the whole thing. Splitting into inlined functions for use only by the parent function and scoping sections of a function into blocks are isomorphic with the only difference that the scoped sections flow linearly. The danger lies in losing context of what those methods actually do, or losing the ability to follow what the larger method is doing (is this function before/after that function?) It's a small thing, but insofar as I've seen studies on it the 'better' approach is larger methods. There are ways to delineate and make them just as or more legible than shattering them into small methods.
|
|
#
?
Mar 8, 2023 20:10
|
|
|
Fair nuff. I guess this is more about my burnout and programming fatigue, in the end.
|
|
#
?
Mar 8, 2023 20:12
|
|
|
prom candy posted:Not really relevant to your actual problem but I am kinda coming around to long functions lately vs. breaking things up into pieces. If the various blocks are commented it can be easier to follow the flow if it's inline rather than having to jump around, and also not having clarity on if the sub functions are used in multiple places or not. I dunno I go back and forth on code style stuff all the time, I certainly wouldn't clog up the PR process demanding someone change it but I might lay out the pros and cons of each approach to a junior. http://number-none.com/blow/john_carmack_on_inlined_code.html is an interesting argument in favor of single long functions. I don't disagree, but I write a lot of small functions these days because I write a lot unit tests and it's easier to write unit tests for small functions. Now ask me if it's easier to write 3 sets of unit test for 3 small functions or one set of tests for one function  Writing (good) code is largely about tradeoffs and there's always going to be someone who disagrees with the tradeoff you made. LLSix fucked around with this message at 20:25 on Mar 8, 2023 |
|
#
?
Mar 8, 2023 20:22
|
|
|
KS posted:Amazon doesn't reduce pay for remote work. Were you by chance in the Bay Area? There are 4 location-based pay bands in the US and you likely moved down a band. In-office vs. remote wouldn't have mattered, though. Yeah I was in the bay area. I shouldn't have said they pay less for remote, more that they pay differently depending on where you live even if you're "remote" either way, which is pretty stupid.
|
|
#
?
Mar 8, 2023 20:26
|
|
|
I'm always worried if I buy a new house here it will be used as an opportunity to adjust my salary when I "move" even though I'd only be moving between two identical CoL areas possibly even in the same town.
|
|
#
?
Mar 8, 2023 20:38
|
|
|
Pollyanna posted:why did you break this hard-to-read 276-line chunk of test setup into smaller functions for the sake of clarity and ease of understanding instead of keeping all the logic inline cuz none of it is repeated anywhere. I've started trying to relay my experience in reviews and stop there. For this example, I might say something like "I found this function hard to follow because I had to keep jumping up and down the file into smaller functions instead of reading top to bottom". (Assuming that was my experience; if I had no trouble reading it, I wouldn't bring it up as a cute rephrasing of some Rule I've decided is Correct.) That presents you with an actual problem ("I, a code reader, got confused"). You can decide whether/how to address it (e.g. thinking "nobody else will ever read this code, it's basically write-only" as you scroll past my comment is totally ok). Or sometimes there's an insightful discussion (e.g. "I do reviews in the ide so I can quickly jump to an implementation and go back with a hotkey, what's the problem" "well I use the web ui so I can't do that so easily"). Obviously that doesn't help you as the receiver, but I've also had success doing it in response to a prescription. "How far did you get in the new code before you realized you were jumping up and down the file a lot?" Maybe we discover that smaller functions 2 and 3 should be combined, but the rest is plenty readable. It's like the X-Y problem.
|
|
#
?
Mar 8, 2023 22:05
|
|
|
This is the Oldie thread so I'll share: I didn't have a PR up in front of a peer until my 30's. The first one hit me like a hammer, had to take a walk around the building and calm down. I was putting way too much ego into it, assuming my code was perfect on the first shot, lots of silly ideas in my head at the time. When a style thing got criticized I pointed out where I'd seen it in other files. After a while I noticed it was 50/50 between "oh, sure, I guess we do that now" and "aha! that's old/wrong, fix that too!" It's really not personal, the lens I'm generally looking through reviews now is how much of a pain would it be to come through here and add another similar feature. We all want good code, PR is a process to get there. That said... Pollyanna posted:Yeah, the comments I got were (verbatim): just do the thing Clear, direct communication with a 1:1 code change is like my ideal comment. I'm not as bothered by the lack of a "please" but compared to a fake question or a grand philosophical argument? I'll take the direct order every time. "It wasn't in the original ticket"? Whooo caaaares, this description got us halfway there and the PR's a fine place to continue the discussion.
|
|
#
?
Mar 8, 2023 23:32
|
|
|
JawnV6 posted:???? im sorry im not seeing it? Personally I would be peeved to get the kind of feedback Pollyanna posted because it feels like a command dictating the structure of something otherwise very subjective. "You need to do exactly this thing" works for me if the consequence of not doing the thing is some weird error, but otherwise yeah if you wanted veto power over each line do it yourself. but there's all kinda people in the world so if teams get by leaving this kind of feedback on each others' work then 🤷 I'm the weirdo who wishes there was less asynchronous feedback and people just talked to each other with their voices 😢
|
|
#
?
Mar 8, 2023 23:46
|
|
|
Sivart13 posted:Personally I would be peeved to get the kind of feedback Pollyanna posted because it feels like a command dictating the structure of something otherwise very subjective. Missing information: was it actually subjective or how things were done there/in that codebase (documented or otherwise)? I find there are a lot of issues around this kind of thing largely because it's hard/not necessarily justifiable to excruciatingly document the institutional knowledge of how this particular thing works. And for other things it's totally worth it. So this is a whole lot of missing context that sounds like a standard mix of bruised ego vs. overzealous gatekeeper in some proportion I can't determine from the available information.
|
|
#
?
Mar 9, 2023 00:01
|
|
|
I filed a ticket expecting it to land back on me without having to flesh out every detail for the expected implementer, me. It slipped later than folks wanted so my manager assigned it to someone else on my team. The first time I'm thinking of it since filing is when the PR pops up. What're they supposed to do here? Approve without complaint? Cheerlead this amazing code that's not quite right? What's polly's expectation? PR's sail through with rubber stamps and never get any clarification or modification? She understands everything perfectly from (lovely, insufficient) descriptions? This really looks like a perfectly normal flow. What's the line count here? 30? Just do the changes. I don't think it's worth raising "Add field to this struct, add test case 1 and test case 2 to these tests, and remove the second test body." up to HR or anything and I understand why that level of detail may not be in the original. If it was a major design/API decision, it may have been worth planning it out more and not having the PR be the first reminder. But this seriously sounds like ~15 lines of cleanup being requested in a PR and.... that's fine?????
|
|
#
?
Mar 9, 2023 00:07
|
|
|
it leaves no headroom of objectivity for when actually objective things come up. "this is a sql injection fix this" needs to feel different from "here is style change" imo
|
|
#
?
Mar 9, 2023 00:39
|
|
|
bob dobbs is dead posted:it leaves no headroom of objectivity for when actually objective things come up. "this is a sql injection fix this" needs to feel different from "here is style change" imo Consistent language in a team or org for this stuff helps a lot. Not having consistent language leads to people not having shared understanding. I'm a fan of prefix "consider" for things I would do that have no bearing, prefix "nit: " for things against specified style that wouldn't hold the review (whitespacing to company or local standard, iterator names, whatever), and "issue" for things that need to be fixed
|
|
#
?
Mar 9, 2023 00:52
|
|
|
I like the habit of flagging some things in a PR as a dumb nitpick. My boss actually types NIT. Its nice a way of saying style thing but not important. But I always just say yes to everything in a PR review anyways, unless its egregious or a whole new feature. The only time I really get mad is when I send a PR back, and its returned again with a whole new set of recommendations that should have been brought up the first time. Now youre just wasting time.
|
|
#
?
Mar 9, 2023 01:33
|
|
|
lifg posted:I like the habit of flagging some things in a PR as a dumb nitpick. My boss actually types NIT. It’s nice a way of saying “style thing but not important.” If I send back over a dozen things that need to be fixed, i probably missed a few.
|
|
#
?
Mar 9, 2023 01:59
|
|
|
If people are going to reject a PR over style issues (and function length is a style issue) there should be a style guide they can point to. Like, feel free to bring up preferences and "this is hard to read" but if you reject the PR over it, the issue should be real.
|
|
#
?
Mar 9, 2023 02:26
|
|
|
Nitpicks shouldnt be in PRs. Use a linter and automate away things that people can bicker about.
|
|
#
?
Mar 9, 2023 02:33
|
|
|
lifg posted:But I always just say yes to everything in a PR review anyways, unless its egregious or a whole new feature. The only time I really get mad is when I send a PR back, and its returned again with a whole new set of recommendations that should have been brought up the first time. Now youre just wasting time. Yeah, that can be annoying. I'm guilty of this from the other side sometimes. I treat PRs as interrupts and if someone submits unreadable code while I'm busy I'll sometimes drop comments about improving readability and not even try to review until they've added comments so I can tell what they're trying to do. (There's one engineer on the team who used to never even give a description of what his 5000 line PRs were doing but he's getting better. Slowly.)
|
|
#
?
Mar 9, 2023 02:35
|
|
|
StumblyWumbly posted:If people are going to reject a PR over style issues (and function length is a style issue) there should be a style guide they can point to. Oh yeah I only reject a PR if I spot an actual defect. Otherwise it's all "here's some thoughts".
|
|
#
?
Mar 9, 2023 02:51
|
|
|
luchadornado posted:Nitpicks shouldnt be in PRs. Use a linter and automate away things that people can bicker about. I've never seen a linter that can tell you a better name for a function or variable, which is the thing I often nitpick about.
|
|
#
?
Mar 9, 2023 03:03
|
|
|
I'd say that names are very important to readability and maintainability - not a nitpick at all IMO
|
|
#
?
Mar 9, 2023 03:09
|
|
|
I think I have my answer now, thanks everyone.
|
|
#
?
Mar 9, 2023 03:10
|
|
|
The best thing we did at my last job was to implement a PR check that ran clang-format to match our style guide and blocked if there were any changes.
|
|
#
?
Mar 9, 2023 03:16
|
|
|
I tend to review PRs with junior devs for some period of time together in a realtime meeting, especially when they've recently joined, and/or there are lessons to learn, and if nothing else to show them how I approach reviews. The comment will end up being short (sometimes a one-liner), but it comes with a discussion.Jabor posted:I've never seen a linter that can tell you a better name for a function or variable, which is the thing I often nitpick about. Yeah, I've repeated "the function name is this, but that's not what it does" many times. It's amazing how complicated of a maze a person can paint themselves (and the rest of us) into by not calling things what they really are.
|
|
#
?
Mar 9, 2023 03:16
|
|
|
Nowadays, if you're my peer or above, I'll just approve unless something is egregiously wrong like the feature implemented doesn't work or the same block is copy pasted multiple times. If you're a junior, I'll say more about style and other non critical things. Ither fucked around with this message at 04:16 on Mar 9, 2023 |
|
#
?
Mar 9, 2023 04:11
|
|
|
scrutinizing a function's name and reconciling that with all it actually does is a pretty big share of my code review work. It's usually well received feedback too. It does require a lot more focus and effort in grokking what's being done vs nitting on idiomatic language faux pas or whatever
|
|
#
?
Mar 9, 2023 04:13
|
|
|
Sivart13 posted:
Oh god are you one of those people who try and make everything a voice call instead of just a message? luchadornado posted:Nitpicks shouldnt be in PRs. Use a linter and automate away things that people can bicker about. Nitpicks are fine as long as they are labelled as such. We all use a browser extension that puts a description before the comment to tell type and if it's a blocker or not. ie "Nitpick - Non-blocking: I think this variable could be better named" Mega Comrade fucked around with this message at 09:01 on Mar 9, 2023 |
|
#
?
Mar 9, 2023 08:58
|
|
|
|
| # ? May 27, 2024 13:28 |
|
|
It's not 1880, we can do asynchronous voice and video!
|
|
#
?
Mar 9, 2023 15:05
|
|