As a software contractor – or as I say to my parents, consultant – I’ve discovered several natural rules over the years. You can learn them the hard way, or you can read my blog. This is The First, Top and Most Essential rule of software contractng.
Consultant’s Rule 1: If it’s not broken, don’t fix it
As a contractor, you’ll regularly arrive in a new company, with a whole new lot of code to mess around with. Monolithic. Archaic. Spaghetti. All terms that could apply to many mounds, sorry, heaps of code you’re likely to encounter.
The temptation is to fix it. Now. At once, and everywhere.
I see newbies do this all the time. I’ve been there myself. “Oh, these thirty lines of code are repeated here. And here. And…here…and here OMG who WROTE this?…and here…I know, I’ll pop them into a neat little function and call it from all over the place.”
Sounds like a good idea, right?
Sure. If you missed any little breaking differences across the twenty implementations, your mistake will be picked up by the automated unit tests.
Um, what automated unit tests?
Some companies do indeed maintain automated unit tests, and even run them occasionally, but are the tests documented? Do they cover the code you’re planning to change? All of it? Every possible instance?
TBH, in a contracting situation you probably don’t yet know how to run some of the classes you’re planning to change – no idea how to get to the place they’re called from at runtime – do you have time to research and test twenty different use cases? You only managed to get the code building yesterday. Oh and by the way, What are you actually SUPPOSED to be doing? Easing your way into the software by fixing a priority 3 bug…so why are you changing a file in a whole different module?
‘So. What?’ you ask. I’ve got to leave that stuff there, steaming gently, waiting for the next idiot to break it instead?’
No. Here are your options:
Point out the issue to your boss and ask for permission to change the twenty instances you’ve found, replacing them by a simple function call.
Good luck with that. They didn’t hire a contractor to refactor their code to perfectly match the structure recommended in universities. They know the software is kludgy, but it seems to work. They hired you to implement some new feature, or expedite a release that they consider business critical. There isn’t time to test your changes. Oh and by the way, your boss probably wrote some of the code you’re dissing right now.
Note, though, that this is the correct approach if you’ve identified an issue which is causing or is highly likely to cause a serious bug.
Don’t say anything to your boss, who looks rather busy. Just go ahead and do it, you’re a professional, you won’t make any mistakes.
Sometimes you’ll slip your changes through, but generally you’ll break a piece of functionality you didn’t know existed. Best case, you’ll delay a release. Worst case, you won’t delay the release and the first your boss will hear about your bug will be from floods of irate customers. Your name will be mud. How much are they paying you again?
Option 3 (for refactoring code that currently works, this is what I recommend)
Do things right for the particular bug or feature you’re working on. Create the function even though you’ll only call it once. Pay attention to function parameters, and to where you locate the function in the code, so it’ll be easy to reuse later.
Optionally, insert a todo comment in all the places you’d like to call your new function from, mentioning this useful function and where to find it.
Commit your fix / feature and tests.
Bide your time.
Later (maybe even six months later) you’ll find yourself working on the areas of code that would benefit from your super-cool function. Off you go. Delete all those useless redundant lines. Call your function instead. Your changes will (hopefully) be tested, because they’re in an area you are in fact supposed to be changing.
As you settle into a new job and become more familiar with the application you can relax a little. You’ll acquire a better grasp of implications and you’ll have some kind of automated test regime in place. (Won’t you?)
Eventually you should be in a position to make sweeping changes with confidence.
Conclusion: Refactoring is wide-ranging and incremental. You can repeat Option 3 above over and over again, until the code base is full of improvements and most of them are in use.
Guiding Principle: Never change anything without having a comprehensive test plan in place.