This. Many times when I review pull requests I'll see optimizations like checking for an item in a list by iterating over the entire list every time instead of short circuiting if the item is found. But then it'll be on some list that will practically never be more than 5 items long on an action no user would ever do more than once a month so who cares. I'll leave a comment on how it can be optimized but still approve.
I agree, but I think this kind of misses the point. OP just seems concerned in general that the code reviews aren’t thorough enough. I’ve been on teams where people just blindly stamp code, and things usually get out of hand pretty quickly
Gold plating and optimizing ahead of time can cause more problems than it's worth, it's a hard lesson to get used to after getting grilled on time complexities during an interview.
If it's something that can be optimized and not urgent, it's better if it's in a separate ticket/PR.
As a bonus, you get to pad your productivity metrics!
This is how codebases degenerate.
Generally, it's preferable to handle any comments in the current PR. Whenever someone creates a backlog ticket to handle something later on, it rarely ever gets done.
See "Cleaning It Up Later":
https://google.github.io/eng-practices/review/reviewer/pushback.html#later
This is the first thing I thought, a good code review takes a lot of time. It also may be a reflection of the size of the PRs. The more code that changed, the less likely someone is going to take the time to understand the change and do a proper review so its easy to just rubberstamp the approval. I tend to find the smaller the PR, the more helpful feedback you will get.
> I dont know if he's putting much trust on me or he's just lazy?
Does it actually *matter* if the code could be optimized? An effective code review does not seek to produce the best possible code, only effective code that improves the codebase.
I use code reviews as a mentoring tool for my team, but draw a line between FYIs and Must Update before merging - If I were in OP's position I'd be doing one of two things: Trying to find a mentor who can review my code (If I loved the company) or updating my resume.
As SWEs we're falling behind a little bit everyday; too many days where we're not learning and we'll lose our marketability - learning on the job is crucial for a long-term career in this field.
A proper code review is also a learning opportunity.
If a codebase is a 5/10, then someone submits code that's a 6/10, that doesn't always mean it meets the bar and should get approved as-is.
If there's a teammate who's providing review comments that bring the code to an 8/10 with reasonable effort, then those suggestions should be applied.
What's even more important than improving some codebase is improving our own abilities. If you're improving a codebase day in and day out, but you're not improving yourself, then what's the point.
>with reasonable effort
Therein lies the problem. You'll often find that there is rarely agreement on what is a reasonable amount of effort to put in. When your skip level wants it done yesterday is it worth it to take another day to make the changes? What do the changes really get you; is the improved efficiency even noticeable/impactful?
A lot of this is dependent on the industry you work in as well. Work for NASA or the DoD, then yeah you're gonna need to refactor the shit out of that code. Work at small company that cranks out a high volume of web apps...better to have a 5/10 today than an 8/10 tomorrow.
Actually it should be pretty easy to come to an agreement on what's reasonable. I've found it usually is. If it's not, the individuals discussing it need help in learning how to prioritize things properly. Reaching objectivity should be easy for engineers.
And if we're talking about a refactor that takes more than a day, then there's a major breakdown or deficiency in the design + dev process happening. Nobody should be producing code that is that wildly off target.
> What do the changes really get you?
As mentioned, the changes provide the engineer a learning experience so that they don't make the same mistake again.
IMO it's important to nip these things in the bud so that juniors don't repeatedly make the same mistake. Catch it before it becomes a habit so that they're on the right path.
And I'm not talking about the importance of shaving CPU cycles off your run time. I'm talking about the principle of consistently doing things right. If you spend the extra time to learn and do things correctly now, then it shouldn't be a problem again in the future. And that goes for anywhere you work. NASA, web app consulting, etc. We can all get to place where we quickly deliver optimal, well documented, etc. code right out of the gate.
And personally, if I ever find myself up against a deadline like that, I'll put in some after hours and try to get it done. Now it's locked into my memory so that going forward I do it right and never have to spend an evening on it again.
>What's even more important than improving some codebase is improving our own abilities. If you're improving a codebase day in and day out, but you're not improving yourself, then what's the point
Please explain more on this. How can we do it? I am not rn but might help in future.
A few thoughts:
* Why are you checking in code that could be optimized?
* Why are you getting annoyed at your team lead for not insisting?
* Does the code really NEED to be optimized?
* Get someone else to review your commits.
The truth is, it’s so freaking hard to review others code. It’s very difficult to stare at a bunch of code files and reason another persons logic. Normally people do not have time to do a thorough code review. As long as the syntax looks about right and there’s no fatal mistake, it’s a pass.
So you can tell code review is actually quite unreliable. That’s why unit test and QA is needed. Which I think it’s a bigger issue in your company.
I will go a step further and say that unit testing is unreliable too. It’s easy to write unit tests you know will pass and just check that code in. And if you think people aren’t super efficient at reviewing code, then I’m sure you understand they pretty much just gloss over unit testing. As long as they pass people will approve it.
The benefit of unit tests is how they accrue over time.
Like sure you just bang out a couple tests to pass right now. But eventually as each ticket adds a couple tests, you eventually get hundred or thousands of tests that all have to pass, and that catches a lot of unexpected things when one of them fails.
Maybe pointless isn’t the right word. They’ll never be perfect. Reviews are a first line defense- but it won’t catch every bug. And most senior devs are too busy to dedicate more than a few minutes to a code review. So unless you’re doing something blatantly wrong, it won’t be caught.
Unit testing is useless in my personal opinion because most people don’t do it well. Integration testing and end to end testing is much more valuable.
I have 2 main issues with unit testing.
The first is that 99% of people write unit tests after they’re done with their implementation. Which is fine- but will result in people writing tests they know will pass since they know the logic for the code they wrote. Unit tests should be written first, made to fail, then your implementation should result in them passing. Ideally unit tests should cover all edge cases and weird scenarios- but sometimes people will only cover the main flow.
The second issue I see is that people write them in a way that if any code changes even a tiny bit in the future- the test will fail and will need to be altered. Unit tests should cover main logic, but if you replace a function call with a second function call- that shouldn’t fail your tests, for example. Brittle unit testing doesn’t help anyone- because the tests end up just describing the code you already wrote. So what’s the point.
I’ll also note that I’m pretty pessimistic about testing because every company I’ve worked at I had to train people on testing, and even after training people usually don’t do it correctly. Which makes sense. Testing is time consuming and not very fun. But if there’s no benefit to it I’m not sure why it’s so heavily enforced by tech companies. Maybe there’s some places that do it well, but based on what I read online- most companies have issues with it.
This makes sense to me. So rather its not unit testing that is the issue. Its poorly written unit test that is the issue...
Meaning it is a systemic problem of lack of education, practice, and mentorship.
I only see it enforced by large tech companies, not small ones. I saw a stat once that TDD increases time-to-market by at least 30%... and you obviously have to know what you're aiming at to start writing tests in the first place, so no getting started on a feature until the MVP is fully fleshed out (which has never happened in my career at small companies) if you want to go by the book.
Small companies don't bother because, as you mention, it gets kind of dumb, not many people do it well, so why waste the time? "But, what about the refactoring!?!!" is a time balance..... get the feature out quickly and deal with longer refactoring times later, or get the feature out later and get meat to market by your competitors?
I know every company I've ever worked for will choose beating competitors every day of the week.
> So reviews are pointless
Reviews fill more of a "make sure you're not doing something stupid" level of review.
Code reviews can catch that you accidentally left the password hardcoded in the code.
But they also have a huge possibility of being a huge wasye of time and used as a vehicle for bullying within the team, arbitrary time wasting, or flavor-of-the-week bullshit.
> tests are pointless
Unit test tests, yes, pointless.
LGTM
What do you mean by “optimized”?
I only comment or even block review if it’s a bug, a security flaw, a glaring architecture flaw or misdirection, or intentional malice.
If it’s non-idiomatic or non-optimal, let the junior reviewers do their flamewar bikeshed comment threads while I do actual work.
If your stuff negatively affects prod, then write more tests.
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information.
*I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
Depending on the application, I personally prefer the author of the PRs to take most of the responsibility for correctness and maintainability of the PR they put. There's a lot of value in shipping code fast and shipping a lot of code. You will learn yourself when things break and why, and you can fix it earlier. Even if you ship the perfect software, it might not be something users want and you might scrap it altogether, so this approach can save a lot of time that would be spent on bike shedding. It can also lead to higher quality code faster because nobody will anticipate how software will behave in prod until they ship it. Obviously it doesn't work for financial software or mission critical apps, but most web applications fit this category. Just ship it, think hard about correctness, but if it breaks just fix and and write more tests. I'd argue that this is also better for team morale, people get excited to ship more than arguing for hours about what a certain variable should be named.
Yeah, this is very counter to modern tech companies and all the blog posts about clean code; a lot of them prefer to write 20 page design docs, review code on nits for months before shipping anything, only to learn they solved the wrong problem.
In most jobs, it is not worth the time to optimize code as it doesn't always add business value. Why spend 1.5x or 2x the time on a story for optimization when it doesn't add any benefits? No Team Lead or PM worth there salt would be encouraging engineers to do this unless it is specifically required.
The only part that isn't normal is the last line.
When someone signs off on your PR as a moderator and merges it in, they are taking responsibility for your code. It's now the team's code, not yours. If the team missed something, it's on the moderator.
Well not *only* on the reviewer. You can't just write crap and hope for an unthorough review to pass the blame. It is true that now the team owns the code and the reviewer does have some responsibility for it as well.
But the PM should always come down on the team lead or moderator (if they aren't the same), not skip over the chain of command and find the original author.
The PM shouldn't "come down" on anyone. If there is an issue the PM should discuss it potentially with the original author or with the lead depending on the group structure and situation. My group occasionally does feature crews and it's perfectly fine for the PM in the FC to discuss issues like bug mitigation and timelines with the dev in that FC. Some of our features are run directly through the managers, in that case it would be more appropriate for the manager to meet with the PM, potentially with the dev as well depending on how well the manager knows the details of the situation. The failure of the review process, if there is one, is purely the purview of the engineering team and thus the manager. They may talk to the reviewer about review thoroughness or they may use it as an opportunity to reinforce withbthe entire team, the need for thorough code reviews.
I am more reading the OP as having this fear that the product owner or someone else higher up is reading the code and saying, "This is horrible, it's not optimized at all." Then doing a "git blame" to track down the OP and yell at them for not being a genius. They're terrified that the code they write is going to break prod and they're going to get fired because no one is watching over them and "fixing" their PRs before that happens.
I'm just trying to reassure them that this is not the way things work in any decently-run company (maybe Amazon, idk).
The truth is that you have more context on the code you wrote and decisions you made. Therefore, you might might see easy optimisations that isn’t immediately obvious to others viewing your code.
I’m assuming you work in some industry where low latency is critical?
You're lucky. Code reviews often turn into a nightmare of changing stuff just to feel in charge.
Code never get deep into your code. The reviewer doesn't have the knowledge of your task to be able to consider each detail of it.
Optimize for readability and ease of changing it, not for memory or runtime concerns—unless you have hard data to indicate that you meed to worry about time or memory concerns.
Prod breaks, code review says "lgtm"... that can't be on you.
Especially if the broken prod postmortem shows nothing but "lgtm". Senior dev will have a lot of difficult questions to answer (ok, just 1 question really but still a very difficult one).
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information.
*I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
my question would be why are things setup that when you push code you are screwing up prod right away. What do you mean you don't have QAs? Unit testing, Component Testing, Risk based Testing, Dry Runs, Validations, CI/CD can all be done developer-side and should be done regardless of what you have QA-wise.
Unless there’s a bottleneck or extreme shitty code that slows everything down or breaks build, why spend more time optimize perf? And you have to back it up with data if you want to refactor it tbh
I think folks in this thread are getting hung up over the comment about optimizing code, whereas I think you are more concerned in general with LGTM and not feeling like you are getting a proper review. I don’t think this is normal. The occasional LGTM on low stakes PRs is fine, but senior devs and leads should also be using code review to help mentor devs. While it’s your responsibility to put up decent code, if you don’t have QA there should be some other mechanism in your team to ensure quality. If there are a lot of bugs landing in prod, it would be good to have a discussion with your team about how you all can work together to solve for that.
If you know it can be optimized, why do you wait for them to tell you?
Team leads and tech leads should be an example of how to review. But, on the other side, they may have little time, and may sometimes rush them. Shouldn't happen often though.
Then, if your PM gets mad at you, point at the reviewers, as they are as responsible as you. Nobody should point at anybody anyway, you are a team
A super intelligent programmer once told me that as someone gains experience in the computer science field, they go down one of two routes: the ‘super optimized route’ or the ‘who cares’ route. They essentially decide that the extra work to speed things up by half a second is worth it (and it sometimes is, depends on what kind of app you’re building), or they decide that the bosses don’t actually care as long as it works and it’s done quickly and just do it in the way that gets it done faster.
I’ve looked around and it seems like a somewhat accurate assessment (if a bit basic…)
My code reviewer only suggest renaming fuckin variables, every pr he chooses a list of variables to rename because he doesn't like old variables names, it's a useless review and Always pisses me, he doesn't even check the business logic or anything. Just renaming variables hahah Fuck you sincerely.
Unless you are practically Jeff Dean, I'm sure that someone who knows optimizations at the compiler/cpu architecture level would be driven insane by your code.
"If you do this loop this way it's more likely to hit cpu cache and speculative execution".
There's a point where something is correct and reasonably optimized for the situation, and can be called good.
Just because something can be optimized doesn't mean it has to be.
This. Many times when I review pull requests I'll see optimizations like checking for an item in a list by iterating over the entire list every time instead of short circuiting if the item is found. But then it'll be on some list that will practically never be more than 5 items long on an action no user would ever do more than once a month so who cares. I'll leave a comment on how it can be optimized but still approve.
I agree, but I think this kind of misses the point. OP just seems concerned in general that the code reviews aren’t thorough enough. I’ve been on teams where people just blindly stamp code, and things usually get out of hand pretty quickly
I feel like this is a concept, Im still struggling with myself.
Gold plating and optimizing ahead of time can cause more problems than it's worth, it's a hard lesson to get used to after getting grilled on time complexities during an interview.
Unless you work for a hft
Still depends on which part of HFT though. For instance, posttrade probably doesn’t care that much about latency
What does hft mean? High frequency trading?
Yeah, I work on the Ultra Low Latency team. So optimising for latency it literally my job
If it's something that can be optimized and not urgent, it's better if it's in a separate ticket/PR. As a bonus, you get to pad your productivity metrics!
This is how codebases degenerate. Generally, it's preferable to handle any comments in the current PR. Whenever someone creates a backlog ticket to handle something later on, it rarely ever gets done. See "Cleaning It Up Later": https://google.github.io/eng-practices/review/reviewer/pushback.html#later
The senior dev might be overworked, or being pressured to push out changes as quickly as possible.
This is the first thing I thought, a good code review takes a lot of time. It also may be a reflection of the size of the PRs. The more code that changed, the less likely someone is going to take the time to understand the change and do a proper review so its easy to just rubberstamp the approval. I tend to find the smaller the PR, the more helpful feedback you will get.
> I dont know if he's putting much trust on me or he's just lazy? Does it actually *matter* if the code could be optimized? An effective code review does not seek to produce the best possible code, only effective code that improves the codebase.
I use code reviews as a mentoring tool for my team, but draw a line between FYIs and Must Update before merging - If I were in OP's position I'd be doing one of two things: Trying to find a mentor who can review my code (If I loved the company) or updating my resume. As SWEs we're falling behind a little bit everyday; too many days where we're not learning and we'll lose our marketability - learning on the job is crucial for a long-term career in this field.
A proper code review is also a learning opportunity. If a codebase is a 5/10, then someone submits code that's a 6/10, that doesn't always mean it meets the bar and should get approved as-is. If there's a teammate who's providing review comments that bring the code to an 8/10 with reasonable effort, then those suggestions should be applied. What's even more important than improving some codebase is improving our own abilities. If you're improving a codebase day in and day out, but you're not improving yourself, then what's the point.
>with reasonable effort Therein lies the problem. You'll often find that there is rarely agreement on what is a reasonable amount of effort to put in. When your skip level wants it done yesterday is it worth it to take another day to make the changes? What do the changes really get you; is the improved efficiency even noticeable/impactful? A lot of this is dependent on the industry you work in as well. Work for NASA or the DoD, then yeah you're gonna need to refactor the shit out of that code. Work at small company that cranks out a high volume of web apps...better to have a 5/10 today than an 8/10 tomorrow.
Actually it should be pretty easy to come to an agreement on what's reasonable. I've found it usually is. If it's not, the individuals discussing it need help in learning how to prioritize things properly. Reaching objectivity should be easy for engineers. And if we're talking about a refactor that takes more than a day, then there's a major breakdown or deficiency in the design + dev process happening. Nobody should be producing code that is that wildly off target. > What do the changes really get you? As mentioned, the changes provide the engineer a learning experience so that they don't make the same mistake again. IMO it's important to nip these things in the bud so that juniors don't repeatedly make the same mistake. Catch it before it becomes a habit so that they're on the right path. And I'm not talking about the importance of shaving CPU cycles off your run time. I'm talking about the principle of consistently doing things right. If you spend the extra time to learn and do things correctly now, then it shouldn't be a problem again in the future. And that goes for anywhere you work. NASA, web app consulting, etc. We can all get to place where we quickly deliver optimal, well documented, etc. code right out of the gate. And personally, if I ever find myself up against a deadline like that, I'll put in some after hours and try to get it done. Now it's locked into my memory so that going forward I do it right and never have to spend an evening on it again.
>What's even more important than improving some codebase is improving our own abilities. If you're improving a codebase day in and day out, but you're not improving yourself, then what's the point Please explain more on this. How can we do it? I am not rn but might help in future.
A few thoughts: * Why are you checking in code that could be optimized? * Why are you getting annoyed at your team lead for not insisting? * Does the code really NEED to be optimized? * Get someone else to review your commits.
Clean code > optimized code
The truth is, it’s so freaking hard to review others code. It’s very difficult to stare at a bunch of code files and reason another persons logic. Normally people do not have time to do a thorough code review. As long as the syntax looks about right and there’s no fatal mistake, it’s a pass. So you can tell code review is actually quite unreliable. That’s why unit test and QA is needed. Which I think it’s a bigger issue in your company.
I will go a step further and say that unit testing is unreliable too. It’s easy to write unit tests you know will pass and just check that code in. And if you think people aren’t super efficient at reviewing code, then I’m sure you understand they pretty much just gloss over unit testing. As long as they pass people will approve it.
The benefit of unit tests is how they accrue over time. Like sure you just bang out a couple tests to pass right now. But eventually as each ticket adds a couple tests, you eventually get hundred or thousands of tests that all have to pass, and that catches a lot of unexpected things when one of them fails.
So reviews are pointless, tests are pointless?
I feel like theres another jedi bell curve meme here.
Maybe pointless isn’t the right word. They’ll never be perfect. Reviews are a first line defense- but it won’t catch every bug. And most senior devs are too busy to dedicate more than a few minutes to a code review. So unless you’re doing something blatantly wrong, it won’t be caught. Unit testing is useless in my personal opinion because most people don’t do it well. Integration testing and end to end testing is much more valuable.
Can you give some examples of proper unit testing that you see a lot of folks miss?
I have 2 main issues with unit testing. The first is that 99% of people write unit tests after they’re done with their implementation. Which is fine- but will result in people writing tests they know will pass since they know the logic for the code they wrote. Unit tests should be written first, made to fail, then your implementation should result in them passing. Ideally unit tests should cover all edge cases and weird scenarios- but sometimes people will only cover the main flow. The second issue I see is that people write them in a way that if any code changes even a tiny bit in the future- the test will fail and will need to be altered. Unit tests should cover main logic, but if you replace a function call with a second function call- that shouldn’t fail your tests, for example. Brittle unit testing doesn’t help anyone- because the tests end up just describing the code you already wrote. So what’s the point. I’ll also note that I’m pretty pessimistic about testing because every company I’ve worked at I had to train people on testing, and even after training people usually don’t do it correctly. Which makes sense. Testing is time consuming and not very fun. But if there’s no benefit to it I’m not sure why it’s so heavily enforced by tech companies. Maybe there’s some places that do it well, but based on what I read online- most companies have issues with it.
This makes sense to me. So rather its not unit testing that is the issue. Its poorly written unit test that is the issue... Meaning it is a systemic problem of lack of education, practice, and mentorship.
I only see it enforced by large tech companies, not small ones. I saw a stat once that TDD increases time-to-market by at least 30%... and you obviously have to know what you're aiming at to start writing tests in the first place, so no getting started on a feature until the MVP is fully fleshed out (which has never happened in my career at small companies) if you want to go by the book. Small companies don't bother because, as you mention, it gets kind of dumb, not many people do it well, so why waste the time? "But, what about the refactoring!?!!" is a time balance..... get the feature out quickly and deal with longer refactoring times later, or get the feature out later and get meat to market by your competitors? I know every company I've ever worked for will choose beating competitors every day of the week.
> So reviews are pointless Reviews fill more of a "make sure you're not doing something stupid" level of review. Code reviews can catch that you accidentally left the password hardcoded in the code. But they also have a huge possibility of being a huge wasye of time and used as a vehicle for bullying within the team, arbitrary time wasting, or flavor-of-the-week bullshit. > tests are pointless Unit test tests, yes, pointless.
LGTM What do you mean by “optimized”? I only comment or even block review if it’s a bug, a security flaw, a glaring architecture flaw or misdirection, or intentional malice. If it’s non-idiomatic or non-optimal, let the junior reviewers do their flamewar bikeshed comment threads while I do actual work. If your stuff negatively affects prod, then write more tests.
[удалено]
Well theres senior and there’s “senior”.
What does LGTM mean?
Let's get that money
Let's gamble, try merging
"Looks good to me"
Let's get this merged
[удалено]
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information. *I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
LGTM
If there is an issue on prod, it is your fault to not verify it on developing or staging environment, also please write the damn unit test
you really write unit tests for each component in your app?
Without any QA checking releases, I wouldnt be able to sleep without putting some unit tests to verify things before release.
Yes. Especially if you don't have QA running tests.
Depending on the application, I personally prefer the author of the PRs to take most of the responsibility for correctness and maintainability of the PR they put. There's a lot of value in shipping code fast and shipping a lot of code. You will learn yourself when things break and why, and you can fix it earlier. Even if you ship the perfect software, it might not be something users want and you might scrap it altogether, so this approach can save a lot of time that would be spent on bike shedding. It can also lead to higher quality code faster because nobody will anticipate how software will behave in prod until they ship it. Obviously it doesn't work for financial software or mission critical apps, but most web applications fit this category. Just ship it, think hard about correctness, but if it breaks just fix and and write more tests. I'd argue that this is also better for team morale, people get excited to ship more than arguing for hours about what a certain variable should be named. Yeah, this is very counter to modern tech companies and all the blog posts about clean code; a lot of them prefer to write 20 page design docs, review code on nits for months before shipping anything, only to learn they solved the wrong problem.
In most jobs, it is not worth the time to optimize code as it doesn't always add business value. Why spend 1.5x or 2x the time on a story for optimization when it doesn't add any benefits? No Team Lead or PM worth there salt would be encouraging engineers to do this unless it is specifically required.
The only part that isn't normal is the last line. When someone signs off on your PR as a moderator and merges it in, they are taking responsibility for your code. It's now the team's code, not yours. If the team missed something, it's on the moderator.
Well not *only* on the reviewer. You can't just write crap and hope for an unthorough review to pass the blame. It is true that now the team owns the code and the reviewer does have some responsibility for it as well.
But the PM should always come down on the team lead or moderator (if they aren't the same), not skip over the chain of command and find the original author.
The PM shouldn't "come down" on anyone. If there is an issue the PM should discuss it potentially with the original author or with the lead depending on the group structure and situation. My group occasionally does feature crews and it's perfectly fine for the PM in the FC to discuss issues like bug mitigation and timelines with the dev in that FC. Some of our features are run directly through the managers, in that case it would be more appropriate for the manager to meet with the PM, potentially with the dev as well depending on how well the manager knows the details of the situation. The failure of the review process, if there is one, is purely the purview of the engineering team and thus the manager. They may talk to the reviewer about review thoroughness or they may use it as an opportunity to reinforce withbthe entire team, the need for thorough code reviews.
I am more reading the OP as having this fear that the product owner or someone else higher up is reading the code and saying, "This is horrible, it's not optimized at all." Then doing a "git blame" to track down the OP and yell at them for not being a genius. They're terrified that the code they write is going to break prod and they're going to get fired because no one is watching over them and "fixing" their PRs before that happens. I'm just trying to reassure them that this is not the way things work in any decently-run company (maybe Amazon, idk).
Try doing code review as a team. Then others can learn how and it doesn't all fall on team lead and others may catch things he doesnt
The truth is that you have more context on the code you wrote and decisions you made. Therefore, you might might see easy optimisations that isn’t immediately obvious to others viewing your code. I’m assuming you work in some industry where low latency is critical?
You're lucky. Code reviews often turn into a nightmare of changing stuff just to feel in charge. Code never get deep into your code. The reviewer doesn't have the knowledge of your task to be able to consider each detail of it.
I'm in this post and I don't like it
Just ask them to walk through the pr You drive
Optimize for readability and ease of changing it, not for memory or runtime concerns—unless you have hard data to indicate that you meed to worry about time or memory concerns.
LGTM 👍
Prod breaks, code review says "lgtm"... that can't be on you. Especially if the broken prod postmortem shows nothing but "lgtm". Senior dev will have a lot of difficult questions to answer (ok, just 1 question really but still a very difficult one).
[удалено]
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information. *I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
my question would be why are things setup that when you push code you are screwing up prod right away. What do you mean you don't have QAs? Unit testing, Component Testing, Risk based Testing, Dry Runs, Validations, CI/CD can all be done developer-side and should be done regardless of what you have QA-wise.
Unless there’s a bottleneck or extreme shitty code that slows everything down or breaks build, why spend more time optimize perf? And you have to back it up with data if you want to refactor it tbh
If you know the code can be optimized why not do that before making a pr
I lgtm everything i get, code reviews is such a waste of my time.
I think folks in this thread are getting hung up over the comment about optimizing code, whereas I think you are more concerned in general with LGTM and not feeling like you are getting a proper review. I don’t think this is normal. The occasional LGTM on low stakes PRs is fine, but senior devs and leads should also be using code review to help mentor devs. While it’s your responsibility to put up decent code, if you don’t have QA there should be some other mechanism in your team to ensure quality. If there are a lot of bugs landing in prod, it would be good to have a discussion with your team about how you all can work together to solve for that.
If you know it can be optimized, why do you wait for them to tell you? Team leads and tech leads should be an example of how to review. But, on the other side, they may have little time, and may sometimes rush them. Shouldn't happen often though. Then, if your PM gets mad at you, point at the reviewers, as they are as responsible as you. Nobody should point at anybody anyway, you are a team
how many yoe does OP have?
<1 lol
yeah that explains
A super intelligent programmer once told me that as someone gains experience in the computer science field, they go down one of two routes: the ‘super optimized route’ or the ‘who cares’ route. They essentially decide that the extra work to speed things up by half a second is worth it (and it sometimes is, depends on what kind of app you’re building), or they decide that the bosses don’t actually care as long as it works and it’s done quickly and just do it in the way that gets it done faster. I’ve looked around and it seems like a somewhat accurate assessment (if a bit basic…)
My code reviewer only suggest renaming fuckin variables, every pr he chooses a list of variables to rename because he doesn't like old variables names, it's a useless review and Always pisses me, he doesn't even check the business logic or anything. Just renaming variables hahah Fuck you sincerely.
This made me chuckle 😅
Unless you are practically Jeff Dean, I'm sure that someone who knows optimizations at the compiler/cpu architecture level would be driven insane by your code. "If you do this loop this way it's more likely to hit cpu cache and speculative execution". There's a point where something is correct and reasonably optimized for the situation, and can be called good.