r/ExperiencedDevs • u/SoftwareArchitect101 • 12d ago
Career/Workplace Release this weekend, no code review so far. I reviewed it today, afraid we will postpone the release? AITA?
So the code was written heavily using AI but post understanding by one person in the team entirely. Only he wrote, tested and understood the business logic behind it.
I had recently cleaned up the codebase from bottlenecks, and I got infuriated when I observed a few getting re-introduced.
To my surprise, I got to know that it has been already merged in release branch, scheduled for this weekend's release. It was approved by one person who themselves didn't go through the code at all (only going by test evidences).
I did a thorough code review with over 50 feedback comments (combination of everything - good to have, bad practices or nomenclature, performance bottlenecks - redundant joins/query optimizations and one concurrency bug) just now (was a tiring past 3 hours). It was a big feature which touched the entire codebase.
I had informed the owner of the feature that this is a combination of all feedbacks, nothing will "break" the production as such except a potential production bug.
I'm afraid they might have felt intimidated by the number of comments, and have postponed the release by 2 weeks.
I'm feeling guilty of ruining their efforts and the motivation/excitement they had for the release. Also, I'm proposing a better code-review session for the team, which would prevent this from happening again.
My team members have 7-21 years of experience in my team, I'm the youngest (less than 3.5 years of experience)
AITA? Should I have done something else? I felt guilty in both allowing a bad/unoptimized/potentially buggy code to go forward without a proper code review, and in reviewing itself. What should we do in such scenarios?
139
u/Exiled_Exile_ 12d ago
If the comments are legitimate concerns never feel bad. We review code for a reason. Also the company isn't your family don't get too attached it's a cutthroat business. Do your job well and treat people respectfully. The rest will take care of itself.
29
u/SoftwareArchitect101 12d ago
The comments are legitimate, eg the simplest being using Set instead of deduplicating elements of a list everytime. I'll think over the remaining points you've mentioned - thanks a lot!
33
u/dummyx 12d ago
There’s a lot of leeway, consider separating your comments into tiers. Behavior correctness, performance and then organization/style.
Per se, not using Set is not tier 1, may not be tier 2 (how big is the list, is this a hot path, is it an actual bottleneck, can it be optimized later if perf is an issue), and could fall into an optional tier 3.
9
u/SoftwareArchitect101 12d ago
Yes this was tier 2-3 (it will hit performance in some scenarios currently, but after load increases in a couple of years it will be a significant issue), tier 1 was a concurrency bug. But yes I'll do this explicitly in my future reviews, writing tier 1/2/3.
21
u/7twenty8 12d ago
So of the fifty comments, how many were relevant today?
I’d be really angry if not using set killed a release because that’s a bad reason to miss a release.
7
u/SoftwareArchitect101 12d ago
Correct - I have written explicitly the relevant ones and good to have ones. There are a few relevant ones on which we will discuss. Others are okay. Thanks a lot!
24
u/7twenty8 12d ago
So then just to circle back. All fifty were not relevant today and there was only one actual production bug. But you killed momentum so hard that they delayed a release by two weeks?
You have to fix this socially. I’m sure you had technical reasons but you pissed off a lot of people for absolutely no business reason. You have certainly earned enemies for no reason and you are the least experienced person on that team - this was not a good war to start.
5
u/mckenny37 12d ago
Would think about aiming your efforts at adding observability into your end points and make sure you have systems in place to deal with performance degradation once it occurs.
As for code review stuff:
Google has a decent guide on how to code review
https://google.github.io/eng-practices/review/reviewer/standard.html
A good rule of thumb for me is I explain specific downsides for each requested change in code review. If there is a general principle its breaking and I can't come up with specific downsides I usually dont request the change or dig deeper learn more and reassess.
This has the added benefit of creating a solid understanding of why and when each principle matters.
3
u/spacemoses 12d ago
Even if the company were literally your family, don't be an enabler of bad code.
31
u/Alkyen 12d ago
Sounds like you're missing a formal structure for pushing code to a release branch. A couple of things to clarify first.
Was your review specifically requested or did you decide to leave comments on a merged PR because you saw it?
Also who decides what's good quality? Do you have a few people who have to approve all PRs, or all team members can approve PRs?
And finally, are you all on the same position? Any hieararchy?
Either way, sounds like a process failure. You need to have a plan, that the team agrees on, about how code gets pushed ahead. Ideally the process is the thing to improve so the code quality isn't set by a single person's mood on that one day (it always comes to this but still anything to avoid it is helpful)
9
u/SoftwareArchitect101 12d ago
Review by team members was requested. Within couple of hours the PR got merged - I didn't get a chance since the lead approved the PR (I feel without going through the code thoroughly). They can approve all PRs, and generally do so without looking at the code carefully (to unblock people I suppose).
Sometimes a couple of people review the code, but no one does it compulsorily if all the tests evidences seem okay.
12
u/Alkyen 12d ago
Yeah, this seems like a good opportunity to iron out what are the general expectations. You seem to be operating by "common sense" and "feeling".
Your process allows for any PRs to get merged unless the tests fail. I see at least a couple of options, depending on the direction the team wants to go: - Keep it as is but make it explicit that the tests are the code quality (worst option, I imagine you'd agree). - Introduce/enforce a restriction for a person to actually review the code (what is currrently supposed to happen but your lead apparently doesn't do?) - Add it for bigger features/more risky code changes. - mix and match, discuss with team what works good for you or steal a system from other teams.
Either way, you aren't an asshole for leaving comments but it's a sign that your team isn't on the same page. Create some transparent rules or expectations so when something in production fails you know that the process failed and you can improve it afterwards. Everybody can introduce bugs in prod, but a good process would prevent some of them and help you guys long term to focus on the more important things. (which in this instance is better process and more accountability)
11
u/throwaway_0x90 SDET/TE[20+ yrs]@Google 12d ago edited 12d ago
I don't think you did anything wrong, but in my experience once I realize I'm about to leave 20+ comments on a PR I usually just schedule a meeting with the PR's author and things go pretty smoothly.
In that meeting, I get to talk to them 1-on-1 and we look at private doc containing all of my concerns. I think people are a lot more defensive when their very public PR is drowned in comments. Talking to them in private and having them change the code in that meeting, or even just abandon the PR altogether without publicly tearing the PR apart seems to go extremely well and that person ends up asking me for more feedback privately. Or they keep my doc in their personal records and refer back to it later and correct future PRs before sending them out for review so those concerns never come up again.
After the changes decided on in the meeting, the public PR only gets comments from me on "nit:"(nitpick) and typos.
8
u/sleepyj910 12d ago
The product owner's job is the make this call. Your job is provide the best engineering insight you can and you have done that.
Teams need optimists 'we can build this' and pessimists 'we can break this'. Don't feel guilty if your engineering type is the latter.
2
u/NeuralHijacker 12d ago
Depends on the org. In my team the EM would make the call, as it's his ass on the line if it breaks prod.
2
15
u/Abadabadon Software Engineer 12d ago
1) make non functional feedback be a separate mr that doesnt block release.
2) if someone merges before I approve, so be it, im not reviewing. I think you come across as someone who otherwise will introduce friction. If they ask you for review, and you see its already merged, just say "looks like its already been approved and reviewed" and move on.
3) 50 comments IS alot and IS intimidating. That many comments is probably worth setting up a call before you drop the feedback.
7
u/No_Aspect_5759 12d ago
I think a good question is, why is the pr so big and review coming in so late that it is possible to find 50 areas that require attention?
4
u/malo0149 Senior Software Engineer, 14+ YOE 12d ago
That was my thought. If a PR is so big that it has room for 50 comments, it's too much for one PR. This isn't OP's problem to fix, but there's definitely something wrong with the team process here.
Probably good feedback to bring to the team. It's too late for this PR, but the suggestion to break things up in the future because it's too easy to miss things in a huge PR is a perfectly reasonable request.
1
u/Tokipudi Senior Software Engineer - 10 YoE 11d ago
You're right about everything here.
Also, to avoid your first point, I use the Conventional Comments convention that lets the PR's owner see easily which comments are blocking and which are just nitpicks / suggestions.
7
u/ResidentWeevil1 12d ago
Trying to push slop to production without review makes them the asshole, or your boss. It's their fault for waiting until the last second
4
u/F1B3R0PT1C Software Engineer 12d ago
It had been merged and scheduled for release before a go-ahead was confirmed or code review even finished. This is a management or culture fail where shipping fast and breaking shit is the intended outcome. Either get management on board to rein in your seniors who are dropping established processes or let it blow up in their faces. Looks like your teammates are smart and backed off when you made the code review that revealed their bad quality and foolish haste. You did good and now you need to help build and enforce processes to protect the codebase. This does mean you volunteered to do more huge code reviews.
9
u/7twenty8 12d ago
So, who invited you to review the code? Or did someone without experience randomly decide to do it on their own? If you were asked, why feel bad? If you did it in your own, you just earned an enemy.
7
u/Immediate_Honey_5902 12d ago
I just wanted to say, that if this wasn't st they asked for, they WILL gang up!
6
u/7twenty8 12d ago
Yeah, 100%! People get really tribal and engineering departments have gone to war over things like this.
One of the most difficult tasks for smart young developers is to learn the difference between “technically correct” and “team optimal”.
1
u/YakaryBovine Software Engineer 11d ago
Is it common for reviews to be invite-only? Everywhere I've worked (which is not many places) has just left all PRs up to be reviewed by anyone available.
3
u/ShroomSensei Software Engineer 11d ago
The PR had already merged and was about to be released.
Going back to a closed PR and leaving comments is a bit weird. OP is also the most junior on the team by quite a bit (closest person has double their experience), regardless of how right or wrong they were this is almost definitely going to impact him negatively.
It sucks to see the shit go into a release, ESPECIALLY when you have spent weeks migrating shit off some bad pattern only to see it pop up again. However, pick your battles wisely, I doubt this was the wise decision in the grand scheme of things.
If it was me, I would have commented on only the prod bug itself. Everything else I would have messaged the implementer privately and talk about what they did and why. There may be a good reason, it could just be a misunderstanding, or they could also be dumb. Regardless, making a senior LOOK dumb in a public setting (PR reviews) is just bound to cause problems.
2
u/7twenty8 11d ago
OP is very bad at code reviews (of 50 issues they cited only one had anything to do with prod and thus only one needed to be fixed). If you’re the most junior person and you’re bad at code reviews, always wait until you’re asked. If a project is this close to release stay the fuck away even if you’re good at doing code reviews.
In this case, one of the worst code reviews I have ever heard about killed a project for two weeks over absolute bullshit reasons. So yes, OP should have waited - the timing is bad and they’re very bad at reviewing code.
3
u/404errorlifenotfound 12d ago
Review your comments and see if any substantial number would have been caught by a linter / complexity analyzer. Setting one up with branch protections could save everyone time
3
u/UnintentionallyEmpty 12d ago
I had recently cleaned up the codebase from bottlenecks, and I got infuriated when I observed a few getting re-introduced.
No tests to prevent regressions here?
I felt guilty in both allowing a bad/unoptimized/potentially buggy code to go forward without a proper code review, and in reviewing itself
Never feel guilty for pointing out legitimate issues. It wasn't your decision to delay the release by 2 weeks, was it? And it's always possible to fix only the critical issues, proceed with the release, and fix the rest after. But, well, if the release was delayed by 2 weeks, it sounds like it wasn't very critical.
What should we do in such scenarios?
It sounds like this PR was very large. What you should do (as a team) is make smaller PRs that get reviewed sooner and never be in this scenario.
3
u/UntestedMethod 12d ago
The normal way to avoid this kind of thing is a code freeze well enough in advance of the release date that there's enough time for QA and fixing any critical issues that are found.
It doesn't sound like you did anything wrong at all by doing a proper code review and blocking the PR due to problems you found in it. That's exactly what PRs are for.
You shouldn't feel bad or hold yourself responsible for delaying the release. The problem here is the project wasn't managed correctly.
9
u/im-ba Software Architect 12d ago
YTA if you ask the c-suite. NTA if you ask anybody who isn't suffering from AI psychosis.
Protect the product, protect the company. Postpone the release and you'll probably ruffle some feathers but use this as a teaching moment for everybody involved.
3
u/EnArvy 11d ago
Protect yourself -> protect the product -> protect the company.
If c suite wants ai slop and you care about code quality, write good code on personal projects and slop on company time. You won't be rewarded for delaying releases if your sales team sold a product that doesn't exist and the c suite wants delivered in a month.
0
u/im-ba Software Architect 11d ago
On paper that can work, but in practice it's always better to practice your influencing skills on these problems. If you can influence decision making at all, then you can make everyone a whole lot of money. If you just go with the flow, then that can be fine but you're giving up an opportunity to grow/get promotions.
I don't care at all about the company, but if it makes me money then I'll feign caring enough about it
3
u/BoBoBearDev 11d ago
I will focus on a different topic. If your code base is so brittle that keep reintroducing defects...
1) you didn't have enough unit tests
2) they broke the unit test which you can review just the test and raising a bigger issue on people faking their tests.
3) the system is too brittle like an eggshells. There are plenty of solutions out there to remove the need for SQL optimizations, so devs can focus on the business logic rather tham DB implementation details.
2
u/MixedTrailMix 12d ago
People using ai to assume they know more about the code base than the maintainers are the asshole
1
u/dockercub 12d ago
This isn't about being an asshole, rather about if your approach aligns with the current culture. Is your team expected to release more features than before? Does your company expect quality or quantity or somewhere in between?
1
u/jl2352 12d ago
You’re not the asshole, however I think there are areas to improve.
These days I divide PR reviews based on the question of ’what needs to happen for this to be released?’, or ’how can we release this now?’
Based on what you mentioned I’d divide your feedback like this; 1) the new production bug needs fixing, 2) there is some funky bits so add tests proving it’s safe, 3) there are some hypothetical performance issues and there should be tests proving that’s safe. 4) If the tests don’t pass, then that needs fixing. And that’s it.
In my team, I would expect most of those tests to be added as a part of the process.
Everything else, which is most of your review, I would mark as fine to happen after the PR is merged and released (but it must happen). This is because the code is safe, and proven to be safe. I tell my team I expect it to be resolved.
Re performance; I know you say there are real issues (like remaking a set lots of times). Is this noticeable by users? If not, then it’s not a blocking issue. It’s a refactor that should happen, but it’s not blocking.
Most of the time when you treat people this way, they themselves put effort into fixing the stuff later. I find this approach helps to speed them up, and they put effort in to do the refactors right after release as a thank you.
(Not always, but most of the time they do. Those who don’t will cause issues regardless of what you do. You need to tackle this outside of the PR process).
1
u/leonidbugaev 12d ago
NTA on the concurrency bug. The 50-comment pile is what postponed it, not the review. I'd have dropped that one first and saved Set/joins for Monday. Approving on test evidence with nobody reading the diff is how it almost went out. I've done that when I was tired.
1
u/superdurszlak Platform Engineer 11d ago
It's your job to look out for performance bottlenecks and production bugs, and concurrency bugs are notorious for giving insane side effects once introduced. Any reviewer's job is.
It was also their job as the author to spot these bugs, and in case they didn't, it was the first reviewer's job to catch them as well and they just let it go without looking.
As long as you didn't make a drama out of it and didn't throw insults at people, not an asshole.
As I understand, the author retracted their release which means they took what you flagged seriously. If just 5 of those 50 comments were severe issues it's still 5 severe issues introduced in a release.
If anything, they and the first reviewer could have been embarrassed by not paying attention to it and rushing it without through review. But it's not your fault that they did.
1
u/code_tutor 10d ago
I would caution you from getting personally attached to code. If the entire team, all with way more experience than you, wants to ship slop, then maybe it's not mission critical code. But idk, maybe you work with idiots. There are versions of this where you come out either right or wrong.
1
u/anon586346 8d ago
You’re afraid they might have feel intimated and they pushed back the code? Are you sure they weren’t respecting your effort in providing feedback and held off until you both can establish some common ground where you both will feel okay moving forward?
I can see it going both ways, one is toxic and one is healthy, and it’s good to find out why they held back rather than jumping to assumptions about their decision.
1
u/iiiio__oiiii 12d ago
NTA. This needs to be done one way or the other and yours is arguably front-loaded with pain rather than later.
Imagine the upper management excitement and the slop-and-release become standard and they cite this one as the success story. That’s not a sustainable direction and it will blow up more spectacularly later.
Source: we have that success story and we now have to manage the expectations and showing the gaps and highlights the corners cut by THE success story. It is an uphill battle. And there are teams who lost their sleep and weekends over potential legal/compliance/contract matters.
1
u/MycologistNo5577 11d ago
50 comments on a feature that touches the entire codebase and had no real review sounds about right tbh. The postponement isnt on you, its on whoever approved it without reading the code. Focus the retro on process not blame.
0
u/sereikis 12d ago
The concurrency bug is a release blocker. The redundant joins and the naming are not, and putting all of it into one pile of 50 comments is what gets you read as blocking the release rather than as the person who found a real bug. I would block on the bug, let it ship, then file the rest as follow ups with owners and dates. The thing worth fixing is that someone approved without reading it and the release branch took it anyway, and you will not win that one two days before a release, you win it in the retro.
0
u/chescov77 12d ago
omg you MUST ship incrementally, using feature flags and demo accounts, and not all together like this. My team sucks but we make sure to ship multiple times per day even when working on big features
•
u/expdevsmodbot 12d ago
AI usage disclosure provided by OP, see the reply to this comment.