When the code is horrible, for example no indentation at all (or only on two levels :/), one-lettered variable names etc., then I make my code look ok, and refactor the rest according to my time constraints.

Of course that depends on whether I'll end up maintaining the code - the more I have to deal with it, the more it will converge to my notion of programming style.

If I am only fixing a bug in the code and there is already a Responsible Party (the Original Author, a System Owner, etc), I try to match the coding style, then send them the patch.

If I have inherited this Hair-Ball, then I have to decide whether the code has a life span longer than the time I am going to be the RP. If I am only the place-holder while we 'end-of-life this Monster as fast as we can', I am comfortable letting sleeping dragons lie. If the Code will out last my curatorship, though, then I feel an obligation to spend time cleaning up the more egregious problems.

I tend to the Verbose style, anyway, so that doesn't bother me as much as 'terse, economical, and uncommented' code. I have always tried to code so that someone on an 0300 Page-from-Operations doesn't have to think to much. The Poor Old Sod might be me, and I know how fast I think at that time of night on no coffee.

So, the first thing I do is wade into the morass and document. My documentation comes in two parts: POD (or equal) and tests. I write up my understanding of the way the code works in the form of tests for the (all too often non-existent) test harness. Once I know that 'what I know' is really true, I write up a POD stanza to go in the code. After a bit, when I begin feel that I really understand that is going on, I start refactoring; Serious Refactoring. I try to spend an hour a day on my Pet; getting the Hair-Ball in shape isn't my primary job here, but I use the refactoring project as a relaxation at the end of the day.

It gives me a certain pleasure to (over the course of six weeks) turn a 10,000 line monolithic block-o-code into a cleaner more maintainable structure. (Only 4,000 lines (including inline POD) consisting of a Base Module to hold the singleton variables, twenty-five Inheritors (one for each of the 'basic' functions), and four Helper/Common Utility Modules. Oh, and 3,000+ tests in the brand spanking new test harness.) Partly, it's because I don't like to have my name associated with slock, and partly it the innate desire to tinker. (And mostly it's the old Boy Scout ethic -- leave the camping site cleaner than when you found it. But...)

I have performed this process maybe a dozen times in the past five years, and I just inherited another opportunity last month. I am going to have loads of fun in 2008.

• We are using a version control system,
• The change is small and contained, and
• We have a decent test suite.

For example, one change I am forever making is to get rid of escaped quotes within a quoted string. I consider this hard to understand:

print "I said \"Hi\".  You said \"Bye\".";
[download]

But, this is much easier to understand.

print qq{I said "Hi".  You said "Bye".};
[download]

If you're lucky enough to be working on part of a larger project or organization with defined coding standards, stick to those even if they disagree with your personal taste. People tend to get annoyed when they see you've checked in a two-line substantive change and 50 lines of renaming variables, moving braces, etc.

As an example, many people on my team never learned the lesson about bind variables in DB calls. So if I'm working with code that has this problem, I'll fix the statements involved in the code I'm working on but I'll leave the other ones in the module alone, unless I spot an actual bug in them.

Refactoring, especially within a module, is usually a good idea if it helps you introduce the change you need to make more cleanly. If I find that there's existing code that has two largely identical blocks and my change would require editing both of them, I'll try to combine them first so my new change becomes smaller. But I don't generally refactor code unrelated to the change I'm making even when I'm certain I'll do it better, just because it's not relevant at that moment.

If the change you need to make is more fundamental, or if the existing code is so horribly fragile that it can't accept any further changes, then large-scale rework (if not rewriting) may be called for. But be careful with this. When you see what looks like incomprehensible weirdness in the existing code, remember that it may reflect incompetence or laziness on your predecessor's part, but it may also have been a necessary workaround for some unpleasant real-world scenario.

One day I was in a piece of code, and I thought, "this is all scrunched together; a little space would make this a lot nicer." And I set about adding blank lines where I thought they improved readability. Later, I was looking at the revision history for the file, and I saw that I had basically undone the changes of the last programmer. That programmer must have thought, "this is all spaced out; I could see more of it if it were condensed..."

Generally, I try to make the least change I can. Sometimes I find something that I think is so horrible that I do change it. In those cases, I'm thinking that the style is bad enough that it's no longer a matter of taste. Even then, I prefer to keep those changes separate from actual functional changes. I want future generations to be able to sort out the important stuff from me imposing my horrifying style on innocent code.

I've inherited code written by someone who wrote verbosely. I keep finding myself saying "I could make this so much smaller, so much more efficient".

I don't find that writing easy-to-read/-maintain code makes inefficient code. This reminds me of a conversation from a few years ago at PerlMonks where I kept trying to figure out why someone insisted on making code smaller. It turned out that they assumed that smaller code was more efficient. That isn't the case.

So I object, in general, to "more efficient" and especially to "much more efficient". Other than for quick hacks, I write efficient code that is "verbose".

Now, if somebody is writing for( $i= 0; $i < @list; $i++ ) { ... $list[$i] ... }, then I'd likely refactor to for my $item ( @list ) [or, if $i is actually used besides for $list[$i], then for my $i ( 0..$#list )] when the time comes. But the main reason for that is neither compactness of code nor efficiency. Looping over lists is less bug-prone than other types of looping.

Note that I'm not claiming that your inheritted code has been written efficiently. It is just that the phrasing made me suspect there might be a "verbose code is slow code" bias.

One of the best routes when refactoring code, especially if you didn't originally write it, is to first flesh out your unit test suite so you are more likely to notice if your "improvements" change the behavior of the code. Writing and running more unit tests also helps you understand the code more fully and may even find some long-standing bugs.

tye,
This reminds me of a conversation from a few years ago at PerlMonks where I kept trying to figure out why someone insisted on making code smaller. It turned out that they assumed that smaller code was more efficient. That isn't the case.

You mentioned that to me a little over 5 years ago when I had about 5 months of perl under my belt. In retrospect, I think my problem was two fold. The first issue I had was that I didn't take the time to understand what your code was doing so when it wasn't fast enough, I didn't understand how to adapt it. The second part is that I did know that fewer ops usually means less work which usually means faster run time. I was a bit naive about achieving the fewer ops.

I try not to make that mistake anymore.

Cheers - L~R

I should have been a little more explicit about what I meant by "verbose", but I was too interested in what everyone else had to say.

Some of the code I was assigned was written by someone who didn't understand how to restrict scope in Perl, or specify global information. As a result, every subroutine had 15 parameters. To say this made understanding his code difficult is an understatement.

Another former collegue never used subroutines. *sigh*

I appreciate your comments on "verbose code is not slow code" and will try to keep them in mind. It is a fallacy I have fallen into.

If you have inherited a code base, you will gradually get to know it. After a while you know the code well enough to form a vision of your own on how things should be, and molding the software to that vision can actually be rewarding since you get to see steady progress as you fix things.

The book Perl Medic is an excellent read.

/J

In the $work context, we have a standard .perltidyrc file that each programmer uses. Each person is free to program in whatever way they wish, but before checking in they should run the project through Perl::Tidy.

Unfortunately, on our current project, 90% of the "blame" in svn annotations for who wrote code is placed on the person who imposed Perl::Tidy on the (100-200k SLOC) existing codebase.

Also, I don't trust Perl::Tidy to not break code.

You of course sent examples to the author of Perl::Tidy? The same author who specifically says that it doesn't work for everything? Also the same author who asks for help in running down the problems found in the field-- Or is enough for you to say that you just 'don't trust Perl::Tidy to not break code'... Of course it might be the case that code that breaks Perl::Tidy is complex to the point that it needs to refactored anyway---hmmm, useful even when it doesn't work; hard to beat that!

--hsm

"Never try to teach a pig to sing...it wastes your time and it annoys the pig."

As most posters here agree, if $work imposes standard style and coding practice, follow that! If I find colleage-code that doesn't follow these rules, I fix it and remind that person about the style we agreed and why. <rant>It cannot be denied that some people care less about small things like indentation and whitespace use than others</rant>.

If I get a project as a new and only owner, the very first thing I do is rewrite every single line to adhere to my style. Not only will the new code be very consistent and beautifull layed out in my perception, but it has proven a great way to understand what the original author had meant with the code in the first place. By reformatting, renaming, and reordering, you get a very good grip on the complete project and you can add the comments of your findings while you are at it, giving the next maintainer (in case you die) an even better base to start from.

Once style issues are `fixed', and of course all tests still pass, the first thing to do then is to put the project in git (fill in your favourite VCS) and find open/outstanding bugs. Now you understand the code, and have no excuse about not able to read it, fixing those should be easy^Wless difficult. Add tests for all the things that were unclear on this journey through code and for all the shiny new features you just added and release it.

Bravo. I concur with every word.

The only addition I would make is that if I have to fix or modify a piece of shared ownership/corporate code that is in a style radically different from my own preferred, then I will often go through the same "re-write and rename to my style" process in order to get to know that piece of code--on a private copy.

Only once I am convinced that I understand what the code really does and it still passes any existing tests, will I attempt to fix or modify it. And once I have made and verified the changes, I'll go back to the original and attempt to re-make them, in the original style, minimizing the changed lines commensurate with doing the job right before check in.

80% (figure varies) of the life cost of a piece of code may be in the maintenance phase, but I've seen studies that put up to 50% of that 80% as backing out incorrectly fixed bugs; fixing introducd bugs; or the backing out of features added that broke existing code.

Hit & run maintenance costs. And the two primary causes are: 1) 'simple' changes made to 'simple' code that turns out not to be; 2) changes made on the basis of out-of-date comments. I suspect, but have never seen proof, that a third cause is attempting to minimise changes for the sake of VCS change history stats.

---

Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.

"Science is about questioning the status quo. Questioning authority".

In the absence of evidence, opinion is indistinguishable from prejudice.

"Too many [] have been sedated by an oppressive environment of political correctness and risk aversion."

If your task is to enhance-or-debug, then by all means limit yourself to that. But obviously during the process of doing that you must also make “triage decisions.” That is, “is this corpse worth saving?”

Many things that stink are not dead, however. (In fact, quite a few things that I have written over the years acquired quite an ex post facto “smell” even to me.) You don't need to rewrite all that. Doing so won't necessarily make it any better at all.

All of us have worked in, or with, shops that had “famously bad code.” It was this-or-that module which was so constantly broken that people became adept at – even proud of – nursing it along. And you just gotta make that “dead-horse decision,” pull the trigger next to Old Dobbin's head and replace the damm thing with what it should have been all along. Sometimes you determine that this is simply what you have to do, but it should not be something that you simply do as a matter of course. The code may stink, but it's bought-and-paid-for.

When you need to enhance or debug code written by someone else, do you restrict yourself to the minimal change? Or do you find yourself cleaning up code, making it look more the way you program?

That depends. Do you want to get in and get out, or are you going to turn this into a career?

I've inherited code written by someone who wrote verbosely. I keep finding myself saying "I could make this so much smaller, so much more efficient".

Sure. And are there exhaustive tests that you can run to verify that your "cosmetic" changes don't make any of the tests fail? And what's the risk?

Mind you, verbose programming can be very useful, especially in an environment where you don't know who may end up supporting your software. Dense programming could cause more problems, and would require extensive retesting.

I have a confession to make. My code is usually pretty terse, but I suffer from over-commenting. So I'll write something brief, then spend the next five lines explaining what I just did (or what I'm about to do, since I put comments ahead of the code).

I'm just terrified of coming back to my own code in six months to a year's time and being unable to grok what it's doing.

Regardless of that, I'm curious: Do you give in to the temptation to make someone elses code look more like yours, or do you successfully resist it?

Nope -- I get in, make the smallest possible change, look over it *very* carefully, do as much testing as possible, and get out. I don't even muck with the indentation.

You've heard the phrase "The road to hell is paved with good intentions"? Many of those good intentions are a variation on "But I only made a small change to the code! It shouldn't have changed anything!"

Great question.

I've inherited code written by someone who wrote verbosely. I keep finding myself saying "I could make this so much smaller, so much more efficient".

Definitely true. “Smallness” and/or “verbosity” make not-the-slightest bit of difference to the Perl compiler, which is going to translate the whole thing into a p-code tree anyhow, and “efficiency” is pretty much subject to exactly the same argument-of-irrelevance. (At a billion ops-per-second, no one can hear you scream.)

A good rule of thumb that I have adopted over the years, borne from the reality that for me it was true, is ... “would you spend your money to change this? If so, how will you convince your ever-skeptical board of directors (good folks... great at asking the hard questions) that you are actually right?”

If the answer is “no, there really isn't a short-term Return On Investment (ROI) in this,” ... don't do it! Spend the money on a really good beer, instead.

You are quite correct, as I recently realized. Please see Re^2: Maintenance vs. Programming style (efficiency).