Code Review

Code Review

Code review ensures high quality software that is maintainable for the lifetime of the code

Code review is one of the most common and powerful ways for software engineers to learn. On top of increasing code quality, it builds engineering culture within a team.

How to handle the situation as a CodeOwner and push back Large Pull Request from External Team?

Anonymous User at Taro Community profile pic
Anonymous User at Taro Community

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.

Show more
85 Views
1 Like
3 Comments
6 months ago

How to apply One Diff, One Thesis via Github?

Anonymous User at Taro Community profile pic
Anonymous User at Taro Community

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

Show more
127 Views
4 Likes
1 Comment
7 months ago

How to become a top developer in outsourcing company?

Anonymous User at Taro Community profile pic
Anonymous User at Taro Community

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.

Show more
255 Views
3 Likes
7 Comments
8 months ago

A good code review process is needed to catch bugs, increase maintainability, and guarantee a high velocity future contributors.