Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
3  
Understand your requirements and scope, basically. Stay within that scope. I feel for you; I've been in both places, and it's really nice when people trust you to do the right things, and not so nice to be micro-managed. It sounds as if your current workplace is not as forward-thinking as your previous one. –  Robert Harvey Sep 6 '11 at 23:13
3  
As a skill upgrade, try to banish "I'm sensitive like that" from your vocabulary. It's rarely relevant in a professional setting. You actually have some control over your responses to workplace events; few things require careful documentation of your emotional response. If possible, try to leave your personal emotional response out of technical questions like this. However important it is to you, we really can't comment on it, or include it in a useful way in our answer. When you delete it from this question, nothing changes. –  S.Lott Sep 6 '11 at 23:20
2  
@Karl - this does not sound like micro management at all, you now work within a team, and must do what works for everyone, not just yourself. –  mattnz Sep 7 '11 at 0:16
7  
I'm going to let you in on a secret: The code was written by him (or a colleague he thinks highly of), and he is emotionally attached to the code. Every reason he said is BS. A good reason would be, "This is business critical code. A bug would be devastating and we don't have the testing in place to protect us from a change as large as this. Lets do it step by step instead." He didn't say this, which means that he doesn't like the fact that you took his baby and changed it. You're going to have some long days ahead of you bud, 'cause this guy will drive you crazy. –  Stargazer712 Sep 7 '11 at 1:59
1  
merge conflicts in Bash script? priceless! your colleague doesn't look like a good coder / reviewer btw. He didn't even mention that change like that requires thorough testing did he? –  gnat Sep 7 '11 at 5:59

9 Answers 9

up vote 15 down vote accepted

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.

share|improve this answer
2  
Good answer - if there's a concern that your hefty and unannounced refactoring is going to cause merge issues, then you need to deal with the merge issues by merging all the other changes into your version. Then you can make arrangements to get it committed. –  Carson63000 Sep 7 '11 at 0:25
    
+1. I usually handle merge conflicts by pulling other people's changes to my branch before merging into master. –  Daenyth Sep 9 '11 at 18:12

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.

share|improve this answer
    
Commenters: comments are for getting clarification to a post, not for extended discussion. If you think this answer is not useful, consider down-voting it. If you have a better solution, leave your own answer. If you'd like to discuss the topic of this post with others, please use chat. –  user8 Sep 8 '11 at 3:24
1  
This was a great answer, and it has given me a lot to think about. I think a lot of the problem I'm having is that while my changes were good, my juniority at the company (bearing in mind I've only been there two weeks) makes others weary of my code, and putting too many changes in makes them uneasy. The fact is that while I do have experience, my career shift makes me a rookie still, and giving me free reign over a semi-major build script would be dangerous. –  Karl Nicoll Sep 8 '11 at 20:43
    
@Karl: All problems solved by the basic technique of having conversations with the people actually concerned. Not us. The people who are actual stake-holders in the code base. –  S.Lott Sep 8 '11 at 20:48
    
@Karl: "isn't that a little condescending" Isn't that a little defensive? I have zero idea what you've done or not done. I can only guess based on the tiniest scrap of evidence. If it bothers you, I apologize. I'm providing input. What more do you want? Please be specific on what more you want, so that I can provide additional advice on how I've dealt with this situation by having formal conversations called "code walkthroughs" and "sprints". –  S.Lott Sep 8 '11 at 21:10
    
@S.Lott - I get what you're talking about, and it's great advice! I just don't think I'm in a position to be doing code walkthroughs, since the code has been around for years, and (regardless of how I may or may not have improved it) many people know it inside out (the reviewer included), and I think that having the new guy come in and refactor it all perhaps ruffles feathers and makes busy people's lives busier. How do you know when you're going off reservation by improving code that worked, and where do you draw the line between "if it ain't broke don't fix it", and allowing the code to rot? –  Karl Nicoll Sep 8 '11 at 21:36

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.

share|improve this answer
    
This is a great answer, and I like the approach you gave. I think the problem I've had is that I didn't follow an approach like that, and perhaps went in too much like a cowboy without thinking about the effect I was having. I think the last paragraph is bang on the money as well. This was my first thought (that I'd made the code look too unfamiliar for a proper review to be done easily), and I suspect it was the right one as well. –  Karl Nicoll Sep 8 '11 at 20:50

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.

share|improve this answer

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.

share|improve this answer
    
I think you're right that while I had refactored a lot of code, I hadn't thought about the possible consequences of my actions, and while I know the code was OK (I didn't change any code other than to copy/paste it into functions, and out of the main), the guy reviewing the code probably saw the diff and didn't want to risk it, especially with other people occasionally committing code in the background. –  Karl Nicoll Sep 8 '11 at 20:47
    
Don't let that scare you, though, refactoring is often good. But some communication with reviewers/peers is recommended before that. –  Coder Sep 9 '11 at 10:32

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.
share|improve this answer

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?"

share|improve this answer
3  
Downvoted for perpetuating that "If it ain't broke" garbage that leads to systems that are total cesspits to maintain because everyone adheres to that and never bothers to fix anything. –  Wayne M Sep 7 '11 at 12:18
    
@Wayne: My experience is that perfection usually drives projects over budget and over time, and even cesspits make the shareholders wealthy and can be delivered on time. My other observation is that Software projects, far more than any other industry, are over time and over budget and fail to deliver what gets promised. –  mattnz Sep 7 '11 at 22:56
2  
"Tell me again, why does this need to be done today" - If you don't do it today it will never happen. If it never happens you get the big ball of mud. –  Raynos Sep 7 '11 at 23:37
    
Agree, but @Raynos is basically my reason for such vehement disagreement with you, mattnz. I've worked at far too many places where everything went to shit because management refused to let things be refactored or redone properly, citing "If it's not broke, why fix it?" when it was broke and nobody except myself could see that it was broken. It's akin to wrapping duct tape around a leaky pipe - it "fixes" it, and it stops leaking for a while, but you aren't fixing the real issue. –  Wayne M Sep 8 '11 at 2:25
1  
Agreeing with @Wayne M here. Do not allow code to rot. Maybe you don't go looking for code to fix, but when you are looking at code, and it needs to be fixed, don't ignore it. Leave it in a better state than you found it. Do you refactor all at once? Not necessarily. Just leave it a little cleaner. Boy scout rule. –  Anthony Pegram Sep 9 '11 at 15:11

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.

share|improve this answer
    
1) This was the first code I'd been given to edit. 2) I'd thoroughly read the coding standards documentation for the team, but it didn't mention anything about refactoring (and limits therein), I followed all conventions as far as I am aware. 3) The code review was what brought the problem to light, it wasn't finished code yet, it was a "half-time review" if you like, to see how I was getting on. Yeah I have done what they asked, I wasn't avoiding the work at all! My issue was that while I was doing it, I took minute here and there while I was in a particular function just to clean it up. –  Karl Nicoll Sep 7 '11 at 20:12

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!

share|improve this answer
1  
+1, with the caveat that it's up to the boss to define the scope and to clearly articulate it. "Update this script" is not exactly a hallmark of clarity. –  Robert Harvey Sep 6 '11 at 23:11
3  
@Robert Harvey - this question focused so much on telling his life story, I doubt the full-length of the boss's instructions were included. Not focusing on what the boss said and how he said it is the problem. –  JeffO Sep 6 '11 at 23:33
    
@Jeff O - I was simply trying to state my background, and how it is perhaps affecting my work now. I apologise if it turned out to be "my life story". If it was TL, you D have to R ;-) –  Karl Nicoll Sep 7 '11 at 20:31
    
Good answer, and I think that's probably where I strayed is that I didn't ask about what I could and couldn't do. I think I've just gotten so used to having freedom to do what I want in my previous job that asking wasn't the first thing that came to mind. I'm going to in the future though, for sure :) –  Karl Nicoll Sep 8 '11 at 20:45

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.