First off, test thoroughly. You won't get a lot of mileage out of finding and fixing a bug your seniors don't think will ever fire, but if you introduce a noisy one you're dead in the water. A test which shows the code violates spec or can realistically fail should lead to the next point.

If you can identify who wrote something you want to alter, take it to them privately and ask some questions leading up to a discussion of alternatives, and finally to a "How about we . . . ?" question. Extra points for leading the guy to propose the solution. Now he's on your side. Remember that the guy has experience; you won't fool him if you try to lead him around too much. You'll annoy him if you won't get to the point in a straightforward way.

Style isn't a good enough reason. The rationale behind the style should be the focus, whether it's prophylactic, like strict and warnings, or for readability and maintainability.

Most coders will respond well to a collegial approach, but will bite back at a cocky, breezy, or confrontational junior.

Added: Use a version control system!! You then get to keep breakage brief and private, and you can back out of mistakes accurately.

This sounds a little backwards to be as a good way to ease yourself into the team. I'd have been inclined to say someting about how many times strictures had saved me untold time and embarasement in the past and would it be ok to add them through out the code base - "I'll fix any problems that show up as I add them".

Same sort of thing with the poorly structured code - "I don't quite see what's going on here" (to the original author), then: "Oh, now I see, but would it be better like this? Is it ok if I make similar changes elsewhere to improve maintainability of the code?".

I'd watch out making cosmetic changes that only affect layout unless the original layout was inconsistent. There's nothing that stirs up wrath more than having someone come and "fix" your carefully laid out code! One thing that may help is running the code through a pretty printer - just to make the layout consistent of course. :)

Or someone (who thinks they understand what the code is doing) makes an 'optimization' that changes an edge case.

Similar to GrandFather's suggestion, I personally try to start with commenting the code -- breaking it apart, trying to figure out what it's doing. Yes, there are some people who get touchy about comments, but you can add the notes in there / write the documentation, and then ask the original author(s) if you understood their code correctly -- it leaves you (and them) in a better position to maintain the code down the road.

Depending on what state the code's in, the personalities, and the skills (most of the folks I work with are IDL programmers who dabble in Perl, so don't think in terms of map or foreach) -- if the people are receptive, you might be able to give some helpful 'did you know you can ...'-type tips, and then show them where it can be applied to their code.

Oh -- and on a related note, the original poster might want to read A refactoring trap, which discusses some of the problems with 'fixing' code.

Tread lightly! The appearance of code is a "religeous war" issue to some people. Until the code becomes *yours* (i.e., you're the primary maintainer), change the code as little as possible. I'd definitely add "use strict" and "use warnings" statements to the top, and fix errors as they appear. But you should use the same style as you find in the (ugly...to you!) file.

(It never ceases to amaze me how much time can be pissed away by people reformatting each others code.)

Once the code becomes yours, however, then you can start on structural and stylistic improvements. (As your schedule allows....)

Also, there are politics involved: At one place, I was maintaining some code that I thought ugly. In a couple of places, it was particularly bad, so I was complaining about it to my boss. About a month later, I found that my boss was the one who wrote that code. It took me several months to fully extract my foot from my mouth...... 8^P

It's also possible that you may come to appreciate the style in use. (I've changed styles more times than I care to count. It's amazing what you can get used to quickly.)

Of course, the code *could* just be a hideous mess with no style. Even then, I'd restrict myself to bug fixes until I inherited it. And never badmouth code until you know who wrote it! ;^)

--roboticus

Thanks - This seems to be my best overall solution to the problem - I have already reverted back to the revision before I took over. I will be maintaining the code, but I feel it is in my best interest to, as you say, tread lightly. I have already stuck my foot in my mouth by making a general statement about the code - the guy who wrote it then went on to show me how his code was better than the example I was showing him. He did not write the particular program that I used as an example. I understand its a sticky situation, and I know how I feel when someone comes along and changes things that I have done. In time, I definatley will change the code to suit my style, as I am the maintainer - but some people have put in a lot of work to get it to the point its at, and I respect that...
Ted
--
"That which we persist in doing becomes easier, not that the task itself has become easier, but that our ability to perform it has improved."
  --Ralph Waldo Emerson

In a couple of places, it was particularly bad, so I was complaining about it to my boss. About a month later, I found that my boss was the one who wrote that code. It took me several months to fully extract my foot from my mouth...... 8^P

I worked at a place where a colleague (sitting next to the head of IT) said "What ******* **** wrote this ****?", only for said head of IT to say 'Me, I did'. Admittedly, the first thing the head of IT did to get code 'working' was to delete use strict, so it was a fairly valid point to raise.

I have yet to be employed by a business that doesn't dance on the edge; eolving their code base rather than planning and developing. I point out the different solutions available (and their false economies) - but who wants to plan and create something to be proud of when hacking it will do for now.

Personally I always point out any code that I know to be bad, or don't understand. But, this works for me, and should not necessarily be applied to all cases. Sometimes I do re-factor code by hand in order to learn what it is actually doing (which will be different from what others think it does, what the business wanted before and what they want now). I am not frightened of being wrong or highlighting my own lack of knowledge and I am happy to put my foot in my mouth. Either way someone learns something.

Although I agree to some extent that you should have consideration for other people's code, I wouldn't lose sight of the fact that it most likely is not their code - it belongs to the company. Your job is to amend, delete to create new lines of code that will profit the company you are working for. Keep a good perspective of what you are doing and who you are doing it for.

-=( Graq )=-

IMO if you are improving the robustness of scripts that have not been "developed" but rather "evolved" then theres a reasonable chance the authors will appreciate it, either becuase they never found the time or inclination to do the cleanup, or because they just arent that good at it. Ive known a number of people without strong development backgrounds who couldnt refactor to save their life, but were extremely appreciative when somebody helped them refactor their code.

On the other hand, sometimes people without strong backgrounds in development will not appreciate the changes. Most likely these people wont understand the criteria that you have used to refactor the scripts and won't see any benefit in the changes and may even see it as a personal slight. People like that will always be a pain to deal with so i wouldnt worry too much about them. Just be friendly and not cocky and it should work out ok.

If you are NOT primary maintainer of the code, then you touch as little as possible to do what needs done.

Either way, touching code that is currently working just to touch it is a damn good way to break something. Since you didn't write it and aren't aware of all the contexts involved, you will probably break something important and not be aware that it's broken. In other words, don't do that. It will hurt. A lot. Like no-salary hurt.

I bet that "this doesn't run under strict" won't be considered to be an error by anyone who doesn't use strict in the first place. If you have uncovered real errors somewhere (i.e. the code doesn't do what it is supposed to, or it is vulnerable to an attack, or will fail on bad data), then certainly point them out in a spirit of cooperation and teamwork.

If not, perhaps you would consider building some tests for the existing code instead? This will first make you better aware of what their code is suposed to do, rather than having you working backward and trying to figure out what existing code actually does. Once you understand the specifications, and have tests to exercise the code, then you can say with some authority whether there are true errors. If not, I'd say leave that code alone and move to the next piece (there is always more). It may well be that an ugly little piece of cruft is perfectly functional and even robust and need never be touched again.

As I said - the code isn't really full of errors, mostly style differences. I am now the maintainer of the code, and for purpose of maintaining it - Id prefer it to be readable - to me, I am more concerned about stepping on toes and how some people here might deal with that. I have great respect for the people who wrote the code.

Thanks for your suggestion of writing tests - my background is not in softare development, and its been suggested enough times to me by people who know and seems like a logicaland reasonable thing to do...

thanks


Ted
--
"That which we persist in doing becomes easier, not that the task itself has become easier, but that our ability to perform it has improved."
  --Ralph Waldo Emerson

You could describe rather accurately the work you've done as "getting to know the code base": you've read the code, in order to be familiar with it, and in the process you've refactored it a little.

Provided you are the one who will be maintaining it, this should not cause a problem with the original authors.

Of course adding tests to ensure that your refactoring did not break anything would be even better. Also using perl tidy (with the appropriate options) to reformat the code, and using Perl::Critic to justify your changes would remove some of the "I don't like the way you code" factor.

One way to make code refactoring more palatable to the original authors is to add a lot of useful comments as you 'get to know the code'. I have rarely found existing code to be adequately POD-ed and you could make substantial changes to code under the guise of 'just cleaning up a little while I comment'.

Another technique I have used is to apply an OO veneer over non-modular code, changing the things I don't like while talking about it as though it was 'just a minor refactor'.

Most developers accept that they could have written their former code better, but there is no need to rub their faces in it. A little politeness never hurt anyone, especially where there are so many different (and sometimes equally valid) ways to solve a problem.

Fundamentally, though, you are either responsible for the code or you aren't. If you are, then (in my view) you can do whatever you think best, and there is no need to 'present' your changes to the original authors (who probably don't care and are working on something else). Personally, I would just keep quiet about it, write good documentation that justifies any extensive changes, and treat your predecessors with deference and grace. Above all, don't change anything unless you fully understand and can test it, and don't be afraid to ask for help.

FYI - These links might also help if you are searching for tools:

DoxyFilt (Doxygen for Perl) offsite
Analyzing large Perl code base.
Becoming familiar with a too-big codebase?

HTH,

I'm working at a (non-perl) project where there are some parts I don't like. I wrote "bugfixes" at a few places. I've applied these only to my copy of the code, and sometimes it turns out that my fix doesn't really fix the problem or it causes a new one. For those changes that really fix something, I'll ask to commit them to the real code, but I've yet to decide on how exactly I'll do this. My fixes are typically not stylistical, I try to change only content.

Sometimes it is easier to push the bad aside and do the whole task from scratch. Then you have "new improved". The problem is that it must be "improved", being "new" doesn't make it better. Unfortunately many projects fall into the "it's new" catagory, when the old was very adequate for the task at hand. Think: an old (manual) typewriter with carbon paper at a remote location without power works quite well for a page or so a week.

Bring a copy of Damian Conway's Perl Best Practices into the office and let folks see you consult it. You can use it to stimulate conversations. Tell your co-workers that you, by and large, try to adhere to his ideas. Get Perl::Critic up and running on your editor - I use Vim and Vim's Perl IDE plugin. Using it is easy and fun. Perl::Critic will check whatever code you have in the current buffer against Conway's Best Practices and loads any violations into a "Quick Fix Window" which will take you one by one to each violation and tell you what is wrong and how to fix it. This makes it painless to 1. Learn the 256 best practices and 2. Quickly and efficiently bring a piece of code into pretty good form. It even suggests areas of code that need refactoring. This way, Damian is making the suggestions for refactoring - not you! I have found that turning this aspect of coding standardization over to Perl Critic frees me up for those things that only a person can do - such as designing the APIs and algorithms. I am quite happy for the freedom it gives me. Plus your coding conventions are designed by arguably to number two person in the Perl world right now! You can easily turn off any rules you don't like or silence it temporarily while you use symbolic references.

Bring a copy of Damian Conway's Perl Best Practices into the office and let folks see you consult it. You can use it to stimulate conversations. Tell your co-workers that you, by and large, try to adhere to his ideas. Get Perl::Critic up and running on your editor - I use Vim and Vim's Perl IDE plugin. Using it is easy and fun. Perl::Critic will check whatever code you have in the current buffer against Conway's Best Practices and loads any violations into a "Quick Fix Window" which will take you one by one to each violation and tell you what is wrong and how to fix it. This makes it painless to 1. Learn the 256 best practices and 2. Quickly and efficiently bring a piece of code into pretty good form. It even suggests areas of code that need refactoring. This way, Damian is making the suggestions for refactoring - not you! I have found that turning this aspect of coding standardization over to Perl Critic frees me up for those things that only a person can do - such as designing the APIs and algorithms. I am quite happy for the freedom it gives me. Plus your coding conventions are designed by arguably to number two person in the Perl world right now! You can easily turn off any rules you don't like or silence it temporarily while you use symbolic references.

I think you're in a difficult position. When you explain it here, you sound like you're just trying to get rid of some of the cruft; but that's almost certainly not how others (at your job) will view it. They're going to perceive you as saying, "Gee, I couldn't help noticing that all your code sucks eggs, and so I rewrote it all. Hope you don't mind." I know you're *not* saying that, but you can pretty well count on people feeling like that's what you're implying.

Ive pretty much taken over as the active maintainer, a lot of the code is being moved from AIX boxes to Linux - So I have a Little leeway there...


Ted
--
"Your emotions make you a monster"
  --The Dead Kennedys