Keep your codebase cleaner — Pt. II Merge requests
In the very first part of the series, I focused on setting up the image for Danger. I will now show you the general issues that we come across daily whilst developing any application. It doesn't matter if you're a mobile/web/frontend/backend developer. The following rules can be applied broadly to the development process.
Common issues we face
- Size of the merge request
- Complexity & lact of clarity (changes to numerous components at once)
- Giving not enough information on MR to reviewer
There are many researches been done but I'll pick a representative list made by IBM that refers to all of the aforementioned issues. Let's go through these listed issues, pick the dark side effects, and I'll show how Danger can help you.
Size of the merge request
The bigger the MR, the longer the code review will take, and the less issues it will discover.
Referring to the noted document you, should focus on submitting the MR as small as possible. If the feature you work on is way too complex, split it into smaller features to correspond to the rule of the small MR and simple code reviews. The bug/issue discovered during the code review will keep you from a stressful Sunday night fixing it in the frantic minutes before leaving for your vacation. Always keep in mind how you feel while reviewing a 4k LOC MR. Can you focus on it? Can you keep track of it? Do you fully understand the purpose of the changes made? If the answer is a simple, NO! Then no worries, Danger is here to help you. All you need to do, is to add 2 last lines to your Dangerfile.
If it so happens that someone creates a merge request with over 400 LOC Danger will kindly warn you not to do so. If it so happens that anyone creates a MR with over 1000 LOC, Danger will fail to do the job and the MR will be unmergeable in the gitlab UI.
Complexity & Unclarity
The more complex code you submit the more headaches you will have for your reviewers. If you tend to work on a complex solution, try to simplify the things (unless you develop a brand new cipher system🤞). You can protect yourself and your colleagues from modifying multiple areas of code at once. It's pretty easy to update the Dangerfile if you want to check multiple areas of code.
Such a setup allows you to check if multiple areas of code are being modified in one merge request. You can also improve the script to check the modification of dependencies and its direct implementation.
Why not to add a 3rd party dependency and use it in one MR?
You can spend a couple of MDs (man days) implementing a new feature using a 3rd party dependency before submitting it for the code review. Imagine, the reviewer knows some better 3rd party library or will tell you to make it on your own because it's really simple. What to do now with the couple of MDs already spent by developing the feature? That's the reason to submit 3rd party dependency updates in separate merge request. It speeds the review up and puts no overhead to the reviewer because he doesn't need to get through all the 3rd party code and look for your changes.
Giving not enough information on a Merge Request to the reviewer
To give a proper code review, you need the context. The author should give the reviewer enough context to understand the problem and its solution. If you as a developer tend to give a vague description of what you've done, you likely won't get a proper code review. To avoid it, try to follow these simple rules:
- Give a proper name to the merge request
- Add a link to the tracking system (JIRA, Redmine, Gitlab ticket number …)
- Briefly describe what & why you didit
- Select your reviewer and notify him
In the Dangerfile I already made the check as to whether the merge request description is longer than 5 characters. You should consider writing a better condition. This is just a sample, thus it's up to you to define your own custom rules.
However if you are as lucky as am I and you use gitlab as your versioning tool, you can use templates for your merge requests. Go and visit gitlab to see how it works.
This post briefly described how to improve the codebase by unifying the merge requests. In the next post we will focus on adding plugins to the Danger implementation to make your life much easier.
You might notice that I mostly use fail instead of warn and you might wonder why? It's simple…
Warning is not like hitting the wall. You can skip it. Failure is the wall that really stops you.
