Should I use Resharper to tidy up other peoples code?

Question

I use Resharper at work. Some of my colleagues do not.

When I open some code that has been written someone who doesn't, it is immediately obvious by the amount of orange on my screen.

What I am unsure of is to what extent I should feel free to tidy up the messes the have unknowingly left. With most of what I am looking at, it is sloppy but harmless, and wouldn't really jump out at me if I hadn't ever used Resharper.

I guess I see my options broadly as

1) The history of changes to source code is essential to maintenance. Change as little as possible, or the next guy wont have a hope of figuring out what has changed. Who cares about unreachable code, unnecessary use of .ToString() etc. anyway.

2) Change the meaningless stuff like includes, fix method documentation comments and things like that. The guy who wrote it likes his code to look like this, so leave it in a state where he wont complain, but get rid of some of the unnecessary orange

3) Orange is just red but lighter. F12 then Alt+Enter until green.

4) Forget the orange, look at that monster 700 line function. What is this 1997? Time to get busy... and if you have got the time, introduce your colleague to our good friend and mentor Mr Fowler.

I tend to flit between the options depending on how much time I have got, to what extent I am now responsible for the code, and how complex the code looks (which can make me go for 1 or 4 usually).

It seems like one of the 4 options should be the one I am striving for though, but I have no idea which one

Answer 1

I do a "reformat" check in before I change any changing to any of the actual logic, this way you can see what has changed.

Answer 2

Your team should agree on a standard. If someone else uses another tool, you may find yourself in an unintentional edit war.

But if you can all agree, then yes. Clean up the code as you go.

Answer 3

If the development tools in your team are not the same eventually there will be even more trouble. Follow the "reformat" check-in suggested above and standardize the toolset with your colleagues, either drop resharper or give everyone the formatting magic wand.

Answer 4

If it's already checked in, be careful. Some people get awfully touchy and it's considered rude if someone on your team is still using tools from a decade ago that can't disable whitespace detection during a diff.

In general, I'll clean up code styling to bring it in line with the stated organization style goals when I'm touching the code for another reason. Try to remember that style is style, however, and as such there is no "one true way." Don't make enemies because you don't like the fact that a colleague uses less (or more) whitespace than you do.

Answer 5

Needless refactoring is just that - needless. It clutters up the repository history logs and you can introduce bugs.

If the "meaningless" stuff (documentation, comments, etc.) is supposed to be formatted a certain way, and it's not up to your development standards, then I would do it all at once in as few checkins as possible.

When you are actually working on the pieces of code and have the opportunity to test your changes, do the refactoring then. Resharper will always be available to show you the way at that point.

Answer 6

Any time you change anything, regardless of the intent, you risk breaking something unintentionally. On a personal side, I wouldn't change another person's code without talking to them first.

Answer 7

I recommend only changing things when you need to. There is, of course, some varying definition on what "need" means. For example, if you write a method that calls another method, and this method you're calling happens to have some code duplication? I'd refactor that, and while I'm at it remove excess using statements, etc. I'd try to avoid just a massive refactor of the entire codebase "just because" though.

Answer 8

My suggestion would be that if you do a reformat that it be done separately from any code modifications. The reason for this is that you can mark it as "reformat only" in the repository and a legitimate code change will not be buried in the middle of a bunch of spacing changes making it impossible for you or the next guy to figure out what broke.

Answer 9

"Leave the campsite cleaner than you found it."

It's the boyscout principle. If it's "their" code and they maintain it then introducing little cleanup-changes shouldn't offend them, but going too far might seem rude or you might be effectively taking ownership of the code.

Answer 10

As the majority of people have already said, yes, better to refactor and leave it tidier.

Refactoring helps everyone get better, and everyone should have access to the refactoring tools (Coderush for me).

However, if your colleagues aren't sharing the refactoring love, then it's a good opportunity for you to enlighten them :)