So I'm in a bit of a quandary

[vek]

Fellow monks I seek your wisdom.

I'm going to be adding some new functionality to an existing program. Unfortunately the code is ugly. No -w, no strict, global variables, highly inefficient and has red flags all over the place. The distinct lack of consistancy within the code is particulary worrying. Sometimes an array will be passed to a subroutine, sometimes a subroutine just goes to work on an array and you have no idea who/what/why/where the array was created. My first initial reaction was a rather loud "yuck" followed by a sigh. On impulse and from a future maintenance perspective I would like to re-write it .

There are a number of factors that a weighing heavily on my mind as I ponder the re-write:

1. The code is bad, but it's not broken. I keep hearing "if it ain't broke don't fix it" at the back of my mind. The program has been in production for a couple of years.
2. It's a large program and a re-write would take a decent amount of time.

And finally.

3. My boss was the original author. I'll obviously have to tread lightly so as to not offend.

Trying to find a nice way to explain the re-write is proving harder than I imagined. I'm starting to wonder whether I should just leave alone and try to add the new functionality without changing the old code too much - but doing that will drive me nuts.

If you've ever come across similar circumstances how have you handled it?

Update: I think big thankies are in order to all who offered their advice. Nice one.

Cheers.

-- vek --

[hacker]

3. My boss was the original author. I'll obviously have to tread lightly so as to not offend.

A few things come to mind:

1. Were you hired to be a perl programmer?
2. Is your skill level with perl expected to exceed that of your boss?
3. Does your boss still actively contribute/write/maintain any company perl code?
4. Is your boss going to be paying for your time to update/fix this code?

If your boss is still actively writing production perl code under these conditions, he puts the company at risk, depending on the nature of the code. If he does not write or maintain any company perl code, he could have written this at a time when -w -T and strict were not stressed as much as they are now.

Good programming practices are important, but spaghetti code is never excusable.

One approach you may want to try, which will allow you to do the rewrite, and at the same time not offend your boss, is to try to convert the code into a module/package-based piece of code, using proper OO concepts (this assumes of course, that the existing code is monolithic, and not modular). This will allow you to do the rewrite, and hide the inadequacies of your boss' code in more maintainable code.

Another approach is to go through it and "update" the code to work better with "current" perl and industry standards around usability and security. A rudimentary 'audit' of the code could be performed, to show where it would fail under certain conditions. Put it under load, fire different variables/inputs at it, etc. and see where it breaks down.

One thing I've always learned.. I improve my code and myself when people are critical of me. If everyone were to compliment my code, I'd never have the incentive to improve it. If everyone were to pick my code to death and find problems with it, I know exactly where I need to improve.

Perhaps the direct approach with your boss might also prove fruitful. He may respect your forwardness and experience, and allow you to fix it. Maybe you can teach him why those "missing" items are important to use, and how each piece of "broken" code could be improved to be faster/more-secure and so on.

[oakbox]

Another approach is to go through it and "update" the code to work better with "current" perl and industry standards around usability and security.

If your boss is still programming, even he probably looks at his 'old' code and cringes a little bit. I had a very similar experience recently. While the company couldn't afford a complete re-write of that piece of code (not mission critical) he DID see the value in going through and making it run under strict and -w.

If the code IS mission critical, selling a re-write of the code to OO shouldn't be a problem. If there is any chance that this code will be used going forward, or if it has functions that might be used by other callers, this would be a good time to make those functions cleanly available to other callers.

oakbox

[little]

I'm going to be adding some new functionality to an existing program.

So you just got a few important arguments arising out of that duty :-)
Ask yourself and (after getting a clear overview of those details in your head) ask your boss if the added functionality shall be:

• secure (both ways: against attackers, keep system integrity)
• error handling (fast debugging)
• reusable (fast further development, short development periods)
• easy to maintain (uptime)
• easy to expand to further functionality (secure investment)
• fast running at a nice (customizable) level (saving money in above places shouldn't cause a need for 'bigger' hardware)
and
• What could (and what should at least) be done to make the code to be added to 'act' in such way?
• Would you need to 'adjust' other code to speed up your developement or to achieve above named goals or can you assure above objectives in any other way, eg. is some functionality meanwhile provided by modules on CPAN and do they do a better job (more powerfull, more flexible, even if only more heavy stress tested - thats an argument for a module from CPAN :-) )?
• Could other code gain any of above named points, especially reusability, ease of maintenance and expansion if modified?
• etc.
At a company I worked once the developers group was cut down twice and of 35 people only 7 now actually do still work there now. But the first thing they did was a code review, and there where about 120 modules to be reviewed (yes, all inhouse products, they even wrote their own (btw. really cool) templating system). The measurement was about the same as above but also covered "coding style" to assure a monolith architecture and to reduce the amount of code in general in the way that further abstraction in the end did not redruce the number of modules, but reduced the amount of code dramatically.
The entire application was partially rewritten to be OO and to run under mod_perl. (35 web servers behind 7 load balancers) So they approved that good coding practice is the base for any kind of efficiency.
Msg me if you want proof.

Have a nice day
All decision is left to your taste

[dws]

I keep hearing "if it ain't broke don't fix it" at the back of my mind. ... My boss was the original author. I'll obviously have to tread lightly so as to not offend.

These are the two hurdles to work through first. I've seen people take over code that "worked good enough," and then get blamed for slacking or incompetence when it took them a long time to add new features. To avoid this happening to you, people need to know what the code looks like under the covers. Your boss being the author complicates things, but there may be a tactful way to approach the issue.

Were I in this position, I would first try to make things safe for my boss by leading off with something like "I can appreciate that you must have been working under non-ideal conditions when you wrote this, because I can see that you didn't have time to ...", and then see if you can't get him to agree that a bit of cleanup and refactoring is necessary before you try to add new functionality.

To give yourself confidence that you're not breaking things, you might want to develop some coarse-grained unit tests before you start.

[danger]

Ah, code maintenance diplomacy when the author is a superior. Tricky. Saying the code sucks and you want to essentially do a code base rewrite to implement a new feature obviously isn't likely to be a good strategy --- unless your boss is uncommonly ego-less and honest about his own code. How forthright you are in expressing your opinion of his code is something you'd best judge for yourself, but it isn't necessary to bash older code in order to argue for significant changes. What do you want? What does your boss want? How does what you want apply to what your boss wants?

On impulse and from a future maintenance perspective I would like to re-write it .

Impulse and the future aren't likely to cut it. The most important factor to focus on is the here and now --- the new feature. Arguments about future maintenance and growth may not seem immediate enough to justify large rewrites of working code at the present time. Even if *you* know that hacking the new feature onto the current system will take a significant portion of the time a major rewrite would take, it won't necessarily *sound* that way to your boss. Arguments about present speed or reliability will fail to be convincing if everything has been working fine for two years of production use.

Trying to find a nice way to explain the re-write is proving harder than I imagined.

You need to make the case that the heavy rewriting, refactoring, and modularization you are proposing will help you reliably implement the new feature in a timely manner. That's what your boss wants after all, right? Criticizing the lack of -w and strict, global variables, or poor style or structure, aren't likely to win your case. The thing works after all. One non-criticizing angle you can try is to argue that the present requirements exceed the original design and that a certain amount of refactoring is a necessary and good thing for the task at hand. Review the code base from this perspective and see what kind of case you can come up with. Speed, maintenance, and future growth arguments can be layered over this, but these may well be secondary issues in terms of what your boss wants now. If you really want to improve the code base you may have to settle for a more modest refactoring strategy rather than a complete re-write.

[CodeHound]

I have no experience in this matter so mine is an idea more than a suggestion. Hope it's appreciated and gives maybe reason to meditate or find a better solution.

Can you maybe develop what you need in a module, separated from the original code? It would give you the basis to "don't fix if it is not broken" and would allow you to do "occasional changes" to the original code, "to fit your necessities for the new functionalities".
I believe this could be some kind of workaround to facing your boss directly on how he coded and how the code makes your life difficult, but if you proceed wisely, you could just obtain what you expect, I suppose.
Hope it helps :)

update: extremely important
Backup. Backup everything before typing a single character.
Don't even think about modifying something you have not backed-up! If something goes wrong, be personally assured that a quick replace can bring everything in working order!

[FoxtrotUniform]

Backup. Backup everything before typing a single character.

You mean, "check the source into a revision-control system". Doesn't really matter which you use, as long as you can use it effectively.

Revision-control systems are so useful, I can't imagine developing without one.

--
The hell with paco, vote for Erudil!
:wq

[Phaysis]

I probably wrote that bad code. (laughs nervously)

During my first contract, as a "Junior Perl Developer", I was assigned to write a large monolith of code based on previous code snippets. This was a mission-critical program, and I had a short time to work on it. I mean, I made it ugly, complete with globals, inefficient passing of "private" variables (by value), the whole works. After the code went into production, I learned my folly: the server (a rather fast Sun box) was swamped, processes died and sat around in memory, the Oracle db was swamped with inefficient requests and half-open connections (a code snippet the bossman wrote apparently opened a new db connection with each query -- a fact that to this day gives me a satisfactory laugh. What's even more sad is that almost two years later, this bloated, naively-written chunk of code is still in production.)

By the end of things, I had discovered the value of -w, "use strict", and taint-checking, and was in the process of refactoring the code when the good boss, out of God(-complex)-like love, decided to cancel my contract and send me on my way back to my agent. Oy.

My suggestion is to rewrite your code using good OO concepts and resiliency checks, and test it on an office test server. Anything that's required try writing as a new module. If you have to make this a hobby project to do during your "down time" between other projects, then go ahead.

Bossman may just put a gold star on your report card.

-Shawn
(P^h) Phaysis
If idle hands are the tools of the devil, are idol tools the hands of god?

[John M. Dlugosz]

Years ago, I did an article for Ed Yourdon's "American Programmer" in the code-reuse issue. I always took the contrary point of view, so I did "the case for starting over". Talking to managers not engineers, I tried to describe the aging process of software and say what to look for that makes maintanence difficult and likely to introduce errors.

"If it ain't broke don't fix it" Well, why are you working on it at all? Adding new features, presumably. A messy program will be hard to extend and this may introduce problems. So if the code is OK (it works), it's still not extensible and doesn't take into account new requirements (or maintainability as a meta-requirement).

So explain that you'll change the overall architecture to accomidate new features, but you'll copy the individual functions (or the algorithm part, if you're adding arguments and making them members, but the keep it simple for the boss) over from the original work, and reviewing each as you go.

—John

[FoxtrotUniform]

I seem to be "Pedantic About Good Programming Practices Man" this morning, yet another super(?)hero that nobody really wants to see:

So explain that you'll change the overall architecture to accomidate new features, but you'll copy the individual functions (or the algorithm part, if you're adding arguments and making them members, but the keep it simple for the boss) over from the original work, and reviewing each as you go.

Don't forget the regression tests. There's little more embarassing than refactoring a program "to improve its robustness", then breaking the code in various subtle ways that slip into production. Properly written regression tests let you say to your manager: "So it works the way it did before, and also does foo, and I can add new features in half the time" with some actual data to back you up.

See also How You (Yes You!) Can Get Involved and The Joy of Test, among others.

--
The hell with paco, vote for Erudil!
:wq

[John M. Dlugosz]

I'm big on unit testing. The unit test becomes the regression test after a change is made. If someone reports a problem and it gets down to a small code sample, I add that to the unit test before fixing it.

Somehow I doubt that his existing code has unit test code with it. So just what is it supposed to do? Mudding the difference between what it's defined to do vs what it just happens to do is one of my pet issues, too.

[neilwatson]

Tell the truth.

If the code has problems don't be afraid to say so. If this company hired you to help them make money then they don't wan t you to be a "yes man". In the end more people will respect you for your honest opinion.

Neil Watson
watson-wilson.ca

[Aristotle]

Sure, but I think noone debated this anyway. How to tell the truth tactfully is a different matter.

Makeshifts last the longest.

[neilwatson]

Personally, this era of political correctness is starting to grate on me. A spade is a spade. Beating around the bush to spare someone's feelings (or avoid their wrath) is not productive.

I'm not saying that we should flay the person in a hail of ridicule. We have to be able to tell the truth and have people accept it in a dispassionate way. People need to learn to be more objective.

Sorry, I'll get of the soap box now...

Neil Watson
watson-wilson.ca

[bilfurd]

Been there. Bad memories.

Explain that the project will take longer than you thought because you have to trace what is happening to the data at each stage. As you figure it out, fix it and/or document it. You may be responsible for maintaining the code for a few years and might as well make your job easier from the start.

Of course, your boss knows you are only documenting and "applying a uniform format to the code" - not "fixing" what he wrote. "Optimizing" how your modifications interact with the original code comes later. (This will look a lot like a rewrite.)

I spent two frustrating years waiting to start a re-write that was always "two months off". During that time, the nasty code I inhereted just got bigger and more convoluted as we tried to work around the existing problems.

[toma]

I have run into this situation many times, although the code I have to upgrade is typically my five-year-old perl.

I have come up with a refactoring methodology that works for my own code, your milage may vary.

It's not such a big deal. Just add use strict; to the top and fix it until it passes
perl -c old_crufty.pl

Here's a cool trick where perl really shines at refactoring: as you add variable declarations, typically my $var;, try to put the my as deep as possible into the enclosing braces. This minimizes the scope of all the variables, and will help later.

The suggestions in this thread about unit test are excellent. Build a few tests and run them on the big mess of code that passes use strict;. Even a few simple tests are vastly superior to no tests.

If you aren't using revision control, start now. I only have to remember two RCS commands to make my life safe from a bad coding day.

Usually the crufty code is a long main that can be broken into a simple sequence of subroutines. Perform this breakup and pass the needed variables to the subroutines. This is the tricky part. If it's really okay code, just written in a linear style, there will be natural breaks and this won't be so hard. If the existing code is really a mess, this is where you give up and do a rewrite. Since you say the code works, there is hope that it is not so bad.

If you can figure out how to make one big pile of code into a collection of slightly smaller piles, you win big. I have a really ugly trick for making this work. I take the mass of globals that get passed about and put them into a hash or two or three, grouped by their functionality. Then I pass references to these hashes to the subroutines, where I use something like my $var= $ugly_hash->{var}, changing all the global variables into new local variables which are just copies. To pass the variables back from the subroutine, I just use my $ugly_hash->{var}= $var.

Once your code has survived the testing and been through this procedure, you have some code that is still quite crufty, but it has a few key advantages. It is still readable by your boss, who hasn't ever written OO perl, it is much easier to maintain. If you get the chance someday, it is easy to clean up, and you might even get to convert it to true OO perl.

I actually think this sort of work is fun. It does not have to be drudgery and it does not require an onerous amount of analysis. You should see much improvement with only forty hours of effort, or you should use a different approach such as a rewrite.

It should work perfectly the first time! - toma

[mattr]

My guess is that your boss doesn ont want you to rewrite the entire program. Lots of companies have these code heaps that are working and they have gotten to this point by Darwinian winnowing and tweaking over years.

If it is really important software with lots of bad things in there it sounds pretty likely that you will be unable to guarantee that a by-scratch rewrite will work the same. Or did you say there was a well-written technical specification document? :<

Depends on what your new feature is going to be, I'd think. If it is pretty modular and you can trace which global to update you might be able to get away with the pie and eat it too. If it is an all-embracing update and you have to manage the system in the future, a rewrite sounds more useful.. but remember, your boss didn't ask you to rewrite it, right? Why not discuss it with your boss and find out what the priorities are. If they ask for a hack then I guess you'll have to dive in and get dirty.

Though I've more often found people apologetic about code after the author has left the company ;) your boss probably will understand that refactoring the system around modular blackboxes with well-defined interfaces will reduce business risk and make it easier to add more features more quickly in the future.

If it is just garbage then you need to explain that you are having a lot of trouble understanding the code and could your boss sit down with you for some sessions to explain it. This might lead in to your enlightened suggestion "Why don't I just take the time to do a cleanup job on it so you can get back to work".

[John M. Dlugosz]

I just came across a sentiment that might be useful here. To meta-quote, Apocalypse 5 quotes, "As it says in Ecclesiastes, there's a time to build up, and a time to tear down.". You could look up the original passage and quote that.

[demerphq]

One thought that you may want to consider is that when working on legacy code often written by many people with different coding styles and conventions is that a quick run through perltidy will at least produce a consistent indentation and code formatting style. It will also catch and highlight a number of "dubious" practices that perl will not.

I use it routinely nowadays...

Yves / DeMerphq
---
Writing a good benchmark isnt as easy as it might look.

[hatter]

I've been in similar code-needs-rewriting situations. I've mostly gone for the step-by-step approach, rather than stop using oldcode one day, and start using newcode instead.
Remember that you can apply strict and other pragmas to a block, you don't have to make the whole thing compliant in one step.
Adding -w or use warnings will often turn up things that the code works despite, rather than because they are there, so be prepared to do some head-scrtching and work out the proper way that function should be working.
Testing - mentioned by several people already, but here just to re-iterate it, test as you go along so you know which change broke everything.
Use wrapper routines when a current subroutine works, but could be called better, so that parameters are localised, pre-processed or passed differently to the new one. This way, new code can use the good function, and old code can use the old one until it's converted. If the old sub is in a library, consider logging each time it's called, so you can figure out which scripts still need converting.

Hopefully after making these ad hoc fixes, one day you'll look at the code and realise that it's just a few hours work to make the last few changes, and in the meantime, the new code you write can take advantage of the new functions and pragma.

the hatter