Code Review Q&A and Videos

How can I handle a situation where I am frequently asked to review Pull Requests for our large monolithic application that powers web, mobile, and platform services, but some of these requests involve over 2000 lines of code and 53 modified JavaScript files from teams outside my own, and I lack context on the new features being added?

Despite declining these requests and suggesting alternative solutions, such as recommending another engineer with more familiarity with the feature, the web team still approves these large Pull Requests. How can I effectively communicate my concerns about the code quality and potential issues or bad coding practices within these large PRs, and what steps can I take to ensure that the appropriate team members are reviewing and approving them?

Context about me & external team: I'm a senior engineer and one of the code owners. I'm responsible for backend changes for API, but I am often asked to review Pull Requests coming from different changes also for Web (React, TypeScript). I noticed a lot of Pull Request coming to me since our codebase is 1 large monolithic application that powers web, mobile, and backend platform services, and it is stored in 1 GitHub Repro.

The external team are people who I do not work directly with and they are new to our codebase, but their team is pushing out new features as new major initiative from another Team and the stake are high for their project to be roll out. Lately, there's multiple bugs introduced from the external team, and it causes Production issues. And I'm getting buy-in from my Directors that we need to lay down some rules for changes from the external team.

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?

Over the past year, we've focused on developing our product quickly, which has led to a significant amount of technical debt in our codebase. As a result, we've shipped applications without proper architecture and planning. However, when I suggest refactoring or re-architecting to my team, many of the senior developers and the engineering manager are skeptical. In my opinion, I am not just offering refactoring for just sake of refactoring; it's necessary because our current code base lacks abstraction and separation of concerns and has slow development times.

Examples:

• Adding a new column to the database takes 1-2 days
• Or changing some code breaks another part of the application(lack of separation of concerns and abstraction)
• Another example, recently I introduced integration tests to our team because we manually tested a lot of stuff on staging before deployment, in that case, my EM's argument was integration tests are very slow to run on every build.

In those examples, how can I reduce skepticism in my team and bring more productivity, and more good code quality, and best practices to my team?

Since the engineer implementing the task went in a different direction from what I had in mind, now I feel like I have to dive into the code and explain the approaches I wrote, and also get involved in the code much more than I would like. How can I overcome this?

Since our company is so small (< 10 engineers), we do code reviews across platforms – so I review iOS and web code even though I’m an Android engineer.

I haven’t gotten any negative feedback on my code review yet, but I want to make sure I’m doing it properly. How can I suggest improvements on other engineer’s code, even if they’re on a different platform, or if they’re more senior than me?

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 work will often times span across multiple services within Uber, which means that I need to write code in codebases outside of my team's. Sometimes when I do this, it takes a long time for my commit to land as there's many rounds of follow-up questions, leading to a lot of rebases, revisions, and time spent on my end. I understand that they're trying to be the best stewards of their codebase, but this can be a bit frustrating sometimes. How can I make this process smoother and land these kinds of changes more quickly?

I know that launching a product with bugs shows a reactive approach. Similarly, if a reviewer catches an issue you missed, that’s also reactive.

But what if you’re thorough in writing and testing your code locally, and you catch a missed dependency before even raising a PR? You fix it yourself without anyone else noticing. Does this reflect positively on you for solving it on your own, or does it still look bad because it was missed initially?

Taking it a step further, say you forget a simple null check, causing errors in your unit tests. You catch this yourself and fix it, but technically, you were still being reactive.

So, how do people realistically measure “reactive” vs. “proactive”? And how are scenarios viewed where you catch and resolve issues on your own while writing code?

Hi, I've been stuck with my current project and am hoping someone can give me a lead here.

I have different actions which need to be run depending on a variable.

Sometimes, I might have to do an action more than one time in the same "if/else" section, e.g. calling actionX() twice (X->X), or different actions in a sequence: ActionX()->`ActionY()`->`ActionZ()`.

The best approach I have come up with so far is like so:

create a sequence, sequence, which holds all the actions which need to be ran inside of it. Then use a for loop to determine which function to call.

while status is less than 2:
user_input = get_user_input()

processed_input = process_input(user_input)

action_sequence = []



# Determine which sequence to execute based on processed_input

if processed_input is 1:

    action_sequence = ["X", "X"]  # Example: X->X sequence

elseif processed_input is 2:

    action_sequence = ["X", "Y", "Z"]  # Example: X->Y->Z sequence

endif



# Execute the sequence of actions

foreach action in action_sequence:

    if action is "X":

        actionX(processed_input)

    elseif action is "Y":

        actionY(processed_input)

    elseif action is "Z":

        actionZ(processed_input)

    endif

endfor
endwhile

Unfortunately this approach is going to become troublesome real quick.

1. What is this sort of topic called? Claude is saying System Design or Control Flow, I'm thinking it's a turing machine of some sort but don't know the software design term.
2. Is there a better way to do this? For example:
   1. What if the actions need to be modified or cancelled? e.g. For the chain X->Y->Z, Action X runs and the loop goes to the next iteration. After Action Y runs it then decides Action X needs to be ran again instead of Z.
   2. The loop runs and then ActionY errors out. I need some way of knowing which actions were successful, which ones errored, which ones failed etc. Or even a count on how many actions took place in the sequence. Perhaps, if it errors, then the chain is dumped and actionE runs instead or something like that.

Sorry if it's a long post, this has been stalling my progress for quite a while now and I just don't know the best way to approach this.

I'm a Data Engineer at Instacart. Every day, I get a lot of emails relating to PRs that are up for review or have been reviewed and are being merged. I'd estimate ~30, but prob as high as 50 some days.

The emails can come from my anyone on my team (~10 other people) or department (probably ~30 other people).

My strategy for dealing with these until now has been to only look at and review code that comes from 4 other individuals. These 4 people are the ones who I work most closely with. I asked my manager about doing code reviews for anyone else and he said to just stick to the core 4 other people I work with (including him), which makes sense given that I have plenty of work to do and those 4 other people produce a fair number of code reviews to do.

My question is, is there any value in me receiving emails for all those other people in my department who's code I don't care about (at least for now)? Should I try and either unsubscribe from or auto-archive their emails so I don't have to delete/archive those emails every time I access my inbox?

Just wondering what people's strategies are for this.

Thanks!