"it works-don't touch it" and continues engineering

Question

Sometimes I work with bad smelling code. Yes, there's bad code out there :) I'm not talking about design problem but about much more simple things like:

• messy indentation
• non consistent use of empty lines
• a big banners introducing functions, which just repeat the function/method names
• non consistent name conversion, even in the same line
• badly stuctured function logic (e.g. redundant checks, repetitions etc.)
• tightly coupling of data and logic
• and much more

An example just to visualize what I'm talking about (C++)

class MyClass 
{
   public:
   /*******************************************************
      | constructor                                             |
   ********************************************************/
   MyClass()
  {
  }

   /*******************************************************
   | destructor                       |
   ********************************************************/
   ~MyClass() {}    

   void Foo(char *Name, const char* Type, char* sub_type);
  };

I personally dislike to work with such code, it just discourages me to write good code, discourages to think about more high level obstructions.

Mostly it drives me crazy when coworkers developers don't understand when you say to them that something is wrong with the code - they just happily continue to support the mess.

Unfortunately managers are not interested in "beauty" of code, don't see the impact of bad code in long term. When there's a spare window nothing is done to clean/improve code - this never scheduled as a task/project. Sometimes to my complaints I hear a well known response: "it works-don't touch it".

I think this is a evil, bad practice. Not handling the problems is like a fat, that makes you clumsy, not competitive.

What to do? Constantly complain to deliver the problem? This will only gain you a bad name and managers won't understand what are you so not happy about. Changing code in your free time? Also not good - you can not take a risk of introducing a bug?

Writing good code should be an integral part of development process.

Do you have the same dilemma? What did you do to improve the situation?

Update:

The subject language is C++.

A new piece of code is written in an uncontrolled form, with emphasis on functionality. No code reviews or coding standards are currently affecting it (if they ever did). I want to change the situation, but just don't know how.

Update 01

Let's say that we want to perform only "syntax" refactoring on a big code base. For example fixing indentation, renaming variable (by using refactor tools of course - not by hand :))). Is it worth to do it and probably introduce a couple of bugs?

Answer 1

Like it or not, refactoring code smells can introduce faults in your code. I'm not at all advocating poor code, but I am saying that as developers, we must acknowledge that sometimes it is more cost effective (and therefore better for our jobs), to leave that messy code alone.

Answer 2

In my opinion, you have to stop it before it starts. Once you allow crap into your trunk, you're gonna be stuck with it. It starts with an attitude about the quality of code in your product. I don't let anything into the trunk that is either personally reviewed by me or personally reviewed by a small group of architects/developers that I trust.

The burden falls on me (us, the trusted reviewers). I actually look at the code before I go into code reviews. I give written feedback. If I don't like it, I reject it. There are plenty of good examples in our code to learn from. And, frankly, if I see repeated attempts to get bad code past a review, the person repeatedly trying becomes suspect in my mind.

Compromises have to be made sometimes. We specifically call out when we are making a compromise and the reason for the decision to do so (usually time to release). That gives us targets for refactoring in the future. But, I don't compromise on what I consider to be "crappy" code -- I just compromise on what I consider to be sub-optimal (or sub-general) implementations.

This attitude is particullarly important if you want your company to be an acquisition target sometime in the future. I used to conduct due diligence code-base reviews for company acquisitions. I can tell you from personal experience that bad code can prevent a company from being acquired.

Per comment: Do some research on ISO/IEC 15939. Look up Cost of Quality (COQ) and Cost of Non-quality (CNQ). Here's a good book if you're interested.

Answer 3

You didn't mention what language you are talking about, but I found out that having coding standards and a plug-in to check them as you go is the best way to at least get well formatted code. If you can show that there is an acceptable standard and the complaints a good checker generates you may be able to make your case.

BTW I use C# and I have to confess that my own code got a lot better looking after I started using StyleCop and a companion plugin. Plus the metrics IDEs are now able to give you help immensely in if not reducing at least taming complexity.

Answer 4

I would say in this situation, you do YOUR best and write the best code YOU can, and hopefully your fellow developers will follow suit. Unfortunately, $$$ makes the world go round, and if you refactor code and introduce new problems, people are not going to be happy that you messed with the code for "beauty's" sake.

Answer 5

it works-don't touch it

I refactor it if and when I have to touch it. Coding is typically like this:

1. Refactor existing code (without changing its functionality) to ready it to accept the new code
2. Add new code (which implements additional functionality).

If there's a section of code that I merely have to read (without my even being required to prove that it's correct) then I won't change it (except possibly, with permission, simply reformatting it).

The cost of refactoring is built in to the cost of developing the new feature (i.e. I don't ask permission/budget to just refactor without developing any new feature); also I'll feel more responsible for the correctness of code after I refactor.

If I need to justify refactoring, then I think, "I need to know that my code is correct, and if I find it difficult to read/understand then that's difficult for me to know."

Answer 6

In reference to:

• messy indentation

You don't mention the language or IDE you use at work, but if you're using Visual Studio, quickly hitting Ctrl+K, Ctrl+D formats your document and fixes all the indentation problems (as well as making the spacing consistent). I would imagine that Eclipse has a similar feature, but I can't speak to that for sure.

I'm not sure if I would go so far as to commit code that I just Ctrl+K, Ctrl+D'd, but you can certainly use it to make things easier on you when you have to read it.

Answer 7

You answered it yourself, I think: "Writing a good code should be integral part of development process."

Codng conventions that everyone follows, peer code reviews would go a long way, and if the process is fixed, then gradually the code will improve.

Otherwise, ad-hoc rewrites only present a risk.

The indentation can be fixed using indent (I assume C), but if you have multiple branches to merge with, it in itself will introduce the gross pains at the time of change - again, leading to bugs. And yes, even the "beautification" changes are error-prone. Humans are not great at mechanical tasks, and I've seen the occasions where a human error during such activity introduced a new bug.

The only comment I could make - try to quantify how this situation affects the quality of the shipping product, and then the managers would listen, if you have solid figures to show.

Answer 8

You need to pick your battles. Personally I dislike bad spelling and grammar in comments, and sometimes I fix it, but mostly I don't because:

a) The guy you fix it for may take it badly; like you're trying to get one over on him b) Risk of accidentally breaking code and the time it takes for someone to review my changes

And that's just comments. If you're changing someones code you better make damn sure there's a sufficient number of unit tests to run on it. Even then, there's nothing like field tested code in terms of solidity. Any change to a live system, however minor, could break things you don't understand.

IMHO you should only fix up code if there's a specific project goal involved.

If you're revisiting some code to improve performance, add functionality or perhaps integrate it into another project, you're effectively losing the value of a lot of the field testing it's had already. So yes, refactor, clean up, run a code formatter, whatever you need to.

Some of the things on your list are more serious than others though. Messy formatting makes me think the programmer is careless, but I may be wrong about that. On the other hand when a large block of code has lots of redundancy or a collection of classes are tightly coupled, that has a real nagative affect on the quality of the system as a whole.

So yeah, pick your battles, avoid changes that don't win you anything, and don't step on toes is the summary of my answer.

Answer 9

I think this article on Philip Dorrell's blog sums up one approach very well.

The TLDR version is that if the code is known/proven to work, it's value far exceeds how pretty or ugly it is. If you change the code without testing it, you effectively decrease its value, even if by all visual means it appears the same.

I highly recommend that blog article if you've got 5-10 minutes to read it.

Answer 10

The first five things you list are essentially aesthetic. What exactly would be the point of changing them for their own sake? There isn't any other than that warm, fuzzy feeling of the code then looking nice. You do that sort of thing when you're changing the code anyway, never for it's own sake.

Even then completely reformatting a file tends to be bad because you make it more difficult for someone else to figure out who changed a line of code and why (since your name will be the last change for probably every line). Source control history is more important than aesthetic changes.

Tightly coupled is a design rather than aesthetic issue. But if it works, changing it is something you do when you need to change it anyway.

Fact is, code could always be better (particularly that written by other people!). Get used to it. At best you should be focusing on how to avoid such "problems" in the future by, say, having a decent code review process.

Answer 11

I like to refactor bad code whenever I see it, perhaps I like a bit too much. Whenever you edit code there is slight a chance that you introduce new errors in it, and that has of course happened to me a few times also.

If you have to touch the code for any reason, you should clean up and refactor it if it's needed. If it smells, leave it a bit less smellier each time you visit.

If the code works and there is no other reason to change it, perhaps you should leave it as it is. Even if it smells, it might still do a good work. Code doesn't rot, it doesn't get worse by time. On the contrary, if it stands the test of time without causing any problems, it's more tested than any new code that you can produce.

So, clean up and refactor by all means, but not just for the sake of it. :)

Answer 12

I've dealt with this problem from time to time, and I can sympathise with your entire post. You have to be careful to pick and choose your battles with this, as you can quickly build up bad will with the people that don't agree. Here are some options:

1. If it's just commenting or minor code-style, just leave it alone until you have to actually go back to the code for a change.
2. If you have a code-style guideline, and there is code that is non-compliant, I would bring it up during a code review. Still, going back and fixing other people's code, just for style will probably piss them off.
3. If there is a single person on your team that is just making zero effort to meet the comply with the agreed coding style or adding poor/low-quality comments, then you might be able to get the remainder of the team bought in on helping you clean things up every now and then. If it's just one person, hopefully they will get sick with people changing their code, and try and improve. Be careful though, cause even that one person can probably complain enough to the managers and have something blow back on you.
4. Ok, so now to the point where there is more than just bad style, but poorly implemented code, sloppy, or whatever. Probably if it's functionally correct, you should just leave as is and move on. I would just note it in a journal and see if you can eventually get by-in from the rest of the team to fix it later.
5. If the code has some faults, and you can see abstract errors, that may be testable, you could send off the test case to the owner, and maybe have the fix the issue. Again, this might start earning you some ill will with the person if you continue doing this, so be careful that the error is relevant to some degree.
6. If the code is so flawed, that you feel it will not be acceptable for long term, then I think that point you need to have a face-to-face with the developer. It is likely that they won't agree with you, but if they do, maybe you can agree on a plan to fix it.
7. Assuming the developer doesn't agree with you, I would at this point have a talk with your team lead and see what they think. If they're going to support you then you should move forward with it, and bring the team lead to back you up with another meeting with the developer.

8. If your team lead doesn't go for it, then I think you need to just drop it, and see how it plays out in the long term. From here I think you have some big choices to make. If the team lead and/or other developers never support you then:

   • You either need to accept the fact that your being too critical to your co-workers, and need to relax, the code will never be perfect.
   • The other case, is that you're in a shop that will never be able to output a high-quality product. You need to decide if you want to continue making an investment with the group. If the answer is "no", then start updating your resume.

Answer 13

I just wanted to pose the opposite argument from that which everyone else here seems to take. Sloppiness anywhere in your code is a dangerous thing and allowing messy and inconsistent source code to go unchecked can lead to some massive problems down the track.

In a previous job I had, there was no clear style guide on things like variable or function naming, meaning that you'd have to continually jump into the source of your libraries to see whether it was get_id(), getid(), getId() or getID(). Given that this code is being continuously worked upon and maintained, it's very important to avoid these situations.

Developing a standard style guide laying out naming conventions, indentation style, etc will go a long way to improving the readability and maintainability of the codebase.

Fixing up the messiness of what you already have should probably be done incrementally so that a) management doesn't think that you're not doing any work, and b) any introduced errors can be ironed out quickly.

Answer 14

Well, since this is kind of an opinion piece anyway, I'll throw in our approach:

When the problem code came from our dev group-

• If you see misspellings in comments, or the wrong usage of words, ("they're" vs "there", "it's" vs "its", etc), fix the problem and commit the one line change. Documentation is frequently auto-generated, and these mistakes can propagate out beyond programmers. One doesn't wish to appear amateurish in front of management, test, or worse, customers.

• On the other hand- if it's anything that gets compiled you don't touch it, unless you're already having to work on that method/function.

  If you're really concerned and the person who wrote it is still around, you can contact them about it. Most people remember when they wrote something that sucks, and usually won't take offense as long as you approach it in a non-threatening way, such as, "I've been looking at your fooMeter code, and I was wondering if you could help me out with how it works..." Fairly often the code is bad not because they don't care, but because they were in a hurry to reach a deadline, etc.

  On the other hand, if that person is gone and the code works, the problem would have to be egregious to justify the risk of touching it.

If the code came from one of the other dev groups we interface with-

• If it's not part of changing an interface which we've specifically arranged to change already, we don't touch it.

After having read some of the other replies, I'm grateful that we don't have any egomaniacs who freak if someone changes their code. No one really seems to mind the first bullet, (I know I don't mind people changing my comments so long as they stay relevant). The other two bullets just maintain everyone's sanity.

Good luck!

Answer 15

Just so long as you are not fighting windmills alone against everyone else the best way to tackle this I found is to make refactoring part of the culture. That is, whenever you are going in the code for a bug fix, for a new feature then take an extra hour to think it through and place the changes in the grand scheme of things (this implies though that you, as a dev team, have one).

Then bring the appropriate refactoring, if necessary.

Your job as a developer is to produce good quality software, for this you (as a team) must be in control of the code base and this implies revisiting every once in a while what has been done and steer it in the right direction.

Whenever you confront the company with this code quality problem they will not care.. in fact most of the time they will see this as a waist of money, and so should they. With this in mind try to convince someone to pay your salary for rearranging useless comments !

When you go in a garage you see the difference right away between a good shop and a messy shop. Not to say the messy shop cannot do good work, but chances are if the place looks like a dump there is a fair chance they will produce garbage in direct proportion to the mess at hand.

Treat your code base like a garage. Your job is to keep the place clean, keep the tools clean and in good working condition, keep the stock room well stocked with everything in it's place and as a team you will be a lot more efficient. You should not have to ask management permission to clean the place up, it should be part of the job.

Unfortunately management cannot see the shop, as a result many coders never cared and have been very happy in working in the smelliest pig stale with cruft and crusts everywhere.

When you make estimates to management include in it time to refactor the needed parts, DO NOT just refactor for the hell of it, it will invariably come back and bite you in the ass and it will almost invariable be a waist of money for the company. Goal is that

1. Make a plan, stop the insanity... create an overview of how it should be, of the overall architecture of the system, of the layout of the code base etc and publish it in for all devs to see.
2. Make it fit Every changes made to the system must be so that it fits in the grand scheme describbed above.
3. Nobody likes an obsessive cleaner Only change things for which you have been asked to change by the people paying your salary. Unless of course you like giving your time away for free and get shit for it.
4. Rework the plan Revisit often the grand scheme of things and decide together if it needs to be refactored. You will eventually fall in a case that was just not part of it, first refactor the plan, then refactor the system.
5. Think it through Never give time estimates on a cafeteria napkin, to them (management) this napkin is a commitment and they will commit others to it, in other words they will hold you accountable for it, take a few hours to analyze the requested changes, draw it out and integrate in the grand scheme of things, take a quick look at affected code see how bad it is. Then multiply your estimate by three for all the stuff we always forget (tests, docs, build, reports etc...) unless of course your estimate procedure includes these artifacts. This represent the real estimate of the requested change. At first they will jump but they must get used to the real cost of things.

A corollary of 5 is keep your estimates and compare with reality... you will be surprized on how off you are the first few times.

I hold to these basic rules and became MUCH more productive, I deliver on time, as promised and very seldom customer complain about bugs. I found that not doing this (as I was doing before) was like working on credit, at one point you have to pay-up... plus the more sloppy you were in your rush the steeper the interest rate !!! when viewed like this how do you think managers will react when you tell them that you had to borrow 3000 man hours at 60% interest to get the job done and now they must pay.

Anything else is like running like headless chicken spewing blood and gore all over the place...

Answer 16

Kind of reminded me of this posting I came across the other day:

http://sites.google.com/site/yacoset/Home/so-you-ve-just-been-hired-by-an-it-department

Answer 17

I was recently assigned a task that involved taking a legacy code base that talked to an integration database and modifying it's behavior to satisfy a mostly new set of specs.

The code base exhibited many of the issues bulleted above, as well as the use of magic strings to couple JSON objects on either side of a service, commented out code that didn't have a comment as to why it wasn't just deleted, and no automated test suite.

I rolled up my sleeves and got to work. I started wrapping the system in tests, outside-in, such that once I was comfortable with that I'd covered the expected behavior of a larger piece I was able to break them into smaller pieces and wrap those in tests (and make them pretty while I was at it). Essentially pinning down the system so I knew I wouldn't be breaking anything.

The entire process took about a month, the first half was almost entirely pinning down and refactoring, while the second half was all new functionality (which of course is all covered with automated tests).

I can't imagine making all the changes that I did with the "it works, don't touch it" mindset. I actually consider that to be technical debt, and it will (at some point) impact productivity.

Answer 18

It may be irritating, but for code style consistency is much more important than quality - even if it's consistency in failure.

It makes absolutely no sense to change style of portions of code without enforcing that new style on the whole team and whole codebase. So, either convince your team and managers that good style is important - or get over it and learn to work with code you have. Don't try to fix random snippets yourself.

Besides, "it works-don't touch it" might well mean "stop messing with other people's comments and get back to your job".

Answer 19

Quite a number of answers has been posted already, but I'll try nevertheless to add something useful.

First of all, refactoring of a big project is in most cases an economically unviable initiative. It will take significant amount of resources which will likely bring no ROI. It could last years! The internals may become better, the developers may get happier but that's all. It will not add more sales as the customers are not going to see any difference.

Just yesterday I read a Joel's article: Hard-assed Bug Fixin'. Not only has he a lot to say, but is a great writer as well.

As a software developer, fixing bugs is a good thing. Right? Isn't it always a good thing?

No!

Fixing bugs is only important when the value of having the bug fixed exceeds the cost of the fixing it.

The same even more applies to your situation. A bug can transfer its effect via customer dissatisfaction on to your sales and ultimately the company operation. How exactly do you expect an inconsistent coding style to have that same kind of effect?

Now the important part. You will have to refactor if the cost of maintaining and of further development of the application in its current state is higher then the cost of "fixing" it.

Different styles in code are almost inevitable in a big project with many people that contributed along the path. All of them have different experience, different conceptions of right and wrong, they set different priorities. You now see their "signatures" in code. The only way to avoid it is to write a consistent set of rules and strongly enforce them on every single developer from the very first day of the project. Choosing the right people for the job from the day one is also important. If you don't select people carefully but just pick up anyone who could do the work and not cost too much, you will get what you deserve.

The biggest issue here however is that you take the project personally. With all due respect to your attitude, it is not your project, it is not your business. I strongly believe that an employee should not care more than the management about how you do work (well, unless he is suddenly paid more than them). Many good developers have experienced the exact same frustration at work. There are three possible outcomes:

• you can't help it and leave
• you develop an apathy and a global loss of energy which may damage your health. There have been many people appearing in similar questions here who reported health problems developed at work that lasted for prolonged periods of time (months to years).
• you stop taking it personally and just do it for paycheck

I suppose I'm more or less in your situation. I also do not like the way the things are, the code itself is also badly structured, with my direct participation we brought one component to almost unmaintainable state - you change one little thingie and everything falls apart. The component itself is originally a quick solution to a sudden customer wish which since has enjoyed the style of development known as hacking things together. I also couldn't do anything about it. I was given the fuzzy instructions to come with the suggestions to improve it after I complained, but since I (and other people) have the other tasks coming in all the time including adding more to that component, I do not see any options to achieve that.

Now the hammer (as though it was not enough already). The more messy and unstructured the code is, the less are your chances of correctly extracting the complete set of rules and business logic from this part of the code. Which consequently means the chances of you refactoring this part without introducing any new bugs are equally low. There may have been several hundreds of bug fixings and customer wishes implemented, one layer over another, some of the documented, but most of them in the heads of customer employees (some of them may have already forgotten them or left the company) of your company developers (some of them already lost the track of changes or long since left the company). I see you're using C++, then this adds to the complexity of your task all memory allocation/freeing policies that may not be obvious to you now but will emerge at the first opportunity in your refactored code. So, technically evaluating your chances of doing it right, well, you see what they are.

My advice to you:

• What can be done? Not much, it is likely too late to change it on the big scale. Just do what you do the best way possible and maybe improve little things now and then where you can.
• Detach the personal bounding to the project and only consider it a job. Collect experience, write a blog about it.
• Whenever you can't stand it anymore, leave. The mental stability and the health state are more important than anything else.

---

Another great article about refactoring:

Things You Should Never Do, Part I

I'll cite for those who are not going to read it... :(

There's a subtle reason that programmers always want to throw away the code and start over. The reason is that they think the old code is a mess. And here is the interesting observation: they are probably wrong. The reason that they think the old code is a mess is because of a cardinal, fundamental law of programming:

It’s harder to read code than to write it.

and

The idea that new code is better than old is patently absurd. Old code has been used. It has been tested. Lots of bugs have been found, and they've been fixed. There's nothing wrong with it. It doesn't acquire bugs just by sitting around on your hard drive. Au contraire, baby! Is software supposed to be like an old Dodge Dart, that rusts just sitting in the garage? Is software like a teddy bear that's kind of gross if it's not made out of all new material?

Back to that two page function. Yes, I know, it's just a simple function to display a window, but it has grown little hairs and stuff on it and nobody knows why. Well, I'll tell you why: those are bug fixes. One of them fixes that bug that Nancy had when she tried to install the thing on a computer that didn't have Internet Explorer. Another one fixes that bug that occurs in low memory conditions. Another one fixes that bug that occurred when the file is on a floppy disk and the user yanks out the disk in the middle. That LoadLibrary call is ugly but it makes the code work on old versions of Windows 95.

Each of these bugs took weeks of real-world usage before they were found. The programmer might have spent a couple of days reproducing the bug in the lab and fixing it. If it's like a lot of bugs, the fix might be one line of code, or it might even be a couple of characters, but a lot of work and time went into those two characters.

When you throw away code and start from scratch, you are throwing away all that knowledge. All those collected bug fixes. Years of programming work.

Answer 20

You use source control right ?

Do your devs without touching the "ugly code" commit it with a label describing your dev/functionality.

Then once the code is working, do the clean up, test it (another good reason to have at least somes automated non regression tests), commit it labeled "clean up", if an issue occurs and someone unfairly blames you, you can just get the non cleaned up code back up and prove him wrong, if it's your fault just tell him that it took 5 minute to get it back up and it shouldn't be a problem.

Also don't try to refactor big chunks of code at once, do it lil by lil, start by harmless refactoring, indentation, remove useless comments, put duplicate codes in methods, make it more readable, and if you do it right, people might even follow your lead if you're good with communication. BUT make sure to not take it too fast (people might not understand wtf you doing and blame you because they don't understand).

Answer 21

I think the most interesting part of the question/statement is: .. it just discourages me to write good code, discourages to think about more high level obstructions...

There's should be more focus on the psychology of programming and how to improve, because that's what good coding is mostly about, not moot syntax or patterns. If a person is not in the proper zone FOR ANY REASON - including having to work with badly formatted code - the overall productivity is decreased...a good programmer can make COBOL sing and a bad programmer make Python look like moldy pizza. I think someone prior posted a link to Freakanomic's point on broken windows, but its very relevant in the development world - great programmers can overcome their surroundings, most of the time, but if we're surrounded by garbage we (most of us) eventually contribute to the pile. Always fix what's broken...

http://en.wikipedia.org/wiki/Fixing_Broken_Windows

Answer 22

Periodically (i.e. on a slow day - does anyone have these) or as you touch code, make changes to the offending code that don't change the code: documentation, formatting, and things that are suggested by tools like Resharper's quick fixes. Use tool-support, don't do it by hand.

Deeper changes should also use tool support.

And my philosophy: a dirty room may not be 'broke' but it still makes sense to clean it up. It makes it more pleasant for the next person to come in. That - and they won't have to spend as much time in the clean room, because whatever of value is there won't be hidden under the dirty laundry. This translates to greater productivity - albeit hard to measure.

Answer 23

IMO every checkin should improve the codebase. Which means that if I'm fixing a bug in some code, I'll usually try to improve the code in that area at the same time as fixing the bug.

Do I embark on a "I'll fix everything now" exercise? No. I simply tidy up the bit that I'm working with.

How do I know that I haven't broken the implementation? We have unit tests. Refactoring without unit test coverage is like walking on broken glass (or broken windows, to make use of a different metaphor), i.e. something to be done very carefully since it can easily get painful.

And if there are no unit tests? Then write some. There will be never be any unit tests until somebody writes some. Yes, it takes courage and effort to be the first one to unit test a new area of code, but someone's gotta do it.

But isn't all this just extra cost? It's not just extra cost, since I've now got some unit tests in place. And so the next time a dev accidentally introduces a bug it will be picked up by the build server and not by the tester / customer. This will save us money since the cost of a bug fix increases exponentially over time.