Don’t let the Boy Scouts rule hinder Pull Requests
The Boy Scouts have a rule: “Always leave the campground cleaner than you found it.” -Robert Baden-Powell-
For us software developers, campground = existing code.
So when fixing a bug or adding a new feature, we clean up all related code right?
Yes, but…
If we do so, we end up “polluting” our pull requests.
As a really trivial example, let’s imagine we’re fixing the bug below, followed by a Pull Request.
Bug 🐛: Notice that person.LastName is not set, therefore we get a NullReferenceException in line 12.
Also notice how the IDE warns us of potential code cleanups. E.g. remove the unused variable at line 6.
We now have 2 options:
A. Apply ONLY the code fix.
B. Apply the code fix AND be a boy scout.
The Pull Requests image below represents option (A) to the left, and (B) to the right. I think most code reviewers would agree it’s easier to verify the code fix with Pull Request (A).
Sure, the above code is tiny enough that applying the code fix plus code cleanup shouldn’t detract the code reviewer too much from the actual requirement. But more often than not, we’re dealing with larger code changes.
Best Practice
We still want clean code, that’s for sure. But perhaps larger code cleanups belong in their own pull requests. That way it sets the expectation for the code reviewer and they know exactly what to watch out for.
For code cleanup PR’s, the code reviewer is focused on checking there are no functional changes.
For bug and feature PR’s, the code reviewer is focused on making sure the code change meets the functional requirements.
