In "No, Be a Jerk", Justin talks about the importance of giving honest criticism of other developer's code. I agree that providing constructive feedback is important. My designs and code improves when there is discussion about how and why a component was written in a particular way. Talking about a problem and giving feedback is a great way to create innovative and elegant methods for solving problems through code. Uninvited refactoring is far more risky.

Just because you are working in someone's code does not mean that you have the right, the wisdom nor the back-story to make sweeping changes that may come with refactoring. A wise way to refactor is to talk to the responsible developer first and verify that your changes will improve the code base, meet schedule requirements and be an appropriate use of your time.

Let's say your have to make a bug fix for a NullPointerException in another developer's code and you see an inefficient sorting algorithm outside of the code affected by your bug fix. You could make the bug fix and refactor the code all in one commit. It seems like a good idea.

There are plenty of reasons not to make any changes beyond your bug fix. First, the code you want to refactor probably works and introducing changes takes that proven code and throws it back into doubt. Just because the refactored code works from JUnit does not mean that it will work from the UI or any other client. Refactoring can add new bugs to previously working code and lengthen the development schedule.

Second, the original code may not be as efficient as it could be, but then again it may not matter. There is a code that marginal or even major performance improvements are irrelevant. A common example is a sorting routine that gets called once in awhile and runs for only a couple of seconds. Shaving even 50% off that time may be interesting but not important since the routine doesn't get called often enough to make a big difference.

Third, the code may have been written that way to solve another specific problem. There are times when a somewhat slow algorithm is necessary because you don't have enough other resources to make it perform better. For example, you may have a memory constraint that doesn't allow you to use a higher performance algorithm. Refactoring this code without the constraint may cause a regression.

Finally, changing code out from under the owning developer may make him pretty annoyed. Small changes may be fine, however sweeping changes without consultation sends a deeply disrespectful message. You still have to work with the developer and maybe face him everyday.

Many of these beliefs come from previous jobs where people went in to working code and refactored. The best case was that the developer didn’t care that his work was redone. The typical case was that the owning developer was pretty ticked off and went to his friends and complained about the situation. The worst case, and one that happens a lot, is that working code gets refuctored with new hidden bugs and delays the delivery date.

I’ve been involved in a fair amount of team scheduling and oversight of late. The simple fact is that there are features to deliver and milestones to hit. Working code is generally considered to be good enough until a real problem comes along. This sounds like a cop-out, but the reality is that suboptimal code does not become a problem until the application is no longer within the constraints in the requirements specification or a customer complains. At that point it gets on the schedule and then it can be fixed in a thorough and thoughtful way.

Don’t get me wrong, I agree that most projects have some amount of bad code and there has to be a way to communicate that fact and fix it. Bad code is not restricted to poorly performing code either. I’ve seen plenty of functions that were pages long that needed to be broken up. Code with cut-and-pasted functions being repeated across classes with a common derivation are incredible candidates for refactoring. The issue is making sure that you are able to communicate a set of changes to the responsible developer and give him the opportunity to defend his choices.

It's great to suggest changes and to volunteer to do them yourself. It's good to be honest and to try to improve other developer's code in a constructive way. The important part is to talk to that developer before making major changes in order to understand why code was written in a particular way. It saves time, teaches and shows respect for your colleague's efforts.