Code Review Q&A and Videos

Hi I have been watching on the taro courses.

I wanted ask is there a template that one can follow while writing test plan and how to map it for code reviews ?

Any example that can be shared to understand?

I have had issues with some of my teammates where code or docs don't get reviewed early and a ton of feedback is left the day before the deadline. This is after sending multiple reminders almost everyday leading up to the deadline and I don't like escalating because it feels like I am snitching on them. I do give reminders in our weekly standup and our common channel.

I got tired of this happening and setup a code bot to send reminders to everyone in the code channel and have considered sending out email reminders (nobody else really does this) or booking a meeting in advance before I even start coding. I tried a concept similar to RFC but I still don't get feedback in time. I get it people are busy but they just completely ignore me, I don't even get a slack sometimes saying they are busy.

I am an SDE 1 if that helps

Even though starting to work for a big company like Meta, Amazon, Google, etc. I believe is a hard to achieve (I haven't work for) somehow it looks pretty straightforward. Learn for interview, get the job, level up. Yes, I am sure it's hard and not many will do it but still you know what should be done (yes, may don't know how). But let me tell you a different story:

I work in a not that famous country in the EU and non of the top tech companies is there. Actually 90+% of the companies are outsourcing companies. As a SE with 10 years of experience in the outsourcing world I can tell you how it works: you work on a legacy code which is so old and so bad (hundreds of people have tried write code there) you can't see good practice at all, no code reviews (sometimes there is bad it is very rare), no unit tests, performance review is only about client's feedback and so on, you got the point. It's about the money only and nobody cares if you are good or not if the client is happy. In very rare cases I have started something from scratch but all of my colleagues were so bad progmmers like myself that we messed up all. It's a deadlock. After 10 years I realized I am a bad programmer and I've seen so many bad practices that I have no passion to do anything anymore. Now to the questions:

1. Is it possible to apply best standards in an outsourcing company like those in FAANG and if yes, how?
2. How can I fill all the gaps I have at the moment? Can I fill all the gaps with side projects only? How can I fill them when nobody will teach me anything new. Nowone will review my code and like @Alex said, they are the main source to learn :) How would I know is the code good or not? Could it be better?

The ultimate goal of my career (and maybe in life) is to fill the gap not only in my skills but to create a company (product based or outsourcing) where everyone who join to have a chance to become a great programmer. But before helping others, I need to help myslelf. This is how I found Taro.

hi,

I just started. I've never been at a company this big. I have worked at a 1000 person place before it shrunk (it grew from 3-500 while I was there) and 65 and 500 person shops.

How do you suggest I ramp up? It's so many people and so many layers of code and tech. I have a good amount of experience in data/ml and also eng. I don't think I'm a 10x engineer but I am pretty proficient at getting things done.

What should my plan be even with holiday slow down coming? People are super nice and I don't feel crazy pressure to ship something rn but I put a lot of pressure on myself and want to do well.

I also know code quality is even more important at bigger companies trying to deal with scale. How do I make sure I can keep up?

1. How do pull requests and pull request comments look like with an increasing level of seniority?
2. Are pull requests for senior engineers not expected to have any comments for improvements? If yes, then what kind of comments should be there?
3. Also, have seen some principal engineers not sharing their pull requests as they are quite high in seniority and merging it directly. Do you recommend that? If not, who should review PRs for very senior engineers if the team does not have engineers at that level?

I was hired to be an Android developer for a startup. Other engineers on my team bounce around back-end, web iOS and Android. I make sure to review every Android PR that comes in but at the same time I have strived to review code from other projects like back-end and iOS. My primary language is Kotlin so I can offer advice on that but since others are written in typescript/Swift I am not sure how to offer reviews although I want to. I mostly end up asking questions.

Currently, we have tech debt mainly discoverable via a custom annotation in the code.

Some examples are:

• Break complex features into separate manageable flows
• Split large classes or controllers
• Feature/part refactoring tickets with linked ticket numbers.

The core tech debt is easy to address and manageable via project boards, but the problem is the rest (product related) often gets out of date and never gets resolved, for which there are some reasons like:

• The product team has been disbanded.
• Feature no longer a priority for the product.
• The part of the code has not been touched for some time but could come into active development again.
• Find better implementation of X

I believe the product-related tech debt should be owned and tracked by the product team but often there are some tech debts left in favor of speed like - breaking complex screens, and classes, and find better approaches etc etc

What's the best way to promote ownership and address such tech debt, so that they are addressed during code reviews and not left in the code as comments indefinitely, nor as a ticket in some project board which never addressed?

This is a serious question involving a friend who met only 3 out of 5 expectations during his mid-year review. He was hired as an IC2 and is about to reach the one-year mark. His team allowed him to approve and merge his own code into production, which resulted in a crash of the application server and AWS environment. This constitutes his first mistake, and someone has contacted the VP in the organization. What potential consequences could he face?

When a colleague sends me a pretty large PR to review, I usually have no idea what’s going on. I know it’s not expected to pull their branch down but how do you usually go through it and add constructive feedback? I usually just wait until someone else has approved it (after their given feedback is incorporated) and then I hit approve too 😬

Context: Working in a small startup where there are a very small number of wiser senior engineers

Everyone in the development team contributes to code reviews

Often I’m unsure about the quality of the feedback and when to act upon it

Sometimes people even change their mind after you have acted on that feedback which is frustrating and can contribute to the delay of finishing a task

Does this also happen in big tech? How do you handle these situations ?

Hello everyone,

I joined Meta a couple of months ago as an IC5, coming from a predominantly Java background. Recently, I've started working with the PHP codebase, which is somewhat new to me. I've noticed that my diffs tend to undergo several rounds of revisions, which not only delays their integration but also consumes more engineering time than I'd like.

A fellow engineer pointed out that my diffs are often too lengthy, leading to extensive feedback and additional iterations. I'm actively looking for ways to streamline my review process and improve the quality of my submissions.

Here’s what I’m considering to reduce the review cycles and improve my coding approach:

1. Reducing the Size of Diffs
2. Pre-Review Checks: -- Any suggestions here would be great

I would greatly appreciate any advice on:

• Strategies or tools that could help in catching potential issues early.
• Best practices for structuring PHP code and managing larger changes in a PHP environment.
• Any insights on transitioning from Java to PHP, particularly with regard to common pitfalls or key differences I should be aware of.

Thanks in advance for your help! I'm eager to learn from your experiences and improve my contributions to our projects.

I am on a tight deadline to deliver an impactful and complex feature. Whenever I send my PR out for review, my lead takes at least 2-3 days to start reviewing the code. Then, there are either a lot of non actionable comments or feedback on tests. So I started sending out a test plan in advance which also receives a lot of feedback. When I turnaround with a refined PR, the lead again takes 2-3 days for a review. I am worried that there are too many iterations for a simple PR and I am chasing the lead for reviews. When I hinted about the review taking time, I got a response that the code shouldn’t have had so many flaws to begin with. The code doesn’t have bugs, he was nit picking on unit tests that could be refactored for better maintainability. I agree, but should that be a show stopper?

I had my code reviewed by a peer and received feedback that there are no blocking changes and the code is mergeable. For a complex feature, I expected 1-2 iterations but each iteration is draining and am starting to wonder if I am really that bad an engineer. The biggest problem is that the reviews happen in person and the reviewer gets nasty and yells at times. He is extremely rude and after a few minutes he tries to explain that all the feedback is for my own good like I'm a kid. I expect some amount of professionalism but I feel disrespected because of the attitude. Is it normal for 2-3 iterations for a slightly complex PR and should I have to chase reviewers each time? Do you any tips on handling the behavioral issues?

I understand the idea and the purpose behind an atomic diff but what would that look like in practice if we were using Github? My understanding about this is sort of vague.

I'll lay out two different approaches that I've been thinking about along with their pros/cons. Please let me know if I'm thinking about this correctly.

EDIT: Upon using GPT, I found an extremely useful resource from google's documentation that actually outlines this problem very well, and resolved my question (links below):

Tl;dr -- it's better to have smaller and more focused PRs (approach 2) since smaller PRs are easier to fix compared to larger ones. If there are merge conflicts when rebasing, the impact on subsequent PRs could be mitigated due to their smaller sizes. Other additional advantages may include (1) faster feedback from peers -- esp. if you're wasting time solving the wrong problem(s) or needing additional feedback for approach, or (2) allow for concurrent engineering while some PR(s) can enter their review cycle. One feature can be broken up into multiple atomic changes (PRs).

With this, I would recommend creating a thread of PRs linked together describing all of their changes made during the development cycle. Each PR should serve their atomic purpose, and all of their changes combined should contribute towards fulfilling some story description (which gives additional context to the reviewer).

General principle: reduce the amount of changes you would need to make for each PR by making them smaller. Otherwise, it may take longer for each PR to be reviewed and gain acceptance for submission, which could cause a chain effect on subsequent PRs as well.

Original Question:

Approach 1 (1 big PR with atomic commits):

Say we have a feature branch that we're working on. We keep each commit atomic. We provide a summary description of our PR, a test plan section, and a commit (diff) section in our PR outlining each commit hash and a short summary for each change (in bullet point fashion).

Pros/Cons:

The Files Changed section on github could be extremely long and contain 800-1000 lines of code although we could keep each commit atomic to control the complexity and make it somewhat easier to review with each commit containing 50-200 lines of code. Another pro is we keep 1 PR as opposed to having multiple prs but the con is changes might take longer to approve because of how many different commits there are to review...?

Approach 2 (Several PRs with atomic commits still)

Meanwhile, there's approach 2 which is to keep several PRs (each 50-250 lines of code) and a master PR that aggregates all of the smaller PRs to outline a story of all the changes that are necessary for some feature that we're working on. The pros is smaller PRs are easier to review and easier to merge. The con is we have to pay the cost of rebasing subsequent branches especially if there are stacked dependencies from one pr to another.

I'm not really sure what the "best approach" would be. Any thoughts?

References

At my company, I've experienced receiving a lot slower code reviews than usual. I think this is because I'm on a Platform team but each individual member is very specialized to their focus area. We're encouraged to tag a specific person (and I've been assigned a specific person for my code reviews) as well as the team for broader visibility.

However, I'm often stuck waiting 24-48 hours before receiving anything and always need to ping the person that has been assigned to my reviews. It felt pretty awkward needing to do that all the time so I brought it to my manager. My manager essentially let me know we don't have a good process around code reviews yet and are still figuring it out, and it could be a good opportunity for me to bring a good process to the table. He recommends pinging people in the meantime and to not view it as bothering. I am open to doing that, but still wondering if there is a better way.

I have 2 questions:

1. What could be a good way to propose a code review process to my team, especially given each individual is specialized to their own area within Platform?
2. In the meantime, what would be a good way to get faster code reviews from my assigned person who is intended to be reviewing them?

My direct supervisor/ tech lead has a tendency to leave a lot of nitpick comments on my and my teammate's PRs during code reviews. I know the intention is positive, but in my view it's excessive and leads our team to spend too much time addressing stylistic or minor changes that don't materially improve the codebase. Since we're building for an early-stage startup I also believe it's a higher priority to ship code that works well enough so the business can get customer feedback, rather than focus on subjective stylistic things. It also increases the noise level in every PR and makes it hard to identify and focus on any comments about significant things.

I raised these concerns directly with my supervisor and also asked for clarification about which nitpicks are actually optional or if I can opt out of implementing any. My supervisor said I need to address/respond to every single nitpick comment, which means if I disagree or don't want to implement the nit, I would have to explain why every single time, which I think is not an efficient use of time. They also said we don't have to implement everything they suggest, and they welcome pushback, but I don't think they realize it feels a bit harder to do that when they leave so many nits and they are in a position of authority and are not my peer. I shared a suggestion that we make it ok for the PR owner to opt out of addressing a nitpick / leave it up to them to decide, and also try not to focus continually on stylistic things that can't be automated by linting etc. This was ignored though.

Does anyone have any advice on how to handle this situation? It's very frustrating and exhausting sometimes, and part of me has tended to cave in and just implement every bit unless I have a really strong opinion against it for the sake of avoiding spending time debating too much. I'd like to be wise about picking my battles.

Basically, I feel like I have so many unnecessary ‘DUH! I can’t believe I missed that!’ moments in public -- mainly when asking for help and creating PR's.

For example, I recently was so focused on the more difficult part of a ticket, thinking about edge cases and trying to really polish this bit of functionality, that I spaced on reviewing the ticket before creating a PR. This meant the lead engineer reviewing my PR had to explain to me that I missed some other aspects of the ticket.

Another time, I spent a while trying to right a really good question about a solution to what I suspected was a tough issue. The problem? I hadn’t thought to test my hypothesis and confirm that this issue would occur (it didn’t) before formulating and asking my question. So I needn’t have bothered anyone else or myself about navigating this hypothetical problem.

When this happens, it makes me feel like my work isn’t thorough or to a high standard, not mention making me look bad. So it’s something I’d really like to improve, but it seems when I focus on being thorough it perpetuates the problem like in the two examples above.

I feel like I need some checklists/processes as guardrails for common scenarios (e.g., creating PR’s, asking for help) I can fall back on, because otherwise I feel my brain will keep missing things and making easily preventable mistakes.

I'm an E5 mobile engineer at a Big Tech company. Some engineers have a habit of leaving "drive-by comments" in code reviews that are only tangential to my PR. How should I push back on these?

Examples:

• I created some snapshot tests using our existing 3rd party snapshot library. An engineer commented that we should change where the snapshot library stores the snapshot images. I felt that this was out-of-scope for my PR, so I told them to ask our mobile infra team instead. They did not follow up with the mobile infra team. Should the expectation be for me to drive those conversations with the mobile infra team instead of them?
• An engineer requested that I abstract and mock how I log analytics events for my feature. I think there's little ROI because it's unlikely for that to change, but I did it anyway to appease them. When I later looked at that engineer's PRs, I discovered that they never abstract & mock how they log analytics events either! This seemed hypocritical and a waste of time due to the low ROI.