Applying the Single Responsibility Principle to Pull Requests
The single-responsibility principle (SRP) states that a piece of code (i.e. a function, class, module, etc.) should do one thing well.
Generally, this comes from the fact that when a piece of software has too many responsbilities, the complexity and maintenance cost grow superlinearly.
Just like SRP is a best practice in software design, SRP also should be used when developing pull requests. Although, not everyone agrees.
I knew a senior engineer who purported that team members should do code cleanups for any file they change when doing sprint work. The idea is noble. Even Uncle Bob, author of Clean Code, described the boy-scout rule, which encourages you to
Always leave the code better than how you found it.
In theory, this principle is righteous. But I find this not to be an effective way to structure changes when working on a team.
When you make a change to a codebase, whether you're adding a feature or fixing a bug, the focus should about that specific change and nothing more.
When you start introducing a bunch of unrelated "quality-of-life improvements", it makes the true intent of the PR less clear. Your coworkers will have to sift through a bunch of red and green to figure out what change is going to have impact vs what are the unrelated code cleanups.
Each time you slip in unrelated code changes, the diff gets larger. The larger the diff, the easier it is for bugs to go unnoticed by the reviewer. The easier it is for bugs to enter the system, the more risk you introduce to your customers and business.
If we apply SRP to pull-requests, our code changes should do one thing well. This way the reviewer can go into your PR knowing what to expect and have a single point of focus. So how do you apply this in practice?
- If you're adding a feature, your change should be adding that feature and nothing more.
- If you're fixing a bug, your change should be fixing that bug and nothing more.
- If you're refactoring, your change should be refactoring and nothing more.
If you're working on a change and notice something unrelated that should be fixed, then open another ticket and handle it in a separate change.
However, sometimes being pragmatic trumps best practices. Sometimes it's necessary to do code cleanup to accomplish a task. The intent here is not to be dogmatic, but to provide of a mental framework when building PRs to ease the cognitive load on the reviewer and minimize risk to your business.