Code Review Q&A and Videos

Hi guys, I found an interesting point in another thread and I don't know, what should be a correct response to it.

Here is it:

"You spent 12 hours of work on an issue and got a working solution. You submit a PR with only a day left before the sprint ends. One of your coworkers looks at your code, thinks it's good enough, but suggests a better solution you haven't thought of. What do you do?"

Especially, what if I have a bigger feature, spent one week for it and need another 3 days, if I will change my code. What would you do?

When working on a new task, I often find myself asked to estimate how long a task may take. Luckily, my team is pretty forgiving, but a critical step will be to start more accurately estimating tasks.

How do you get better at breaking down tasks to understand what needs to get done, and then giving proper estimations for how long those tasks will take?

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.

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

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.

I'm sure Alex and Rahul talk about this in their , but wondering how to write clean code, particularly in the context of someone who doesn't have technical people available to review my pull requests in my work or in side-projects.

I'm sure this skill is almost entirely developed by working with talented people and writing a lot of code, but is there a place for resources like "Clean Code" the book or courses on Udemy?

I am working on my P0 goals for this half however, the XFN is not communicating well. I am done with my changes and blocked on them for days. They don't review code as fast as I need and keep me waiting. How should I approach them? Also, I want to build good relationship with them for a good Performance cycle feedback. I think they don't take me seriously. what is the right approach here?

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?

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.

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?

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 ?

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

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 😬

Often we talk about how, when getting feedback during a PR or Design Review, there are mistakes that I have seen, at least in my case, creep up repeatedly. Any tips to ensure that the learnings stick?

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?

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.