A recent lunch led to a great conversation about refactoring other developers’ code and when it is reasonable to do so. Generally, I believe it's unwise to refactor other developer's code without talking about it first. Especially when the code you want to refactor is a peer or a lead. We came to a conclusion that it is fair to refactor without comment when you are under a tight deadline and its the only way you can see to fix the problem.

First, I really don't like refactoring someone else's code if that person still works at the company. It can be seen as a vote of no confidence and a real ego bruiser. Even the worst developer still takes pride in his work and making that person upset or angry does not make your project go more smoothly. Discussions, mentoring and positive criticism will often get the point across and the person learns how to write better code.

There are still times when it is reasonable to go ahead and refactor the code without warning. Let's say you are working the weekend to hit a deadline and you have to make sweeping changes to someone else's code and you can't get in touch with him. This scenario happens in somewhat older projects where the old code has been patched over and over again. It was good code once, but the application has grown beyond the algorithm. The best way to fix it is to rewrite it into clean code that fits current requirements.

Hopefully you have a good understanding of what the code is supposed to do and can write one or more replacement methods that adds new functionality while faithfully recreating the old. It is very easy in these situations to regress. At this point, the best thing to do is to test, commit the code and then tell the developer responsible for code about what you did and why. Most people will be gracious and happy that you fixed what was probably seen as a burden. Other developers may get really crabby or even yell about the changes. However, hitting deadlines is often more important than personal egos and real pros are often ready to accept that.

Another scenario is when you are the lead developer and you have several people churning out code for a set of features. Sometimes you have to integrate all that code which often requires some measure of refactoring. You may see, for example, that an algorithm is simply incorrect and will not operate correctly. If you have time, you should bring it to the attention of the responsible developer and communicate the problem. If a deadline is looming then you are probably better off just making the changes and telling the person about it later.

Clearly, there are two common themes here. The first is that communication is critical, whether good or bad. The idea is to drive home why you made changes to explain your point of view and hopefully get buy in. Its also the opportunity to find out whether your change is really appropriate. Sometimes the refactored code is worse because it introduces regressions in other concealed areas. The owning developer is often the only person that knows about these other dependent components.

The second theme is time. The tighter the deadline, the faster you have to move. Missing deadlines can impact sales, support, documentation, QA, salaries and staffing. This may sound a little over the top, but many customers only budget and buy during certain times of the year. Missing a customer's budgetary or buying cycle can mean missing an entire year's worth of revenue from that customer. Small companies with few customers are very sensitive to losing revenue. Consequently, slipping a date because you didn't want to modify someone else's code is usually not helpful.

Communicate and delegate when there is time and refactor carefully when there isn't.

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.