I can't see any harm in deleting the code if it's necessary - provided you use source control. The other answers to this question describe the ethics of deleting someone else's code quite well.
–
AnonymousAug 6 '12 at 15:51

8

I don't think this is specifically mentioned in any of the [excellent] answers: even if existing code deserves a rewrite, if it works now, it is very likely a waste of time [money] to rewrite. I have learned that in many scenarios quick 'n dirty is the way to go. You're paid to write code, so you should be as efficient as possible. You don't need a well-designed framework for something that will only be run a few times. You don't want to take a week to do something that could have been done in a day using poor design practices. Many times managers prefer this approach.
–
tazAug 6 '12 at 19:51

Needs more context. Why are you changing the code, what is the scope of the change - hours, days, weeks, months of work. Who is paying for the change and is it really needed. Who approves the need for the change. What is you process and how did it fail so badly that you got into this mess to start with.
–
mattnzAug 7 '12 at 5:08

12 Answers
12

I think good communication is always the best practice. Talk to the developer and see if there's a reason why it's coded the way it is. It may be that they have been meaning to get back and refactor it for ages, it may be that they did it that way for a very good reason, or it may be that you both can learn something from the conversation.

Going in and rewriting without prior communication is a recipe for ill will.

Yep, I've learned from experience that some developers can get extremely agitated if you even fix bugs in their code without asking. Of course, this was on a project with no issue tracking or task planning, and I'm amazed it ever even shipped.
–
fluffyAug 6 '12 at 16:04

+1 for "talk to the developer" - we have a bug (well, at least one, probably more) that the customer now expects... And is so far buried that I was the only one who vaguely knew why it was left alone.
–
IzkataAug 6 '12 at 19:04

3

I don't disagree with "good communication" in principle, but I think this answer is a little trite. Discounting all of the troublesome questions about what circumstances led up to this question being asked, it really shouldn't matter in the slightest whether a developer answers "yes" or "no" to the question of "should I rewrite your code?", because that's not necessarily going to be the answer that's best for either the team or the product. Sure, one should try to discover all one can about the original assumptions and decisions, but how do we know that the OP didn't already do that?
–
AaronaughtAug 6 '12 at 21:20

@Aaronaught that's true, but unless the developer is really disfunctional, it's more likely to be the best answer for the team than what someone less familiar with the code might think. And even if it's not, there's no harm in finding out what the story is.
–
JohnLAug 7 '12 at 11:32

2

@Aaronaught - The question of whether to ask the original developer first implies that the OP has not yet talked to the original developer and thus has not tried to discover all he can. I hope the answer is the opposite of trite--it's fundamental.
–
Matthew FlynnAug 7 '12 at 18:03

Generally, if you all agree that you are all responsible for the code, then it's ok to rewrite any code if it's an improvement. Discuss it with the other developer first as a courtesy. There may be valid reasons it's written in a certain way. On a personal level many developers get emotionally invested in what they produce and, as others have said, you want no ill will.

Specifically it depends somewhat on team dynamics. A senior developer about to rewrite a junior developer's code should probably perform a code review (my site) with them before simply rewriting it. That way the junior developer can learn from the situation.

This is the only answer so far that I can agree with. If something needs to be rewritten, I rewrite it. I don't even look to see who the original author was; why should I? Assuming there are tests (there are tests, right?), and assuming its purpose is reasonably clear or explained by comments (if not, then it definitely needs to go), then I'll know pretty quickly if I've broken anything. Of course I'm talking about rewriting a few classes, not an entire framework, but if the rewrite is that large then it's more of a scheduling/project planning issue than a shared-ownership issue.
–
AaronaughtAug 6 '12 at 20:25

If you're reworking a few lines to be more efficient, just do it. Maybe ask if there's some danger of you erasing subtle behavior. Report the change during scrum or via email once it's done, unless it's really trivial (fixing a typo).

If you're reworking a decently sized class, you should probably ask to make sure you understand the design/behavior. Let people know ahead of time so that anyone that might be working in the same area can be aware and coordinate with you.

If you're reworking a larger system, you should definitely work with your team to make them aware of the changes and plan the design.

As Matthew Flynn said, it is necessary to talk to the developer first. The initial developer might tell important things about the functionality and explain why this or that solution was given.

But before refactoring or re-writing the code, either make a backup or branch containing that code. If the old code works fine, there will remain a chance that it might be recovered.

If you have a source-control system (which I assume you do have), then, if possible, refactor the whole code and only afterwards, when you're sure it works fine, check that in.

Do not delete the old code, at least before you're sure the new code works fine. But leaving the old code there forever isn't a good thing either. I suggest you delete the code some time later, like in a month after it passed the testing.

"I suggest you delete the code some time later" - where should it stay in the meantime? In a commented block? That's what source control is for, to keep a backup copy of all changed/deleted code for easy retrieval, while keeping the current source code clean.
–
dj18Aug 6 '12 at 19:24

@dj18, I usually keep the code commented for a while to make it more evident, and so that everyone who deals with the code can see that it was modified at once. Besides, its easier to look up in this way when the code is just edited
–
superMAug 6 '12 at 20:00

1

That's another function of a proper version control system: you can view when changes were checked in, compare versions to see exactly what changed from one checkin to the next, and associate comments with checkins to explain why a modification was made. If it is so important for others to know right away that a piece of code was changed, perhaps send out an e-mail notification instead of waiting for the other developers to chance upon your comments.
–
dj18Aug 6 '12 at 20:12

2

The knowledge that this answer claims should be obtained from the original developer should be in the code, or at least in the accompanying comments or documentation. If this is important information then it should not be siloed in somebody's brain, and development policies shouldn't encourage this. Also, why not delete the old code? You can get it back if you really need to; that's what revision control is for.
–
AaronaughtAug 6 '12 at 20:28

1

@Aaronaught: I think what superM meant to say was of the "here be dragons" kind, or "lessons learned". While pitfalls should be fairly obvious to a seasoned programmer, subtle choices are less obvious and might not be well documented. (I agree that those are bad signs that need to be addressed.)
–
rwongAug 7 '12 at 6:58

If your original writers are there, communicate with them. Not JUST for ettiquette, but for additional information, in case there are cases or circumstances that you aren't aware of. Sometimes, they'll tell you something valuable.

We have abysmal code control. I currently have a ton of stuff that needs review and maintenance, and don't even have anyone to do code review with. The previous writers (aside from moi) did not bother to comment any code. And, they're gone from the company. There's no one to ask about the code.

When I re-write, I comment out, with comments that I commented it out, as well as date and name/initials, and why it was being commented out. I comment new code, with name or initials, date, reason is was added, etc.

Additional comments are made at the package head (er, usually), indicating what functions/procs/SQL/etc. were changed, with date. It makes it easy to look up sections that have been changed, and those sections have full documentation.

Comments are cheap. Dates will be an indicator of what changed, and after x-number of (days/revisions/new hires/retires) then the code can be cleaned of old comments and the remaining is the new baseline.

It doesn't really matter if the original developer is on the team or not, or even if you are the original developer. Complete rewriting of any shared code should be run by your team, for the following reasons:

It takes a significant amount of time. If someone else is working on a related change, you don't want to waste their time.

Other people have a different perspective. Even if you've been programming 20 years, someone else might know things about interactions with code you rarely touch.

Other developers are the "customer" of the source code. Source code is written for other developers to read. They may have architecture requirements, input on style, or feature requests they have been wanting in that code, but haven't had time. You don't want to finish a refactor only to find out another developer needs it refactored in a different way.

The very premise of this question raises a number of concerns for me that I don't think any of the existing answers are adequately trying to address. Let me ask some follow-up questions here:

Are you absolutely, positively certain that you are talking about rewriting and not refactoring? By definition, changes to code style or structure that result in an improved (or simply different) implementation without changes to the external behaviour is a refactor, not a rewrite. Breaking a monolithic 500-line routine into a set of 50 subroutines may involve writing quite a bit of new code, but it is not a rewrite. The term rewrite implies throwing away an entire product or feature and starting over from scratch; it is not something to be taken lightly.

If the original code's behaviour is so wrong, or the implementation is so buggy as to require a full-fledged rewrite, why was it not caught by your team's process and addressed in its proper context (i.e. technically rather than socially)? Bad or questionable code ought to be exposed quickly on a healthy team via tests, code reviews, design reviews, etc. Do you have these?

Is the manager/tech lead aware of the problem, and if not, why not? No matter what kind of process you have, code quality is ordinarily the development manager's responsibility. He or she is the first person you should be asking - obviously not in the context of "so and so wrote crap code" but simply "I think I see a major problem here, can we talk about a rewrite?" If it doesn't warrant this type of discussion, then maybe it doesn't warrant a rewrite either?

If the size and scope of the offending code is large enough to warrant serious discussion of a rewrite, why is it exclusively owned by one developer? Most if not all of the times when I've seen code and thought "rewrite", it's been trampled on by a dozen different people and the badness is a direct result of the accumulation of style inconsistencies, mistaken or altered assumptions, and good old-fashioned hacks. Code grows thorns over time precisely because of its shared ownership.

My point here is, it's strange that one person would own such a large swath of code in an environment that supposedly practices collective ownership. Are other people afraid to touch this developer's code? Are they afraid to bring up the subject? Maybe there is in an underlying problem with your group dynamic, or with this developer specifically; if there is, don't ignore it. Or, alternatively, perhaps it is your first time working on this project, and if so, why are you thinking about a rewrite so early in the game? Something about the situation doesn't seem to add up for me.

The question states, in the first paragraph, any developer can change any line of code to add functionality, to refactor, fix bugs or improve designs. Will a rewrite not do these things? Or will it do something extra - and if so, what? Fundamentally, what are your reasons for wanting to do a rewrite, and how sure are you that they'll benefit the product or the team? You don't need to answer for me, but you'd better have some objective points to make if and when you choose to discuss it with the team.

I truly feel that this question entails a huge amount of context and should not be answered without a very clear understanding of that context. The "correct" answer completely depends on your team, your product, your organization, your personality, your process, the scope of the original code, the scope of the changes, and so on and so forth. This is, IMO, an issue that you absolutely need to take up with your team, not on an online Q&A site.

+1 The only correct answer in this discussion.
–
mattnzAug 7 '12 at 5:06

In my experience on XP teams with collective code ownership (and pair programming) is that major changes to the design or "rewrites" of portions of the system usually involve discussion with most of the team (everyone who cares). With critical core functionality, the existing design is the best the team could do at the time. If it can't easily be changed, it's helpful to get everyone's ideas on what they think it should do, and various ways that it could be improved. If you can't get consensus, try some "spikes" or experiments to gather information.
–
Jeff GriggJan 6 '13 at 3:14

From my observation the general practice is: Never delete the code. Just comment it out and keep it.

My practice is to comment it out while replacing it. (Mainly for reference.) Then delete it on the the final commit of the working change. (This requires version control and an understanding of how to revert changes.)

When I find old commented code, I try to delete it in a separate commit with appropriate comments. This makes it easier to revert or inspect the change should it be required.

For all changes you should be able to use the revision history to determine the developer(s) who created the code. (Annotated revision history helps here.) Contact them if possible to see why the code is as it is. On many projects, cleanup time just doesn't occur and things get left behind. Check the bug tracker to see if there is a record of the required cleanup. Sometime changes need to be cleaned up a release or two later.

If you are changing code, there should be a reason you are working with that code. Check with the original developer to find out why the code is like it is. If there is a legitimate reason not to fix it, add a comment explaining why it wasn't fixed, or why is shouldn't be changed. This will save the next developer some time.

Tags like FixMe, ToDo, Bug, and Hack can be used to indicate code which could be changed later. (It is acceptable to tag limitations as Bug and not fix them if they won't be triggered under the required conditions.) It might be bug if a home accounting program overflows at 20 million dollars, but I wouldn't waste much time fixing it.

Code review changes should be done by the original developer if it is appropriate to modify the code.

It probably depends on why the rewrite is necessary. If it's because there are problems with what's written, and there have always been problems with it, then talking about it is called for, if only to minimise how often code needs to be fixed in the future.

My suggestion in this case is to pair when fixing the code. This way, it provides a good example of the intermediate states, and (hopefully), some context on why the rewritten code is better. Also (if only as a personal exercise), try refactoring the code to be better without a total rewrite. It will be harder, but it's good for you.

On the other hand, there are some other times when a rewrite is called for:

The team has learned a lot since the code was written, and the dev(s) who wrote it would write it differently now, even without your input.

A new feature requirement changes the situation such that a targeted mini-rewrite is appropriate.

In these cases, I probably wouldn't bother with a conversation.

Thinking about it, it seems to me that the longer the code has been there, the less likely a conversation is necessary.

I think @aaronaught made some good points, which really leads to the answer I wanted to give, which is that it really depends on who's making the change (and why) and who wrote the code.

In my personal experience code is normally changed because either it doesn't work as intended, or you simply need to extend what it actually does.

In a Team dev environment, you shouldn't have to (and may not be able to) talk to the original coder, everything should be clear from the code.

That then leads to the question which consumes most of my time, which is what did the original programmer intend, and it is that question which most often leads to code being deleted, and is why we should comment everything, and where inexperienced junior programmers most often fall foul.

Any programmer that is changing someone else's code (refactoring) really should as a matter of courtesy and practice copy the same coding style of the code already in place, and first take steps to figure out how the original code worked, and what it was trying to, and actually going to, achieve. Often this in itself identifys bugs, but certainly it forces people to endure the pain that the next person will have looking at your code.

In my team anyone can delete, refactor or rewrite anything, and I view 'ownership' as a practice which breeds lazyness, as if one person is sure to be notified of any changes, why would they need to make the code readable.

So in short, no, you shouldn't have to ask the original author of the code, and if on looking at the code you do, then it is a sign that either his code isn't readable enough, or you need to improve your skills. However, I find it good form to leave the original code in place, commented out, until you're absolutely sure that in rewriting, you haven't accidentally removed required functionality. Nobody is perfect.