There are quite few things we cannot escape in life, like getting fatter if we don’t exercise or eat properly.
For us developers, though, what we really wish we could avoid is technical debt. Sadly enough, technical debt cannot usually be averted (at least not completely).
From my perspective the very moment when the code is committed, it becomes automatically legacy code that will need to be maintained.
People in the organisation will change, clients will change and most importantly requirements will change. Our shining and beautiful code that we’ve just written will make no sense to the future developers that will look at it (and give it enough time, it will not make sense to whom wrote it in the first place also).
What of it then? As our codebases get larger and larger and get contributed by more and more developers, we’ll find ourselves lost in a code maze without any direction: we’ll see a variables called count, count2 and count3 in a method with several hundreds line of code, with nested foreach and possibly recursion.
How did we end up here? I’d prefer to start with why.
Developers are said to bordering artistry. While I’m not totally convinced by it, I see the great gift we received: we are makers and we love it. The whole process of creation can become a transcendental experience, where we isolate ourselves from the world and solve difficult problems using our mind and knowledge.
The main concern here is that, while we do it, we almost never take a step back and try to figure out if someone else would understand what we’ve done or even like it.
Feedback is in the eye of the beholder
As we will are not able to make sure our code will be easily understood and maintained by others we do code reviews. Usually senior devs go through the code and say what’s good and what is not, often according to their own taste.
A Pull Request etiquette
What we’ve come up with is a standardized etiquette for the Pull Request submitted and the Pull Request Reviewer.
Etiquette for the Pull Request Submitter
What am I trying to do?
Please explain with your own words what is your intent with this code change.
What I am thinking in terms of impacts (changes in the app/configurations that need to change in order for this to work)?
Do we need to update the build and or release definitions? Do we need to change the Powershell scripts? How will the application behavior change?
Is the team in sync with this impact?
Is the rest of the team aware of this change? I’m a changing a functionality someone is also working on?
Did I validate the change locally?
For code changes: Did I run the application locally with this change?
For script changes: Did I validate the script locally? Did I account for the script usage in the current builds/releases?
Did I provide tests for this code?
You are expected to provide code for every change you submit. If you were unable to (UI-only change/untestable code) you need to flag this to your dev lead.
How did I improve the codebase?
You are expected to improve the codebase with each and every pull request. If your change did not involve code, fish for:
- Renaming a class using the actor model (e.g. QueryPerformer, ContractTypeDeterminer)
- Renaming variables to meaningful names
- Moving a class into the correct layer
Etiquette for the Pull Request submitter
Does it solve the problem? Will it be relevant two weeks, two months, two years, two decades from now?
Taking a step back, is this a real fix to the problem or just a temporary patch that will get the client to send back another bug report next month?
Does it do it efficiently given the constraints?
Keeping into account the time and budget constraints is this a good solution? Is it worth to invest more time and improve the code scalability/performance?
Would this code be easy to support moving forward?
Would we be able to understand this code in six months?
Does this change has a significant risk of introducing a security breach?
Are there possible security considerations that haven’t been taken into account?
Checklists are not intended to solve all the problems. They serve a very specific purpose: they keep us focus on the fun and great work we want to do, while making sure we do not forget to take care of the houseworks as well.
Having a set of checklists for the PullRequest process ensures that the code that gets in has gone through some serious consideration before being merged and makes the review process more consistent across different reviewers.