Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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:

  1. 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.
  2. With the fix: but that obscures the actual replacement of defective code with files where findGeoragphy was correct to findGeography.
  3. 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.

share|improve this question
    
No Unit Tests? Why not write it before the fix? (if appropriate) –  Wolf Mar 18 at 9:34
    
The code wasn't unit testable because it was heavily database dependent and there was no specification of what it should do, so the unit tests would have been "testStillDoesWhatItDoes()" instead of something behaviour or semantic and with layering violations it would have been really hard to mock out as well. Also, a lot of the changes where of opportunity like correcting Engrish method names as I came by them but didn't have time to write tests for and they'd remain in Engrish otherwise. –  ArtB Mar 18 at 13:41

3 Answers 3

up vote 7 down vote accepted

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.
share|improve this answer

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.

share|improve this answer
4  
Regarding the second paragraph: In such situations I sometimes re-use the ticket ID of the fix I am making, but still put the cleanup in a separate commit. –  Bart van Ingen Schenau Jan 16 '13 at 16:47

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.