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 am a great believer in the Boy Scout Rule:

Always check a module in cleaner than when you checked it out." No matter who the original author was, what if we always made some effort, no matter how small, to improve the module. What would be the result? I think if we all followed that simple rule, we'd see the end of the relentless deterioration of our software systems. Instead, our systems would gradually get better and better as they evolved. We'd also see teams caring for the system as a whole, rather than just individuals caring for their own small little part.

I am also a great believer in the related idea of Opportunistic Refactoring:

Although there are places for some scheduled refactoring efforts, I prefer to encourage refactoring as an opportunistic activity, done whenever and wherever code needs to cleaned up - by whoever. What this means is that at any time someone sees some code that isn't as clear as it should be, they should take the opportunity to fix it right there and then - or at least within a few minutes

Particularly note the following excerpt from the refactoring article:

I'm wary of any development practices that cause friction for opportunistic refactoring ... My sense is that most teams don't do enough refactoring, so it's important to pay attention to anything that is discouraging people from doing it. To help flush this out be aware of any time you feel discouraged from doing a small refactoring, one that you're sure will only take a minute or two. Any such barrier is a smell that should prompt a conversation. So make a note of the discouragement and bring it up with the team. At the very least it should be discussed during your next retrospective.

Where I work, there is one development practice that causes heavy friction - Code Review (CR). Whenever I change anything that's not in the scope of my "assignment" I'm being rebuked by my reviewers that I'm making the change harder to review. This is especially true when refactoring is involved, since it makes "line by line" diff comparison difficult. This approach is the standard here, which means opportunistic refactoring is seldom done, and only "planned" refactoring (which is usually too little, too late) takes place, if at all.

I claim that the benefits are worth it, and that 3 reviewers will work a little harder (to actually understand the code before and after, rather than look at the narrow scope of which lines changed - the review itself would be better due to that alone) so that the next 100 developers reading and maintaining the code will benefit. When I present this argument my reviewers, they say they have no problem with my refactoring, as long as it's not in the same CR. However I claim this is a myth:

(1) Most of the times you only realize what and how you want to refactor when you're in the midst of your assignment. As Martin Fowler puts it:

As you add the functionality, you realize that some code you're adding contains some duplication with some existing code, so you need to refactor the existing code to clean things up... You may get something working, but realize that it would be better if the interaction with existing classes was changed. Take that opportunity to do that before you consider yourself done.

(2) Nobody is going to look favorably at you releasing "refactoring" CRs you were not supposed to do. A CR has a certain overhead and your manager doesn't want you to "waste your time" on refactoring. When it's bundled with the change you're supposed to do, this issue is minimized.

The issue is exacerbated by Resharper, as each new file I add to the change (and I can't know in advance exactly which files would end up changed) is usually littered with errors and suggestions - most of which are spot on and totally deserve fixing.

The end result is that I see horrible code, and I just leave it there. Ironically, I feel that fixing such code not only will not improve my standings, but actually lower them and paint me as the "unfocused" guy who wastes time fixing things nobody cares about instead of doing his job. I feel bad about it because I truly despise bad code and can't stand watching it, let alone call it from my methods!

Any thoughts on how I can remedy this situation ?

share|improve this question
32  
I'd feel uneasy working at a place where your manager doesn't want you to "waste your time" on refactoring –  Daenyth Jun 12 at 14:43
13  
In addition to having multiple CRs, the key point is that each commit should be for a single purpose: one for the refactor, one for the requirement/bug/etc. That way, a review can differentiate between the refactor and the requested code change. I would also argue that the refactor should only be done if there are unit tests in place that prove your refactor did not break anything (Uncle Bob agrees). –  Snowman Jun 12 at 15:24
2  
@t0x1n I don't see that as any different –  Daenyth Jun 12 at 15:29
1  
@t0x1n yep, I missed that one. Not enough coffee this morning. In my experience there are a few ways to refactor. Maybe you look at the code you need to modify and immediately know it needs cleanup, so you do that first. Maybe you have to refactor something in order to make your change because the new requirement is incompatible with the existing code. I would argue the refactor is intrinsically part of your change and should not be considered separate. Finally, maybe you see the code sucks halfway through your change, but you can finish it. Refactor after the fact. –  Snowman Jun 12 at 15:40
3  
Bullet 1 doesn't claim that separating the commits is impossible. It just implies that you don't know how to do it, or your VCS makes it hard. I do this all the time, even taking a single commit and splitting it after the fact. –  Useless Jun 12 at 16:47

12 Answers 12

up vote 12 down vote accepted

OK, so there are now more things here than are suitable for a comment.

tl;dr

Your intuition about what you ought to do (refactoring as you go) is correct.

Your difficulty implementing this - given that you have to work around a poor code review system - comes down to your difficulty manipulating your source code and VCS. Multiple people have said that you can and should split your changes (yes, even within files) into multiple commits, but you seem to have difficulty believing this.

You really can do this. That really is what we're suggesting. You really should learn to get the most out of your editing, source manipulation and version control tools. If you invest the time in learning how to use them well, it makes life much easier.


Workflow/office politics issues

I'd make essentially the same recommendation as GlenH7 that you create two commits - one with only refactorings and (demonstrably or obviously) no functional changes, and one with the functional changes in question.

It may be useful though, if you're finding lots of errors, to pick a single category of error to fix within a single CR. Then you have one commit with a comment like "dedupe code", "fix type-X errors", or whatever. Because this makes a single type of change, presumably in multiple places, it should be trivial to review. It does mean you can't fix every error you find, but may make it less painful to smuggle through.


VCS issues

Splitting the changes made to your working source into multiple commits shouldn't be a challenge. You haven't said what you're using, but possible workflows are:

  1. if you're using git, you have excellent tools for this

    • you can use git add -i for interactive staging from the command line
    • you can use git gui and select individual hunks and lines to stage (this is one of the few VCS related GUI tools I actually prefer to the command line, the other being a good 3-way merge editor)
    • you can make lots of tiny commits (individual changes, or fixing the same class of bug in multiple places) and then reorder them, merge them or split them with rebase -i
  2. if you're not using git, your VCS may still have tooling for this sort of thing, but I can't advise without knowing what system you use

    You mentioned you're using TFS - which I believe is compatible with git since TFS2013. It may be worth experimenting with using a local git clone of the repo to work in. If this is disabled or doesn't work for you, you can still import the source into a local git repo, work in there, and use it to export your final commits.

  3. you can do it manually in any VCS if you have access to basic tools like diff and patch. It's more painful, but certainly possible. The basic workflow would be:

    1. make all your changes, test, etc.
    2. use diff to make a (context or unified) patch file with all changes since the last commit
    3. partition that into two files: you'll end up with one patch file containing refactorings, and one with functional changes
      • this isn't entirely trivial - tools such as emacs diff mode may help
    4. back everything up
    5. revert to the last commit, use patch to replay one of the patch files, commit those changes
      • NB. if the patch didn't apply cleanly, you may need to fix up the failed hunks manually
    6. repeat 5 for the second patch file

Now you have two commits, with your changes partitioned appropriately.

Note there are probably tools to make this patch management easier - I just don't use them since git does it for me.

share|improve this answer
    
I'm not sure I follow - does bullet 3.3 assume the refactoring and functional changes reside in different files? At any rate, they don't. Perhaps separating by lines makes more sense but I don't think we have tooling for this in our CVS (TFS). At any rate, it wouldn't work for many (most?) refactorings where the functional changes rely on the refactored changed. For example suppose I refactor method Foo (which I need to use as part of my functional changed) to take 3 parameters instead of 2. Now Imy functional code relies on the refactor code, even splitting by line won't help. –  t0x1n Jun 12 at 17:25
    
Different lines in the same file are fine in the workflows given. And given the two commits would be sequential, it's perfectly ok for the second (functional) commit to depend on the first. Oh, and TFS2013 allegedly supports git. –  Useless Jun 12 at 17:34
    
We also use TFS for the source control. You assume the second commit will be the functional one, whereas it would typically be the opposite (seeing as I can't tell beforehand what refactoring would have to be done). I suppose I could do all my work of functional + refactoring, then get rid of anything functional, and add it back in a separate commit. I'm just saying, that's a lot of hassle (and time) just to keep a couple reviewers happy. A more reasonable approach to my mind is to allow opportunistic refactoring and in return agree to CR such changes yourself (accepting the added difficulty). –  t0x1n Jun 12 at 18:49
    
I do admit I like refactoring->functional much better than functional->refactoring though, perhaps I'll try it the next time I get the chance. It remains to be seen if removing the functional portion of a functional+refactoring change is as simple as it sounds. I believe in some cases it will, in some it won't. –  t0x1n Jun 12 at 19:12
2  
I think you're really not understanding me. Editing source and grouping edits into commits, yes even edits in the same file, are logically separate activities. If this seems difficult, you just need to learn the available source code management tools better. –  Useless Jun 13 at 8:28

I'm going to assume that Change Requests are big and formal at your company. If not, just make the changes (where possible) into many small commits (like you're supposed to).

Any thoughts on how I can remedy this situation?

Continue doing what you're doing?

I mean, all of your thoughts and deductions are entirely correct. You should fix things you see. People don't do planned refactoring enough. And that benefit to the whole group is more important than inconveniencing a few.

What may help is to be less combative. Code reviews shouldn't be combative "Why did you do that?", they should be collaborative "Hey guys, while I was here I fixed up all this stuff!". Working (with your lead/manager if possible) to change that culture is tough, but pretty vital to create a high functioning development team.

You can also work (with your lead/manager if possible) to advance the importance of these ideas with your colleagues. Make it into you asking "why don't you guys care about quality?" instead of them asking "why are you always doing these useless things?".

share|improve this answer
4  
Yes, CRs are big and formal. Changes are CRed, signed off, then submitted into a queue. Small changes are supposed to be added as iterations to an ongoing CR rather than commited separately. WRT continuing what I'm doing, that may indeed benefit the group but I'm afraid it won't benefit me. Those people I "inconvenience" are probably the same people who would rate me in the yearly review. The problem with changing the culture is that the big chiefs believe in it. Perhaps I just need to earn more respect in their eyes before attempting something like that... –  t0x1n Jun 12 at 15:14
11  
@t0x1n - Don't look at me. I've made a career of doing the right thing in the face of people who are stubbornly beholden to sucking. Maybe not as profitable than I might have had if I made people happy, but I sleep well. –  Telastyn Jun 12 at 15:31
    
Thanks for being honest. It is indeed something to contemplate. –  t0x1n Jun 12 at 15:50
    
I often run into this too. Ensuring I have a "cleanup" patch and then a work patch helps a lot. Usually I fight within the system and then leave to work somewhere that is less stressful. That said, there are sometimes valid reasons for your coworkers' concerns. For instance if code goes rapidly into production and there is no sufficient tests. I have seen code review as an attempt to avoid testing. It does not work. Code review helps keep a body of code uniform. It does little for bugs. –  Sean Perry Jun 12 at 22:08
    
@SeanPerry aggreed - but I'm talking about normal circumstances where tests exist, bug bashes will be performed, etc. –  t0x1n Jun 13 at 7:45

Why don't you do both, but with separate commits?

Your peers have a point. A code review should evaluate the code that was changed by someone else. You shouldn't touch the code that you are reviewing for someone else as that biases your role as a reviewer.

But if you see a number of glaring issues, there are two options you can follow.

  1. If the code review was otherwise fine, the allow the portion you reviewed to be committed and then refactor the code under a second check-in.

  2. If the code review had issues that needed correction, request the developer to refactor based upon the Resharper recommendations.

share|improve this answer
    
I gather from your answer that you believe in Strong Code Ownership. I encourage you to read Fowler's thoughts about why it's a bad idea: martinfowler.com/bliki/CodeOwnership.html. To address your points specifically: (1) is a myth as refactoring happens while you are making your change - not before or after in a separate, clean, unrelated fashion as separate CRs would require. (2) With most devs, it will never happen. Never. Most devs don't care about these things, much less when coming from some other guy who is not their manager. They have their own things they want to do. –  t0x1n Jun 12 at 15:23
6  
@t0x1n: If your managers do not care about refactoring, and your fellow developers do not care about refactoring ... then that company is slowly sinking. Run away! :) –  logc Jun 12 at 15:38
6  
@t0x1n just because you make the changes together, doesn't mean you have to commit them together. Besides, it's often useful to check your refactoring had no unexpected side-effects separately from checking your functional change had the expected effect. Obviously this may not apply to all refactorings, but it's not bad advice in general. –  Useless Jun 12 at 16:45
1  
@t0x1n - I don't recall saying anything about strong code ownership. My answer was to keep your role as a reviewer pure. If a reviewer introduces changes then they are no longer just a reviewer. –  GlenH7 Jun 12 at 17:35
2  
@GlenH7 Perhaps there was some misunderstanding here - I am not the reviewer. I'm just coding what I need, and run into code I can improve in the process. My reviewers then complain when I do. –  t0x1n Jun 12 at 18:42

I have lots of sympathy for your situation, but few concrete suggestions. If nothing else, maybe I'll convince you that as bad as a situation is, it could be worse. It can ALWAYS be worse. :-)

First, I think you have (at least) two problems with your culture, not just one. One problem is the approach to refactoring, but the code reviews seem like a distinct problem. I'll try to separate my thoughts.

Code Reviews

I was in a group that HATED code reviews. The group was formed by merging two groups from separate parts of the company. I came from the group that had been doing code reviews for several years, with generally good results. Most of us believed code reviews were a good use of our time. We merged into a larger group, and as near as we could tell, that "other" group had never even heard of code reviews. Now we were all working on "their" code base.

Things were not well when we merged. New features were 6-12 months late, year after year. The bug backlog was huge, growing, and life-draining. Code ownership was Strong, especially among the most senior "gurus". "Feature branches" sometimes lasted years and spanned a few releases; sometimes NOBODY but a single developer saw the code before it hit the main branch. Actually "feature branch" is misleading, since it suggests the code was somewhere in the repository. More often it was only on the developer's individual system.

Management agreed that we needed to "do something" before quality became unacceptably low. :-) Their answer was Code Reviews. Code Reviews became an official "Thou Shalt" item, to precede every check-in. The tool we used was Review Board.

How did it work in the culture I described? Better than nothing, but it was painful and took more than a year to reach a sort of minimal level of compliance. Some things we observed:

  1. The tool you use tends to focus code reviews in certain ways. This can be a problem. Review Board gives you nice, colorful line-by-line diffs and lets you attach comments to a line. This makes most developers ignore all the lines that didn't change. That's fine for small, isolated changes. It's not so good for big changes, large hunks of new code, or code that has 500 instances of a renamed function mixed with 10 lines of new functionality.
  2. Even though we were in an old, sick code base that had mostly never been reviewed before, it became "impolite" for a reviewer to comment on anything that was NOT a change line, even to point out an obvious bug. "Don't bother me, this is an important check-in and I don't have time to fix bugs."
  3. Shop for an "easy" reviewer. Some folks will look at a 10-file review with 2000 changed lines for 3 minutes and click "Ship It!". Everyone quickly learns who those people are. If you really don't want your code reviewed in the first place, send it to an "easy" reviewer. You check-in will not be slowed down. You can return the favor by becoming an "easy" reviewer for his code.
  4. If you hate code reviews, just ignore the emails you get from Review Board. Ignore the follow-ups from your team members. For weeks. Until you have 3 dozen open reviews in your queue and your name comes up in group meetings a few times. Then become an "easy" reviewer and clear all your reviews before lunchtime.
  5. Avoid sending your code to a "hard" reviewer. (The sort of developer who would ask or answer a question like this one.) Everyone quickly learns who the "hard" reviewers are, just as they learn the easy ones. If code reviews are a Waste Of Time (™), then reading detailed feedback about the code you OWN is both Waste Of Time (™) and an insult.
  6. When code reviews are painful, people put them off and keep writing more code. You get fewer code reviews, but each one is BIG. You need more, smaller code reviews, which means the team needs to figure out how to make them as painless as possible.

I thought I was going to write a few paragraphs about Code Reviews, but it turns out I was mostly writing about culture. I guess it boils down to this: code reviews can be good for a project, but a team gets only the benefits they deserve to get.

Refactoring

Did my group despise refactoring even more than it hated code reviews? Of course! For all the obvious reasons that have already been mentioned. You're wasting my time with an icky code review, and you're not even adding a feature or fixing a bug!

But the code was still in desperate need of refactoring. How to proceed?

  1. Never mix a refactoring change with a functional change. A number of people have mentioned this point. If code reviews are a friction point, don't increase the friction. It's more work, but you should plan on a separate review and a separate check-in.
  2. Start small. Very small. Even smaller than that. Use very small refactorings to gradually teach people that refactoring and code reviews aren't pure evil. If you can sneak in one tiny refactoring per week without too much pain, in a couple of months you might be able to get away with two per week. And they can be a little bigger. Better than nothing.
  3. We had essentially NO unit tests. So refactoring is forbidden, right? Not necessarily. For some changes, the compiler is your unit test. Focus on refactorings that you can test. Avoid changes if they can't be tested. Maybe spend the time writing unit tests instead.
  4. Some developers are scared of refactoring because they are scared of ALL code changes. It took me a long time to recognize this type. The write a chunk of code, fiddle with it until it "works", and then NEVER want to change it. They don't necessarily understand WHY it works. Change is risky. They don't trust themselves to make ANY change, and they certainly won't trust you. Refactoring is supposed to be about small, safe changes that don't alter behavior. There are developers for whom the very idea of a "change that doesn't alter behavior" is inconceivable. I don't know what to suggest in these cases. I think you have to try to work in areas that they don't care about. I was surprised when I learned that this type can have long, stable careers as software developers.
share|improve this answer
    
This is a super-thoughtful answer, thank you! I especially agree with how the CR tool can affect the focus of the review ... line-by-line is the easy way out, that doesn't involve actually understanding what was happening before and what happens now. And of course the code that didn't change is perfect, no need to ever look at that... –  t0x1n Jun 13 at 22:42

I personally hate the idea of of post commit code reviews. Code review should happen, as you make the code changes. What I am of course talking about here is pair programming. When you pair, you get the review for free and you get a better code review. Now, I am expressing my opinion here, I know others share this, there are probably studies that prove this.

If you can get your code reviewers to pair with you, the combative element of code review should evaporate. When you start making a change that isn't understood, the question can be raised at the point of change, discussed and alternatives explored which may lead to better refactorings. You will get a higher quality code review as the pair will be able to understand the wider scope of the change, and not focus so much on line by line changes which is what you get with side by side code compare.

Of course this isn't a direct answer to the problem of refactorings being outside the scope of the change being worked on but I would expect that your peers would better understand the reasoning behind the changes if they where there when you made the change.

As an aside, assuming you are doing TDD or some form of red green refactor, one way to ensure that you get engagement from your peer is to use the ping pong pairing technique. Simply explained as the driver is rotated for each step of the RGR cycle, i.e. pair 1 writes a failing test, pair 2 fixes it, pair 1 refactors, pair 2 write a failing test.... and so on.

share|improve this answer
    
Excellent points. Unfortunately I sincerely doubt I'll be able to change "the system". Sometimes reviewers are from different time zones and geographical locations too, so for such cases it won't fly regardless. –  t0x1n Jun 12 at 16:29

Sometimes, refactoring is a bad thing. Not for the reasons that your code reviewers are giving, though; it sounds like they don't really care about the quality of the code, and you do care, which is awesome. But there are two reasons that should stop you from refactoring: 1. You can't test the changes you made with automated tests (unit tests or whatever) or 2. You're making changes to some code that you don't understand very well; i.e., you don't have the domain specific knowledge to know what changes you should make.

1. Don't refactor when you can't test the changes you made with automated tests.

The person doing your code review needs to be able to be sure that the changes you made did not break anything that used to work. This is the biggest reason to leave working code alone. Yes, it would definitely be better to refactor that 1000 line long function (that really is an abomination!) into a bunch of functions, but if you can't automate your tests then it can be really hard to convince others that you did everything right. I have definitely made this mistake before.

Before refactoring, make sure there are unit tests. If there are not unit tests, write some! Then refactor away, and your code reviewers will have no (legitimate) excuse for being upset.

Don't refactor pieces of code that require Domain-specific knowledge you don't have.

The place I work has a lot of chemical engineering-heavy code. The code base uses the idioms common to chemical engineers. Never make changes that are idiomatic to a field. It might seem like a great idea to rename a variable called x to molarConcentration, but guess what? In all the chemistry texts, molar concentration is represented with an x. Renaming it makes it harder to know what is actually going on in the code for people in that field.

Instead of renaming idiomatic variables, just put comments explaining what they are. If they are not idiomatic, please rename them. Don't leave the i, j, k, x, y, etc. floating around when better names will work.

Rule of thumb for abbreviations: if, when people are talking, they use an abbreviation, it is okay to use that abbreviation in the code. Examples from the code base at work:

We have the following concepts which we always abbreviate when talking about them: "Area of Concern" becomes "AOC", "Vapor cloud explosion" becomes VCE, stuff like that. In our code base, somebody refactored all the instances called aoc to AreaOfConcern, VCE to vaporCloudExplosion, nPlanes to confiningPlanesCount... which made the code actually much harder to read for people that had the domain specific knowledge. I've been guilty of doing stuff like this too.


This might not actually apply in your situation at all, but there are my thoughts on the issue.

share|improve this answer
    
Thanks Cody. Regarding "your code reviewers will have no (legitimate) excuse for being upset" - their excuse is already illegitimate, as they are upset at the increased difficulty of going over the changes rather than correctness, readability, or anything like that. –  t0x1n Jun 14 at 15:51
1  
@t0x1n yeah, they sound like they pretty much suck. My condolences! –  Cody Piersall Jun 14 at 15:53

Probably this problem reflect a much bigger problem with the organization culture. People seems more interested in do "his work" right than in make the product better, probably this company has a "blame" culture instead of a colaborative culture and the people seems more interested in cover itself than in have a whole product/company vision.

In my opinion, you are completly right, the people who review your code are completely wrong if they have complains because you "touch" things outside "your assignment", try to convince those people but never be against your principles, for me this is the most important quality of a true professional.

And if do the right thing give you bad numbers in some stupid corporation way to evaluate your work, what the problem?, who want to "win" this evaluation game in a insane company?, try to change it!, and if imposible or you are tired, find another place, but never, never be against your principles, is the best you have.

share|improve this answer
1  
You start wanting to win the evaluation game once you realize that it rewards you with salary increases, bonuses, and stock options :) –  t0x1n Jun 12 at 21:40
    
No. You are trading money for principles @t0x1n. Simple as that. You have three choices: work to fix the system, accept the system, leave the system. Options 1 & 3 are good for the soul. –  Sean Perry Jun 12 at 22:14
    
not only its bad for soul, a company with principles that focus on local maximuns its normally less optimal that a company that focus on global maximuns. Not only this, work its not only money, you spend a lot of time each day in your work, be confortable at your job, feel that your are doing the right things, probably its much better that a bunch more dollars each month. –  AlfredoCasado Jun 12 at 23:53
    
Meh, souls are overrated ;) –  t0x1n Jun 13 at 8:17
    
@SeanPerry I truly tried to fix the system but it was very hard. (1) I was practically alone in this, and it's hard to go against the big chiefs (I'm just a regular dev, not even senior). (2) These things take time which I simply didn't have. There is a lot of work and the entire environment is very time consuming (emails, interruptions, CRs, failing test cycles you need to fix, meetings, etc. etc). I do my best to filter and be productive but typically I can barely finish my "prescribed" work in time (having high standards doesn't help here), let alone work on changing the system... –  t0x1n Jun 13 at 8:23

This question now has two distinct issues - ease of reviewing changes in code reviews, and time spent on refactoring.

Where I work, there is one development practice that causes heavy friction - Code Review (CR). Whenever I change anything that's not in the scope of my "assignment" I'm being rebuked by my reviewers that I'm making the change harder to review.

As other answers have said - can you separate the refactoring checkins from the code change checkins (not necessarily as separate reviews)? Depending on the tool you're using to do code review you should be able to view the diffs between specific revisions only (Atlassian's Crucible definitely does this).

(2) Nobody is going to look favorably at you releasing "refactoring" CRs you were not supposed to do. A CR has a certain overhead and your manager doesn't want you to "waste your time" on refactoring. When it's bundled with the change you're supposed to do, this issue is minimized.

If the refactoring is straightforward and makes the code easier to comprehend (which is all you should be doing if it is just refactoring) then the code review shouldn't take long to complete and should be minimal overhead which is won back in spades when someone has to come and look at the code again in the future. If your bosses aren't able to grok this then you might need to gently nudge them towards some resources which discuss why the Boy Scout Rule leads to a more productive relationship with your code. If you can convince them that the "waste [of] your time" now will save two, five or ten times as much later on when you/someone else comes back to do more work on that code then your problem should go away.

The issue is exacerbated by Resharper, as each new file I add to the change (and I can't know in advance exactly which files would end up changed) is usually littered with errors and suggestions - most of which are spot on and totally deserve fixing.

Have you tried bringing these issues to the attention of your colleagues and discussing why they are worth fixing? And can any of them be fixed automatically (assuming you have sufficient test coverage to confirm you haven't broken stuff with the automated refactoring)? Sometimes it isn't "worth your time" to perform a refactoring if it is really nit-picky stuff. Does your entire team use ReSharper? If they do, do you have a shared policy on what warnings/rules are being enforced? If they don't, again perhaps you should be demonstrating where the tool is helping you to identify areas of the code which are possible sources of future pain.

share|improve this answer
    
Regarding CR separation, I have pointed out in my post and several other comments why I think it impossible. –  t0x1n Jun 12 at 16:26
    
I'm not talking about nit-picky stuff, I'm talking about actual performance and correctness issues, as well as redundant code, duplicate code, etc. And that's just R# stuff, there's also some genuinely bad code I can easily fix. Unfortunately not all of my team use resharper, and even those who do don't take it too seriously. A massive educational endeavor is needed, and perhaps I'll try to lead something like that. It's hard though, as I barely have enough time for the work I have to do, let alone side education projects. Perhaps I just need to wait for a downtime period to exploit. –  t0x1n Jun 12 at 16:32
    
I'm just going to chime in and say that it is definitely not impossible, as I see it done all the time. Make the change you would make without the refactoring, check it in, then refactor to clean up, and check that in. It's not rocket science. Oh, and be prepared to defend why you consider it worthwhile to have spent the time refactoring and reviewing the refactored code. –  Eric King Jun 12 at 18:23
    
@EricKing I suppose I could do that. However: (1) I'd have to work with ugly code and keep notes of what I want to improve until I'm done with the functional changes which both sucks and actually slows my functional progress (2) Once I submit my functional changes and revisit my notes for refactoring, that will only be the first iteration and completing the refactoring may take more time, which as you suggested I'd have a hard time explaining to my managers, seeing as my work is already "done". –  t0x1n Jun 12 at 18:54
1  
"I'm talking about actual performance and correctness issues" - then this might be stretching the definition of refactoring; if the code is actually incorrect it would constitute bug fixing. As for performance issues, that isn't something that you should just be fixing as part of a feature change, it's probably something that needs measuring, thorough testing and seperate code review. –  Chris Cooper Jun 13 at 7:55

If you can split the requested change and the unrequested refactor into two separate change requests, as noted by @GlenH7, then that is the best you can do. You are then not "the guy who wastes time" but "the guy who works twice as hard".

If you often find yourself in the situation that you cannot split them, because the requested work now needs the unrequested refactor to compile, I would suggest that you insist on having tools to check automatically for coding standards, using the arguments outlined here (the Resharper warnings on their own may be a good candidate, I am not familiar with it). Those arguments are all valid, but there is one extra that you can use to your advantage: having those tests now make it your duty to pass the tests on each commit.

If your company does not want to "waste time on refactoring" (a poor sign on their part), then they should provide to you a "warning suppression" file (each tool has its own mechanism) so that you are not annoyed anymore with such warnings. I say this in order to provide you with options for different scenarios, even worst case. It is also better to have clearly stated agreements between you and your company, also on the expected code quality, rather than implicit assumptions on each side.

share|improve this answer
    
This would be a fine suggestion for a new project. However our current codebase is huge, and Resharper emits many errors for most files. It's simply too late to enforce it, and suppressing existing errors makes it even worse - you're going to miss them on your new code. Also there are many errors, issues, and code smells a static analyzer won't catch, I was just giving Resharper warnings as an example. Again I may have worded the "wasting part" bit too harshly, I should have said something like "dedicated time to something that's not a priority". –  t0x1n Jun 12 at 15:27
    
@t0x1n: the Boy Scout Rule involves the modules you are touching, mostly. That may help you to draw a first dividing line. Suppresing warnings is not a good idea, I know, but from the perspective of the company, suppressing them in new code is correct and following their conventions -- well, maybe I am getting carried away by my own argumentation :) –  logc Jun 12 at 15:33
1  
that's the frustrating part! I only touch files I would have touched anyway as part of my task, but I'm getting complaints just the same! The warnings I'm talking about are not style warning, I'm talking about actual performance and correctness issues, as well as redundant code, duplicate code, etc. –  t0x1n Jun 12 at 15:37
    
@t0x1n: it sounds very frustrating indeed. Please note that I did not mean just "static code analysis" but also other recommendations, something equivalent to Nexus. Of course, no tool catches 100% semantics; this is just a remediation strategy. –  logc Jun 12 at 15:43

I can remember 25 years or so ago "cleaning up" code that I happened to be working on for other purposes, mostly by reformatting comment blocks and tabbing/column alignments to make the code neat and therefore easier to understand (no actual functional changes). I happen to like code that's neat and well-ordered. Well, the senior developers were furious. It turns out they used some sort of file compare (diff) to see what had changed, compared to their personal copies of the file, and now it was giving all sorts of false positives. I should mention that our code library was on a mainframe and had no version control -- you basically overwrote whatever was there, and that was it.

The moral of the story? I dunno. I guess it's that sometimes you can't please anyone except yourself. I wasn't wasting time (in my eyes) -- the cleaned up code was easier to read and understand. It's just that the primitive change control tools used by others put some extra one-shot work on them. The tools were too primitive to ignore space/tabbing and reflowed comments.

share|improve this answer
    
Yeah, I get hosed for spacing too, as well as trivial stuff such as redundant casting etc. Whatever isn't completely mandated by my change is "noise". –  t0x1n Jun 13 at 22:34

If you can split the requested change and the unrequested refactor into (a lot of) separate commits, as noted by @Useless, @Telastyn and others, then that is the best you can do. You will still be working on a single CR, without the administrative overhead of creating a "refactoring" one. Just keep your commits small and focused.

Instead of giving you some advices in how to do it, I prefer to point you to a much bigger explanation (in fact, a book) from a specialist. This way, you will be able to learn a lot more. Read Working Effectively with Legacy Code (by Michael Feathers). This book can teach you how to do the refactoring, interleaved with the functional changes.

The topics covered include:

  • Understanding the mechanics of software change: adding features, fixing bugs, improving design, optimizing performance
  • Getting legacy code into a test harness
  • Writing tests that protect you against introducing new problems
  • Techniques that can be used with any language or platform—with examples in Java, C++, C, and C#
  • Accurately identifying where code changes need to be made
  • Coping with legacy systems that aren't object-oriented
  • Handling applications that don't seem to have any structure

This book also includes a catalog of twenty-four dependency-breaking techniques that help you work with program elements in isolation and make safer changes.

share|improve this answer

I too am a great believer in The Boyscout Rule and Opportunistic Refactoring. The problem is often in getting management buy in. Refactoring comes with both risk and cost. What management often overlooks is that so too does technical debt.

It's human nature. We're used to dealing with real problems, not trying to prevent them. We're reactive, not proactive.

Software is intangible and so it's hard to understand that, like a car, it needs servicing. No reasonable person would buy a car and never service it. We accept that such negligence would decrease its longevity and, in the long run, be more costly.

Despite the fact that many managers understand this, they are held accountable for changes to production code. There are politics that make it easier to leave well enough alone. Often the person needing convincing is actually above your manager and he simply doesn't want to fight to get your "great ideas" into production.

To be honest, your manager is not always convinced that your remedy is, in fact, as great as you think it is. (Snake oil?) There's salesmanship. It's your job to help him see the benefit.

Management doesn't share your priorities. Put on your management hat and see with management eyes. You'll more likely find the right angle. Be persistent. You'll effect more change by not allowing the first negative response to deter you.

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.