I’m working a small-to-medium legacy codebase and when working on a ticket I will come across code that should be cleaned-up or that I need to clean-up just to be able to understand the follow of the application.
A real example is:
if( !a && b ){
doSomething1();
doSomething2();
doSomething3();
doSomething4();
doSomething5();
}else if( a ){
doSomething1();
doSomething2();
doSomething3();
doSomething4();
doSomething5();
}else if( !b && ( a || c ) ){
doSomething1();
doSomething2();
doSomething3();
doSomething4();
doSomething5();
}
Another is fixing typos and Engrish in comments and documentation across a dozen source files.
However, often this clean-up ends up not related to the main issue and I wonder how is it best to commit the clean-up. The way I see it, there are three options:
- Before the fix: this works chronologically since this is the order in which they occur but if they break something they complicate the fix and it makes it harder to diff the fix with what was in production. It also introduces an additional commit which is noise.
- With the fix: but that obscures the actual replacement of defective code with files where
findGeoragphywas correct tofindGeography. - After the fix: this requires removing and clean-up you made that helped you understand the code and then retesting the fix, committing the fix, and then going back and redoing the clean-up. This allows the cleanest diff to the defective code, but duplicates effort and can lead to spurious commits as well.
tl;dr: So, what is the best method of committing clean-up code?
Context: We have no unit-tests and development proceeds by running changes and eye-balling them and throwing them over the wall to QA to validate through manual regression fixes. Also, if we don’t do some form clean-ups the code becomes unintelligible. I know this is far from ideal, but this is real enterprise dev that puts food on my table and not my choice.
3
I follow a process similar to Karl’s answer. I prefer a separate commit of the cleanup / refactoring prior to the feature changes for a few reasons:
- Separating the two commits will explain your process and thinking better to others who look at the logs.
- It provides a specific set of changes useful during a code review.
- It means if you want to roll back your feature changes you still keep the cleanup / refactor.
- The cleanup changes can be tested separately and prior to feature updates.
I prefer the extra commit before the fix. This clearly separates the two tasks you are doing, and allows other people to see what was cleanup versus what was a bug fix. Also, cleanup (for me at least) is usually a lot faster, so if I commit a cleanup fix, I don’t have to worry as much about someone making a difficult-to-merge change while I’m working on the longer task.
I have used the “commit with the fix” approach at places with policies that require a change request ID with every commit, and don’t allow developers to create new change requests just for cleanup code. Yes, such policies are unproductive, but workplaces like that do exist.
1
If the fix might have to be patched into an existing release, you should check in the fix first, and then the clean-up after that, or you are (at least some of the time) making the person who needs to apply the fix work harder than they need to (either to understand what changed, or to patch the cosmetic changes before patching the real fix (I’ll note that I have made more than a few alledgedly cosmetic changes which ended up breaking something, so patching more than the minimum change to fix a defect can increase the risk of releasing something bad).
Yes, this is extra work. Yes, it’s a pain. But yes, it’s the right way to do things. My normal workflow is to do everything in one sandbox (clean-up and defect fixing), then create a new sandbox, apply only the minimal defect fix change from it, check this change out in the original sandbox, and check in the clean-up code. It’s not that much extra work, in my experience.