Is it bad to have an “Obsessive Refactoring Disorder”?

Question

I was reading this question and realized that could almost be me. I am fairly OCD about refactoring someone else's code when I see that I can improve it. For example, if the code contains duplicate methods to do the same thing with nothing more than a single parameter changing, I feel I have to remove all the copy/paste methods and replace it with one generic one.

Is this bad? Should I try and stop? I try not to refactor unless I can actually make improvements to the code performance or readability, or if the person who did the code isn't following our standard naming conventions (I hate expecting a variable to be local because of the naming standard, only to discover it is a global variable which has been incorrectly named)

Answer 1

Refactoring is generally a good idea if it does not interfere with your schedule and does not negatively impact your work. Still, I believe it is a matter of priorities.

The goal is not to write the perfect code.

The goal is to deliver a working product.

The larger a project is, the less chances there are to keep the codebase tidy and perfect. Just accept some imperfection ratio and watch out not to cross it. Minor deviations are okay if they're not potentially dangerous.

Answer 2

Refactoring is generally a good thing; however there is a thin line to walk on if you're refactoring code someone else wrote.

Remember: prioritize. What's better, clean code or working code? At the end of the day, your customer won't care if you have two convertFooToBar functions in your code as long as it works. You can refactor for version 2 when you have time later.

However if the code has obvious problems/code smells like copy/paste methods, then your best option would probably be to discuss it with the original author and try to get them to understand why what they did is wrong and how they can avoid it in the future, especially if the code's performance is poor. If they refuse to change it themselves, make a note of it to change later (and maybe inform your boss about their stubbornness). Heck you could even file a bug report against it if it's really nasty.

If it's minor stuff like formatting, leave it alone until you have nothing better to do. In the meantime, make sure everyone has a copy of your code style guidelines.

Answer 3

There is a time and place for everything.

Please note that for many applications, code needs to be tested, and even tested extensively before going into production. Any change cause all tests to be invalidated!

This means that if you change any code you may cascade quite a bit of work needing to be done, which in turn may cost your employer quite a bit of money. Is it worth that?

When that is said, if you are allowed to, it is important to know what is allowed. In some workplaces the culture say that code fixing is everybodys responsibility and then you can just do it (NOTE: the test must pass both before and after), and others say that you need to discuss this with the person who authored the code (because it is most likely him or her who will have to fix it if your code isn't perfect).

Answer 4

It depends how/when/what you do.

I always try to refactor code as I come across it (but not while I know someone is actively working on it!! :)

What I try to avoid is pernickety/subjective types of refactorings, things that I may have done one way, but another would do differently, and that are EQUALLY valid.

Going too far with those things will only create conflict.

Answer 5

Kent Beck's mom said, of a baby: if it stinks, change it. I wouldn't call that obsessive parental hygiene disorder. Just the same, when you smell bad code, change it is a healthy attitude and a healthy practice. Keeping babies and code clean is better for us all.

Answer 6

Yes, anything mental related you describe and has disorder in it is a bad thing as it provoke difficulties.

Because the description of mental disorder:

A mental disorder or mental illness is a psychological or behavioral pattern generally associated with subjective distress or disability that occurs in an individual, and which are not a part of normal development or culture.

That's why you can live with some difference as long as it doesn't become a disorder. That is preventing from having a normal life, both in private or at work.

Therefore, Obsessive Refactoring Disorder is a bad thing.

Obsessive Refactoring can be a good thing. If it provide good and not bad effect in a project, company or individual mental health. Example: ADHD can be used for the good if it doesn't provoke the bad. In other words, you can use some disabilities at your advantage. Then it's not a disability anymore.

Answer 7

Obsessive Refactoring is not a bad thing, but it is the wrong solution to the problem. Instead of taking on all the problems in the code (and possibly angering members of your team) you should be working towards educating the rest of the team as to why refactoring is important. But this too has to be done in a way that will not cause too much friction.

If you have coding standards make sure they encourage the kind of coding that you are refactoring towards. See if you can work refactoring into your team's schedule and make sure that everyone takes the time to do so. Code reviews and gentle suggestions can also help. Refactoring and code quality are as much a part of the culture as they are best practices.

Answer 8

Some other thoughts I didn't see mentioned:

1. Don't fix things like incorrect naming conventions. Make the person who did it fix it and learn how to do it right.
2. Don't refactor someone else's code unless (a) it's your job or (b) the code is really bad. Premature refactoring is a prime time-waster and you risk being viewed as a pedantic time-waster.

Answer 9

In my opinion the cleaner the code the more likely it is to work properly. The less lines of code there are in an app the easier it is to maintain.

Answer 10

Depends on the nature of your project, but IMHO you should stop it. Any change in the code base is an opportunity to introduce a bug. Even if you have a comprehensive automated tests system, it may still not catch everything. Worse, it may create a bad work environment (like in the original poster's case) and than you have far bigger problems than a misplaced curly brace or an "ugly" function name. Live and let live...

Answer 11

I don't think that there is such a thing as too much refactoring. Especially if it is a large code base. You have to constantly put an effort into maintaining the code quality in order to avoid building up technical debt. This is how you can keep developing at a constant pace, instead of a gradually decreasing pace as you build up more and more debt.

Especially the case you mention, two functions that do basically the same thing. If the functions do this incorrectly, and you discover a bug related to one of these functions, the chances are the same bug is present in the other function as well. So code duplication should be factored out at any cost.

But what I think that you might be missing is that you should involve the team in this instead of just personally refactoring the code. If two functions do the same it is probably because one developer was not aware of what another had created (or even worse, one developer had forgotten what he had already created). When you discover stuff like that you should communicate it to the team, so the team as a whole grows to a higher level of understanding of the entire code base. Not everybody has the same capacity to grasp the entire code base, but everybody has the capacity to improve.

So my attitude about it is, keep the code base as good as can be, but it is a team responsibility, not an individual, to keep it there.

Answer 12

Ron Jeffries states that one of the ExtremeProgramming practices is to Refactor Mercilessly. He says mercilessly, not as needed or depending on the time available. This is the conflict between "clean code" (internal quality) and "duct tape" (schedule). Usually schedule wins and clean code is found somewhere else.

It is important that at least some developers on the team have the deeper need to do something about it even when the deadline is near. I agree with the answers above, not all developers must refactor all day, but (un)fortunately most don't care anyway. No danger here. You need to be stubborn to maintain this attitude. So "obsessive disorder" is the one and only attitude that keeps you doing it.

It's a good thing to have an "Obsessive Refactoring Disorder". Keep it and grow it. Maybe you can add some test infection as well ;-)

Answer 13

Preemptive refactoring is an opportunity to streamline code before some external force demands a refactor, such as a bugfix or feature request that is difficult or impossible against existing spaghetti. I would rather introduce new code on my own terms, rather than under duress. On the flip side, refactoring without unit tests can introduce new bugs.

I encourage communication. We use a code review process, so developers can point out when they think something was done poorly. If I refactor, I point out the changes to the original author and try to start a dialog.

Also note the distinction between something that is done poorly, and something that could be done better. "Better" can be subjective, and sometimes the code is good enough, i.e. neither perfect nor dangerous.

Answer 14

While refactoring other people's code might sometimes be necessary, I think refactoring a team-mate's code is just wrong.
Actually, having to look at some team-mate's code is a team smell.

That code is none of your concern. You are to use the interfaces provided and expect the implementations to work as agreed upon. The implementations are not yours to maintain, the interfaces are not yours alone to (re)design, because they are to be a team concensus.

Code refactoring is the process of changing a computer program's source code without modifying its external functional behavior in order to improve some of the nonfunctional attributes of the software.

Refactoring somebody elses code will not yield any measurable improvement on your side of the interface.

If you realize, that the implementation someone else is responsible for constitutes a performance bottleneck or a source of errors, then this is something completely different. This is external functional behaviour, that does affect your work and the project as a whole.
In that case, coming forward with fixes is not wrong per se, however you should try not to upset team-mates, to hinder them in their professional development (people grow from problems) or to add so much code, that you also need to take responsibility for the module in question.

I myself have experience with people who use refactoring other people's code as a way of procrastinating their own work. And I've often been witness to "optimizations" that simply add a number of assumptions to the implementations, making it fail in almost any corner case considered in the original version.
Now it's not that I don't like procrastinating with refactoring my own stuff or to dysoptimize my own code. What would programming be without shooting myself in the foot every now and then? But I am allergic to other people shooting me in the foot.

If desired, shared code ownership can be well established through pair programming or joint review sessions, but definitely not through sticking your finger in someone else's code.

You don't want spaghetti code. Why would you want a spaghetti team?
Separation of concerns is an extremely powerful concept, when trying to get complex systems to work smoothly. A team is also a complex system.

Answer 15

IMO, you should not stop to do it. But you should do it as not having top priority. Isn't there other development tasks that are to be done right now?

Also note that your managers might not like this because they do not see any change for the user.

Answer 16

It is a matter of good design. It is a lot easier to change one method; then to have to go through many different methods that are exactly the same (or have slight variations).

I am biased though, I tend to do the same thing.

Answer 17

Here's completely opposite view:

1. Refactoring someone elses code is very evil and not very professional. Who is going to fix problems with it once it breaks? Does the original author need to fix your refactoring mess, or are you going to move it to your responsibility? Either way, it's not very good. Hope you're not going to say you did it just because you don't like the indentation...
2. Refactoring your own code is ok. You admit your original design was not your best work, and you want to try again on your own time. Just try to make it not change the deadlines for the whole project, and you'll be fine.
3. There are exceptions to rules (1) and (2), and one such exception is when your manager tells you to rewrite someone elses code. Then the manager is responsible for distributing the fix requests to correct persons and there is no reason for programmers to admit their code is bad.