The Design of Software (CLOSED)

A public forum for discussing the design of software, from the user interface to the code architecture. Now closed.

The "Design of Software" discussion group has been merged with the main Joel on Software discussion group.

The archives will remain online indefinitely.

Don't touch that code!

How many times have I heard this. Someone who thinks it's more conservative to leave bad code in place because "we've been debugging it for months and we can't risk breaking it".

I would always advocate getting bad code under test to enable larger design changes via refactoring. Recently I met some opposition to this. Some design changes I made were seen as 'radical' even though I made them in small, iterative steps with supporting tests.

For me, the "don't fix it if it ain't broke" attitude is the reserve of cowboy coders. What do you think?
Chris Steinbach Send private email
Thursday, September 08, 2005
If you don' have anything new to do then it makes sense.
If you find a refactoring opportunity take it.
But, in general, how you know you are improving the code and not breaking it more?
son of parnas
Thursday, September 08, 2005
==>For me, the "don't fix it if it ain't broke" attitude is the reserve of cowboy coders. What do you think?

While true, you see it a lot with "cowboy coders" -- I see it even more with business managers keeping an eye on the budget. Many times "don't fix it if it ain't broke" is a perfectly valid statemnet and will save significant $$$ in expense. It's a judgement call. How much will it cost to "fix" it, and what's the expected benefit (and later, measuring what the *actual* benefit it).

Just because it may be ugly/brittle/poorly-architected/whatever, doesn't necessarily mean it's cost effective to change it. Every minute you spend "fixing" it, comes with a cost. Is that cost justified?
Thursday, September 08, 2005
If you can quantify "bad code", you shouldn't have a problem.  In other words, if you're labeling code as "bad" because there are known bugs in it, then do what's necessary to fix it -- including refactoring if needed.  Of course, if you can quantify a problem, you shouldn't have to worry about anyone applying "if it ain't broke", because you've proven it *is* broke.

On the other hand, if it really *isn't* broke, then it's cowboy coding to go in and "fix" it despite the fact that the code is working.
D. Lambert Send private email
Thursday, September 08, 2005
D. Lambert, are you suggesting that there is no such thing as "bad design" if code functions according to external requirements? This is probably another discussion: the difference between, and significance of internal and external quality.

I'm assuming here that internal quality matters. You may have another opinion and I don't mind exploring that.

As for quantification, there are code metric tools that will give out warnings when you have a method with too many lines, or a class with too many methods. But even the most complex and involved metrics only quantify what is plain to see in a few seconds. Bad code isn't difficult to identify. Bad code hints at bad design.
Chris Steinbach Send private email
Thursday, September 08, 2005
There are a couple of dimensions to this. One economic: is making a quick fix cost effective? Risk is another aspect: is a minimal change less risky than a more substantial design change? Then there quality: does the internal quality of a software product matter?

From an economic perspective, my initial thought is that the "don't fix it" mentality can only have short-term economic advantage. If we try to optimize for cost-effectiveness in the short term, then we won’t have time to sharpen the saw; we’ll be way too busy trying to get that tree down within the hour.

The impact on risk is cumulative. For each small change we make without considering its impact on design, the risk of introducing a bug in the next change increases. At the same time the cost of analysis increases with the complexity of the code. The existing code no longer supports further development, it actually hinders it.

The last question I'll leave open for now, or maybe start a new topic: does the internal quality of a software product matter?
Chris Steinbach Send private email
Thursday, September 08, 2005
Chris -

No way - there's definitely a difference between good design and bad design.  All I'm talking about is being deliberate about when to address that problem.

If you've got to make changes to the code to fix a bug or add a feature -- fine.  Go ahead and do the refactoring.  But if you're going to fix the code because the design isn't elegant and you *might* have to maintain it someday, you're borrowing trouble.

Think of it in terms of a black box.  If all you've got is external evidence (functional tests, performance tests), and that evidence shows that the code is working, why would it bother you?

If you're talking about affecting and controlling the design of new code, that's a different story, and I agree that time spent getting the design of new code right is time well-spent (same thing applies to significant maintenance).  Time spent worrying about the design of code that's already time-tested and proven is not well-spent.
D. Lambert Send private email
Thursday, September 08, 2005
> there's definitely a difference between good design and bad design.

If only people could agree on what that is.
son of parnas
Thursday, September 08, 2005
"Just because it may be ugly/brittle/poorly-architected/whatever, doesn't necessarily mean it's cost effective to change it. Every minute you spend "fixing" it, comes with a cost. Is that cost justified?"


Even Martin Fowler and Kent Beck go from this position.  They see refactoring as a continuous process that should be done when the need arises... which doesn't necessarily come.

I try not to refactor a piece of code unless I have to make changes to it.  I then add testing to ensure that however it worked before, it works the same or better afterwards.
KC Send private email
Thursday, September 08, 2005
You mention Fowler and if there's anything you take from what he says imho it's this:

I'm a professional Software Engineer.  I will do what I think is best in order to implement this feature\fix this bug.  If that includes refactoring this class\method\package I'll do it.

That doesn't mean you re-write every piece of code you come across.  It means you evaluate case-by-case to get to Y from X what do I need to do.  I need to understand what's going on; understand where the crucial issues lie and improve the functionality\codebase.

That's it.  'nuf said.
Raymond Kroeker Send private email
Thursday, September 08, 2005
My shop constantly has this problem at work.  I think we have settled on the following rules:

-If it works and is reasonable to maintain/change, leave it. 
-If it can be refactored in less time than it takes to make your change w/o refactoring, change it. 
-If, due to new design requirements or information, the design is flawed and can be fixed without incurring enough of a time hit to alert management, change it.
-If it is a slightly bad design but the change would require changing virtually every file in the entire program, leave it. 
-If it is possible to adapt a new style of working, instead of changing the architecture, adapt a new style. 

Often, I am in one of the last two situations.  The architecture of my current day job project is very "functional" vs. "object orientated."  That doesn't mean it is necessarily wrong, just that it is different from how I might have done it in a vacuum.  The problem is that nobody codes in a vacuum.
Joshua Volz Send private email
Friday, September 09, 2005
I remember a piece of code that used to enable and disable various controls conditionally.
It was in place for long. The issue regarding this was reopened around 13 times but since it was there for long no body dared to change the logic completely.
But when I could not tolerate any more reopening of the bug. I just rewrote it such that it expresses the actual senerio in a plain and simple manner.

And BINGO! it was never reopened since then.

In yet another incident, there was requirement to convert a search query supplied by the user into an sql query.
The implemented solution was lame. They just kept regular expressions to parse the supplied query which obviously was a logic flaw. The issue kept on being opened again and again untill I just revamped it and wrote a LALR parser for it. It took me four hours it implement but the bug has not been reopened since then and it has been around 2 years when I fixed it.

I guess these examples give a picture about what does the *BAD CODE* or *BAD DESIGN* mean and how costly it might prove to have bad code in place since conservatives have no guts to revamp it.
Alpha0 Send private email
Friday, September 09, 2005
The truth is somewhere in between the extremes. Refactoring, if done in small steps, many times may get accepted. But again, in the real world, a lot of code does not have enough unit tests, enough integration and system tests. Even if they exist, carrying out system validation again and again because of a series of small changes is very costly. If you don't do system tests, mere passing of unit tests is no guarantee that your changes are sane. So, naturally, anyone would be reluctant to change an already working system.

But the problem with code is, that we tend underestimate its lifetime. A piece of code, if it works, tends to get re-used across different projects, inspite of the fact whether it is designed for re-use or not. Different projects have slightly different requirements on this piece of code and what you see is that this code slowly starts bloating with special conditions and over a period of time becomes klunky, inefficient and error prone and a nightmare to maintain.

Like all other social orders, it takes courage to propose refactoring/redesigning a piece of code especially when it is working for long (with all its known shortcomings).

--Hemanth Sethuram
Hemanth Sethuram Send private email
Friday, September 09, 2005
I agree with pretty much everything that was said here - "if it ain't broken" etc. Though there are more sides of a medal...

Most harsh one is when developers have to maintain a code, which makes them sick, the ramifications to company may vary from decreased productivity to necessity of finding and training replacements. Developers' loss of happiness and enthusiasm will also demand for better people and project management skills from team leads and other managers. Experienced professionals learn to cope with solutions they do not approve, but they also learn to share or avoid responsibility for problems caused by those solutions, which also adds to manager's burden.
Friday, September 09, 2005
+1 for what D. Lambert said. If it's working w/no critical bugs & I can simply not touch it, I won't touch it. If you have a lot of critical bugs to fix, or features to add, that's a good time to schedule cleanup/refactoring because doing either of these to crappy code will cost a lot of time.
John Foxx
Friday, September 09, 2005
How do the other people who will have to interact with this code feel about your proposed refactoring?

War story time: years ago, I inherited responsibility for a certain module (tens of thousands of lines of code) after the original developers had left the company.  We had another group at a remote site who were tasked with coming up with a long-term replacement for this module that would be plug-compatible with our module, but would include new capabilities and could be reused in other tools.  Meanwhile, I had responsibility for maintaining and extending the existing code to meet the short-term needs of our tool (which couldn't wait for the completion of their project).

I should note, in view of what happened, that I have the utmost respect for the abilities of every member of that other team.  Their project was one of the best-run projects I have seen in the industry, and their tech lead was an extraordinarily talented individual.  Never the less...

They decided that they needed to refactor the existing code to make it "more understandable" before continuing with their project, so that they could make sure they replicated all the nuances of the existing code in their own project.  They did a fairly massive job of cleaning up "dead code," renaming and reorganizing functions and data structures to stuff they considered more intuitive, and regrouping functions into new files and directories that
they considered more natural.  Then they carefully re-ran our full system test suite against their refactored code to make sure it gave the same results as the old stuff, and then checked their code in to our tree so that they could track my future changes as they continued with their  project.

I think I was working on some relatively isolated project at the time, so I didn't have to interact with most of their changes immediately.  There was a release shortly after they merged in their changes, things duly passed QA, and the code got shipped to our customers.

Late one afternoon, a top-priority bug report comes in and lands on my desk.  We have a major customer dead in the water, unable to make any further progress on their project until we give them a fix or workaround, and it's been tracked to that module I'm responsible for.  Under our support contract, we were supposed to give them a response in 24 hours.

I make a debug build of the latest release, and go to fire it up on the customer example in the debugger so I can see what's going on.  I go looking for the main function involved in what is likely to be the customer's problem so I can put a breakpoint in it.  It's not there!  The other team renamed it something else.  I look for a certain global data structure that might give me a clue about what's going on.  It's not there!  They've renamed it and changed it around.  Suspecting some sort of data structure corruption (e.g., various flags not getting cleared properly after various steps), I go to call our all-purpose data-integrity checking function that will check and report all kinds of possible problems in the module's data structures.  It's not there!  The other team removed it as part of their "dead code removal" (this function was sufficiently expensive to run that we only ever called it directly from the debugger, not the customer flow).  I can't just go back and insert an old version, because of all the changes to function names and data structure details.  The offsite team has all gone home for the day, and I think their tech lead was on vacation to boot.  Meanwhile, the clock was running...

I somehow managed to muddle through and resolve the issue.  But I was groping blindly in the dark without most of my usual tools to resolve such problems.  I'm sure in the abstract, the names the other team chose for things may have made more sense, at least to them.  But they weren't the names and structures I was familiar with, and a critical customer support crisis isn't the best time to try and become familiar with a different view of the code. 

So even if a proposed refactoring makes sense to you, you should be concerned with how others who have to interact with the code will feel about your changes.  If it's a small change, and they can recognize what you did from the surrounding familiar context, that's fine.  Otherwise, you may have a latent problem bringing others up to speed on your changes.
DaveW Send private email
Friday, September 09, 2005
Thanks for the story Dave. We could all learn from this.

Thinking about how this could have been avoided, would it have been possible for the other team to work more closely with you? Maybe pair-programming or code reviews would have helped.
Chris Steinbach Send private email
Saturday, September 10, 2005
If it's "programmed by coincidence" then you can't prove that it "ain't broken"--so, by all means begin refactoring.
Peter Ritchie Send private email
Saturday, September 10, 2005
Chris - the two biggest obstacles to working more closely with the other team were the distance and my other priorities.  It's a lot harder to pair with someone who's working several hundred miles away on a different time schedule than someone in the next office.  And my management wanted me spending most of my time on my own project, rather than babysitting or monitoring one of the better-run teams in the company who were working independently.

I don't think anyone anticipated the increased difficulty in debugging after they checked in their changes.  I think the experience is partly a testimony to the increased communications difficulty in working with remote teams, and partly a reminder that what you see as improvements to the code may be viewed differently by someone who isn't part of the original team decision.
DaveW Send private email
Monday, September 12, 2005
Distributed development together with shared code ownership is not easy. For this to work, there probably would have to be a formal review procedure in place whereby the other team works on a different branch and you integrate their changes only after looking over them. Frankly I imagine this would cost too much.

A better idea, if you have any political clout, is to stop any decision to do distributed development before it gets going.
Chris Steinbach Send private email
Tuesday, September 13, 2005

This topic is archived. No further replies will be accepted.

Other recent topics Other recent topics
Powered by FogBugz