Code Review Q&A and Videos

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.

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 complained to my skip manager about my colleague's (their manager) code review standards, but ultimately discovered deficiencies in the quality of my own pull requests. As a result, I believe I have lost credibility. How can I regain trust and credibility in this situation?

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.

It's starting to take a toll on me and I'm not sure what to do at this point. I've tried my best for the last couple of months to elevate the entire team. I spend a lot of time writing thoughtful and educational comments when reviewing. I've emphasized the importance of atomic PRs that are small, but every single PR is about 500-1000+- and 10-20 files. In addition to that, lots of the code written is just erroneous or poorly written from a system design standpoint. I often find myself having to design the entire feature from scratch for them, or give them specific guidance.

How can I help my team improve in this area? I might start to encourage folks to create a design doc outlining shortly the work they're going to pursue, but I'm not sure if this will add too much friction and time consumption from needing reviews from others.

The most frustrating part is when I'm not requested as a reviewer, people will just do LGTM without much consideration, and then I'll find problematic code in our repo.

We're in the early stages development of a new frontend and backend.

We are using a linter, formatter and a compiler tool, so that's not a concern.

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?

I know to refactor my commits only based on previous review comments. I code and test it thoroughly most of the time, but it's hard for me to identify places to refactor. How to get good at refactoring or even identifying places to refactor?

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?

Hi,

I often hear from here that you should strive to land code as fast as possible to make a meaningful impact and get feedback from your users. I follow this mantra pretty closely as I see those story points as free baskets that make my team look good (because objectively, we're getting stuff done), but it seems many engineers don't subscribe to this philosophy. Sometimes, I see PRs that have been QA'd and approved linger around for days to weeks... It feels like a problem, but it also feels like I'm the only one who brings it up. What is the best course of action?

I have been seeking to develop habits / follow a system to increase code quality, both in terms of readability and number of functional issues. I have come up with a few ideas I have listed below but I am curious what other people have been trying out! Have any of the below worked for you? What specific steps do you take to ensure code quality?

1. More awareness while writing code: it's tempting to write a rough draft version of the code thinking you will polish it later but oftentimes it's easier to spot mistakes while your attention is on the erroneous line of code. Example of this in action: search for where the method you're modifying is used in the code base before making changes to it.
2. Do not submit the code for review immediately after writing the code. Give yourself a short break, come back and review your code with a fresh mind, which will make spotting bugs easier.
3. Write down the possible edge cases in a notebook before submitting the PR for review. For medium to large PRs, the process of writing itself can help you consider more edge cases than simply keeping them in your head.

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?

What’s the best way to handle a situation as a tech lead when you are under pressure to deliver your tasks and ensure that the other team members are also on track? If one developer slacks and doesn’t meet the deadline, is it automatically the lead's fault, especially if the situation was raised well in advance. If the person continues to under deliver with poorly written code, is it okay to spend time and pair program and get work done just to meet deadlines? Keep in mind that the developer is a junior engineer and requires fair bit of hand holding.

I had to approve a PR, although I was not 100% confident, it wasn’t a breaking code and it wouldn’t have caused any backward compatible issues. I received extremely harsh feedback for approving a sub standard from a peer/colleague. Had I not completed the PR, my manager would have provided feedback on the delayed project, what’s the best way to navigate between two leads or managers who are not aligned and expect different outcomes from the same situation?

Please help.

How do you keep up with everything that is happening on the team with different projects, doc designs and code reviews?