As a Software Developer working on a product squad, one of my daily responsibilities is to review other developers code. But, when this code change is too big, to review it can be a torture. Usually, you don't know where to begin and it takes too much time to understand the impact of the changes that your colleague is doing.
Here are some tips that I use to make my Pull Requests more "reviewable".
What is an extensive Pull Request?
It makes no sense to talk about splitting a Pull Request if it is small and understandable. But, when the Pull Request have more than 200 lines of changed code, you can start thinking about splitting it.
Another metric that I follow is the number of files affected by the change. If there are more than 5 changed files, it could be a sign that you should split your change into small pieces.
Beware!
These 200 lines of code can't include tests because tests can be as extensive as necessary, and it is ok. Tests usually have a lot of lines just to setup the test environment.
describe MyClass do
describe '#method' do
describe 'when you have to write 8 lines of code just to make your test understandable'
it 'considerably raises the Pull Request changed lines of code'
...
end
end
end
end
To classify a Pull Request as extensive, you have to consider only the changes that you made on the code that will run.
Tip #1: Split your Pull Request into understandable pieces
Any Pull Request must have a goal. Sometimes it can have multiples goals, and that is where the problem is.
Last week I had to work on a project that I never touched before. When I started working, I realized that the project was not ready to run on a dockerized environment. So, I had to create some docker related files (Dockerfile, docker-compose.yml, ...). It was nothing related to my initial goal, so I opened a Pull Request only to setup the docker environment. In this case, the goal is simple to understand for anyone who will be reviewing.
Another example is to setup or change linter rules. Sometimes there is a rule that never affected you, but in that specific change, it started affecting and you have to deal with it. But, to deal with it can mean that you will have to change a bunch of code in random files that are not related to the initial goal of your Pull Request. It is a smart move to open another Pull Request for these linter changes. Also, this example is perfect for the next tip.
Tip #2: Look for changes on unrelated files
When an extensive Pull Request have changes on files that are not directly related to your initial goal, it is time to ask yourself if it could be another Pull Request.
Usually this happens when you have to refactor one piece of code that affects multiple unrelated files. You could just do the refactor first and then apply the necessary change to achieve your goal. Again, here you transform 2 goals into 2 Pull Requests.
Tip #3: Think about what you could merge that would not affect any current behavior
To refactor is the usual case of change that does not affect any current behavior. Sometimes you just find an opportunity to optimize a query or to replace some code that is not in the best place. I am not going to tell you that "You must not change working code" because I believe that every developer wants to change working code when they realize an opportunity to make that code better.
If an opportunity appears, you can:
- Change the code if it is a small change;
- Put the change on your to-do list and work on it right after your current work;
- Create a Technical Debt to work on it later if you don't have time to spend on this type of work.
Tip #4: Sometimes you just realize that you can split that big change into small pieces later, and it is ok.
I usually start working without thinking if I could split the major task goal into small achievable goals. Then, when I achieve the major goal, I realize that it could be splitted.
Then, I list all small goals I think that makes sense, open a fresh new branch from the main branch and start working on it. After finishing the first small goal, I open another branch, now from the branch that I was working on, and develop to achieve the second small goal.
After finishing all small goals, it is important to tell reviewers that every small goal is a part of a bigger goal, especially if they have an order to be respected.
Conclusion
One of the most satisfying things to do at work is to review a code that you don't have any problem to point out and you understand every single change that has been done. If you create this experience for your colleagues, I am sure that you will be influencing them to also do code changes in small pieces.
Nobody likes to waste time on a Code Review. Don't be that developer.
Cover Photo by Markus Spiske on Unsplash
Git branches photo by Microsoft Docs
Thanks to Jéssica Veng for reviewing the text.
Latest comments (1)
Nice Post Alyson! It's very good to split changes in multiple pull requests.
I could add some things that helps me a lot
in my daily life when I need to split and organize my pull requests.
They're not evenly exclusive to Pull requests and their segregation, but should help you with your commit's history readability:
1- Make very small commits (one for concept)
PS: To do It, one thing I did was a CLI bash script which prevents me to use "git add ."
This forces me to see what I'm changing and not "git add ." "by default". We tend to do It automatically without realizing it and it's the main reason of big commits and pull requests.
2- Sometimes I also use the "interactive rebase Tool" (extra tip/advice: Stop right now to do merges, and instead start doing rebases and your life will change a lot!) and re-organize the order of my commits, change it's name, group some commits, mark them with tags and discard those I've changed foward and those ones that don't make sense to me anymore.
3- Sometimes I make a brand new branch, following an old commit or the master's newest commit and apply some cherrypicks on there from the branch I was developing in, seconds ago, (The One of advice number 2).
It is necessary when I want to split them up like you explained above; tho, Se can mix up the both techniques and have well structured PRs and a nice commit history line.
I thank you again for the article. It is extremally important to discuss this kind of Theme.
Bye bye, gratz for your first technical article.
See ya!