How do you refrain from doing too much when maintaining code?

Question

At my previous job, I was in a relatively easy ASP.NET developer role that had me turning over a large number of small database systems for internal use within the company. I churned out the required code, and all the while I had almost complete creative control. After releasing a system, I'd even come back to it when I was in a low period of work and refactor the code and generally improve the system for the users and/or other developers.

I've now moved on, and tried to shift my career more towards what I am interested in. In doing so, I managed to find myself a great job (lots of learning, complex algorithms, etc).

The problem is, I seem to have picked up some bad habits in my former job that seem to have leaked into my new job. My first task at my new job is updating a Bash script that performs a build and deploy of some code, and basically the guy I was doing the work for essentially refused to review my code because the file diff was massive.

The problem was that while going through the code, I'd basically refactored half of the code into functions, standardized all the output, added a "debug mode" and basically given the script a hell of a spit-shine. But in doing this, I'd gone way off reservation and done too much. My colleague agreed that the code was good, but there was no way that I could commit the code without confusing other developers, and causing merge conflicts.

Now I'm worried that my freedom at my last job, coupled with my inexperience as a programmer has gotten me into the habit of changing code on autopilot.

Are there any sure fire techniques for keeping yourself on a tight leash when maintaining code? Any tips for judging when you've done too much refactoring? Where do you draw the line between "if it ain't broke don't fix it", and allowing the code to rot?

Thanks!

---

Some Clarifications

• I've been working on this particular script for three days, and while I have been doing so, I've had the code change every day when pulling from source control! It's not big changes by any means, but changes nonetheless that require merging.

• The combined length of the Bash scripts (if they were to be concatenated) is approximately 4000-5000 lines. The part I'm working on (one file) is 2000.

• You have to consider that I am definitely at the bottom of the food chain here. Doing "code walkthroughs", while I have done them in the past, is not an option here because I can't tell other developers how to do their job with only 2 weeks experience under my belt. I'm looking more to establish a rapport with my colleagues at the minute, not rub them the wrong way by criticising their code.

Answer 1

The problem is not that you did too much; you did it in parallel with other people that edited the same code.

Usually, when a massive refactoring is needed, it needs to be announced, so that people had minimum merge conflicts and had time to understand the new layout. In the meantime, you can do the refactoring on your private branch and then apply the (small) changes made by other developers from master or their branches to keep your refactored code current.

So, don't throw away your new shiny code. Make the smallest change it takes to current crufty code, let everyone merge and discuss when your big refactoring is safe to be applied.

Answer 2

The updated questions.

Are there any sure fire techniques for keeping yourself on a tight leash when maintaining code?

No.

Any tips for judging when you've done too much refactoring? Where do you draw the line between "if it ain't broke don't fix it", and allowing the code to rot?

None.

you can't just intuitively know when you are making too many changes, and that to know that, [you] need to ask.

Assessment.

My colleague agreed that the code was good, but there was no way that I could commit the code without confusing other developers, and causing merge conflicts.

Your colleague is crazy or jealous or both.

Actions.

"confusing other developers"? You need clarification on this. Who -- specifically -- are these mysterious other developer who will be confused? What will confuse them?

Have a meeting with the potentially confused. Call it a "code walkthrough" to explain the changes. End of confusion.

"causing merge conflicts"? You need clarification on this. Who -- specifically -- is also supposed to be maintaing this at the same time. Why? Why aren't the maintenance tasks organized better?

Have a meeting with all the other folks doing maintenance. Call it a "sprint kickoff" meeting to coordinate the changes. Then have a sprint until it's done. End of merge conflicts.

Disallowing a change for farcical reasons indicates serious issues that are not being addressed. Or non-issues that have been created from whole cloth.

Answer 3

Well, first of all, when asked to do something, ask what you are allowed to do and what you are not. I'm guessing that if you have asked your boss previously he would have said that you were not allowed to do that bunch of refactoring due to A, B, C reasons.

I'm also guessing that, as you described, your work environment does not support that kind of liberty, am I right? If so, you should, again, clarify it with your boss and teammates. "Ask" is the key word here.

Good luck!

Answer 4

Here would be my questions before passing too much judgment:

• How much code had you seen in your new job?
• Was there plenty of examples to show various conventions and standards that you ignored or didn't notice?
• Could you have gotten other co-workers to check your work and see how far you were going with this?

On the one hand, there is something to be said for cutting yourself some slack and adjusting to a new work environment. You're human and will make mistakes, that's OK. Your first few commits may be way off compared to other stuff but at least you did get something done, right? If you can take the attitude of doing better with each task and have some patience and persistence, my guess is that you'll be fine. If you lose either of the patience or persistence then you could be in trouble. If you lose the patience, then you may get very frustrated or depressed easily while if you lose the persistence, then you may get apathetic about your work which is a dangerous sign to my mind.

If you are working from an existing code base, it may be a good idea to look for conventions and standards, verify that these are in fact used and noticed and then adopt them in what you do. It may be easier to make the code homogeneous than you think but you have find and verify things initially as it may be just luck that someone put in a pattern and didn't recognize that it was a pattern.

Answer 5

Well, I'm probably going to get a few down-votes, but your colleague is not exactly wrong.

You refactored the hell out of a massive script, and if diff shows 30%+ code changes, you have to carefully review the whole damn thing to make sure you accidentally didn't copy-paste something like rm -rf /tmp/tempdataformyapp into rm -rf / tmp/tempdataformyapp

If the script is some backup script for some mundane task, that sees little to no modifications, and is internal by it's designation, you just made an unnecessary reviewing task for your colleague that might take considerable amount of time to finish.

In that case, I would make you write an unit-test for every damn function in that script, before I had to do a code review for completely unnecessary changes.

On the other hand, what you did, most likely, was good for maintenance. And if this is a core object that sees frequent modifications, and is used extensively, the changes were necessary. And huge bash script is a huge no-no in the first place, every bit of order in something like that helps.

TLDR; Maybe you're wrong, maybe you're not.

Answer 6

I think we all agree that improving code is a good thing. Breaking code into smaller, more maintainable pieces, adding comments and tests, and applying "best practices" all help to make a more solid product.

However, there's always a time and a place for such things, and it's usually not while working on some other task or all at once.

Instead, for future similar tasks, you might consider approaching the problem like so:

1. Understand the scope of the change you need to make.
2. If the change is overly complex due to duplicated or not-well-factored code, discuss with someone familiar with the code base. Explain the refactoring that you want to make and why it makes the code more maintainable.
3. Make the refactoring changes, without making your feature/bug changes. If necessary, do this in several small steps.
4. Now that the code is more maintainable, make the requested code change and fix the problem with a simple small change.

This way, you don't introduce unnecessary changes, you have the opportunity to learn about the reasoning behind choices in the system, and the changes you do make are easily consumable and verifyable.

I don't think it's entirely unreasonable for someone already familiar with a product to reject a changeset that changes so much code that it's impossible to get a good overview of what changed. Several small, focused changes are much easier to digest and understand than a single, monolithic change.

Answer 7

Bug tracking tools are your friend.

When refactoring large amounts of code, it is helpful to plan ahead. Break the refactoring work down into smaller work items. For each item, create an enhancement request in your bug tracking software. When there is a dependency upon other refactoring, note it in the report.

This allows for ...

1. the ideas to be captured and not lost.
2. these requests to be prioritized and factored into the schedule.
3. other people to read them over, and offer feedback.
4. a "paper trail" to track your contributions.

Answer 8

My answer to the original question is to think about the test impact of your changes.

When you look at code you would like to refactor, think through all the ways it is currently being used. Consider the cyclomatic complexity and then consider how well you will be testing each of those paths through the code.

If you are not overwhelmed by the magnitude of the testing involved with your change then you need to either:

• Think about it some more
• Make your changes and see how well you can test them

Remember that the larger and more complicated the code base is, the more likely you are to make changes that break something in ways that you did not expect.

Answer 9

My take is that the response you got is correct.
You are paid to do things that make the company you work for money. If the company does not make money, it will no survive. If you don't make the company money with what you do, you will not survive. If the next guy makes more money with less time than you, you will not survive.

If you want to refactor working code so it works the same but looks pretty, do it on your project, in your time. Your day job is not there for your own self gratification.

So how do you prevent yourself over engineering your solutions, ask some simple questions - "Would I be happy to pay this much for that output". "How does this affect the production life cycle", and "Tell me again, why does this need to be done today"

Also useful (This will be a red-rag-to-a-bull)

"If it ain't broke, don't fix it", or slightly less controversial "Who is paying to fix this?"