What Are the Unwritten Rules of Deleting Code? 384
Press2ToContinue writes "I came across this page that asks the question, 'what are the unwritten rules of deleting code?' It made me realize that I have seen no references to generally-accepted best-practice documents regarding code modification, deletion, or rewrites. I would imagine Slashdot's have come across them if they exist. The answers may be somewhat language-dependent, but what best practices do Slashdot's use when they modify production code?"
The more..... (Score:5, Insightful)
You may need to indicate that something is removed (Score:5, Insightful)
It's often good to just delete. Sometimes however I feel the need to put in a comment reminding the reader that the old functionality is gone. For example, it could say "no need to support device X here" or "input has already been validated". That's because it's not always evident to look in source code history to notice that there used to be something there.
Agile/LEAN applies here (Score:3, Insightful)
Unused code gets deleted no exceptions. If there's no client demonstrating its value, its value is zero. The reasons are obvious in systems that scale into the hundreds of thousands to millions of lines of code.The way I describe it to my teams is, Just-In-Time development rather than Just-In-Case. You can have a conceptual framework in place and fill it in on demand without committing yourself to an orthogonal matrix of features that 'might be useful'.
Re:Your VCS should manage this (Score:5, Insightful)
Revision control is just one aspect. If you aren't the only person working on the code, or if there are other external dependencies such as you publish an API that others depend upon, then some form of automated testing will help ensure you haven't broken anything by deleting that code. Heck even if you are the sole developer then automated tests are still an incredibly good thingl.
If you are deleting code within a larger project then try deprecating the code first. Flag it as deprecated in the documentation (you do have documentation don't you?) and put log messages in the code that warn whenever it is called. After a while you can be confident that it is unused (or only used in obscure rarely called functions) and you can delete with confidence.
Revision control is your safety net, your last line of defence if you erroneously delete code that is in fact needed. It also allows you to always refer back to that code. Good commit messages will also help.
Re:Your VCS should manage this (Score:4, Insightful)
Let me know how well that goes for you when your IDE tries to generate a call graph including third party applications that interface with yours via an HTTP API.
Re:The more..... (Score:5, Insightful)
I comment the code and leave it there for at least one minor version. If things are all right, I'll remove them. The code is in source control anyway.
Re:Version Control Is Your Friend (Score:4, Insightful)
Version control is not your friend when you use it like this. It becomes your worst fucking enemy.
Unless somehow your version control system somehow magically achieves the ability to imbue the knowledge of all the things that you tried and failed, and why they failed.
If you don't want the older, now obsolete and/or broken code to be seen.... get a proper IDE with code folding. Using the VCS for this is definitely using the wrong tool for the job.
If you try something obvious, and it doesn't work, odds are very good that the next bozo to come along is going to try exactly the same thing, and the odds are that they're not going to test it as well as you did.*
You need to comment the code out, and you need to add a short explanation of why it doesn't work. E.g. // this looks obvious, but because of foo baz and kwok should never be attempted. YourName Date
Then, when the idiot next in line tries to make exactly the change you said wouldn't work, (because you know that they will), and you are told to figure out why they broke the build, when you do figure out why it's broken, you can go back in the archive, pull out your previous attempt and explanation, and then clearly demonstrate that it isn't your fault.
Version control is an archiving and accountability tool. Not something to magically hide the code until you need it, because if it that's how you use it, you'll never know that the previous version was there. Thus it fails at the most important thing code must do - communicating clearly to the programmers. And communicating what doesn't work (and why) is well over half the battle, more like 80 or 90 percent. (Yet another example of the Pareto Principle in action)
TLDR: deleting old code to hide it epic fails the DRY principle
Re:The more..... (Score:5, Insightful)
Why not just delete it right away? Commenting out code in version control just creates more changes.
Re:Version Control Is Your Friend (Score:4, Insightful)
deleting old code to hide it epic fails the DRY principle
The purpose was not to hide code, but to delete code.
If code is no longer valid and maintained, it shouldn't still be available as if it were.
Re:The more..... (Score:5, Insightful)
"It seems that perfection is reached not when there is nothing left to add, but when there is nothing left to take away" --Antoine de Saint Exupéry
That is the only rule of deleting code; if you can take it away or do it more simply without causing Bad Things, do it.
Re:The more..... (Score:5, Insightful)
If everyone uses the system "comment out first, remove after it has shown to work", seeing commented out code means that the code is still being reviewed. That might be valuable information.
Mentioning the deleted code (Score:5, Insightful)
At a previous employer, there was a *written* rule for deleting significant blocks of code.
If a properly functioning block of code already in production was being deleted due to changes in business requirements, a comment should be inserted at or near the point of deletion, which mentions that some code was deleted and the original code can be found in version x.xx, and preferably a phrase saying what it did.
However, if the code was not yet released to the production system, or was being deleted because it's buggy, it was acceptable to simply delete it without leaving a comment (if somebody needed to research an old bug they could look in the bug tracker, which would show which version of the code last had the bug, so there was no need to mention the bug-deletion in the code).
Re:Version Control Is Your Friend (Score:3, Insightful)
Unless somehow your version control system somehow magically achieves the ability to imbue the knowledge of all the things that you tried and failed, and why they failed.
Thats why you
a) Comment well in your code explaining the algorithm being used, which gets preserved in VCS alongside the code
b) Correctly document adding/removing functionality in the commit log
If you don't want the older, now obsolete and/or broken code to be seen.... get a proper IDE with code folding. Using the VCS for this is definitely using the wrong tool for the job.
This is about code removal (eg when refactoring, tuning etc). Not hiding.
TLDR: deleting old code to hide it epic fails the DRY principle
Quite the opposite generally. Usually when I'm removing code it's when I'm refactoring to generalise code precisely so it can be reused in multiple places. Keeping the previous code in place commented out would mean I was then repeating myself.
When I have seen code where the developer's left in all the old code commented/disabled it simply makes the code look bloated and harder to read. Especially when the code blocks have changed so the code breaks the levels of {} indentation.
You need to comment the code out, and you need to add a short explanation of why it doesn't work. E.g. // this looks obvious, but because of foo baz and kwok should never be attempted. YourName Date
Rather than leaving in bloated code which in context will probably no longer make sense, it'd be better to have the correct functioning code only and in the comments where you explain what it does if there is an obvious but non-functional solution explain there what it is and why it's not done. If anyone really wanted to look at why then they can use a VCS tool to look at the first commit where that line appeared, and thus find the previous commit with the old code (or just use the date in your style suffix).
I'm guessing your performance and thus pay/bonus is determined by the number of lines of code you have in production, rather than code quality (far harder to assess)?
Re:How can ... (Score:5, Insightful)
Indeed, version control is the only real solution.
We use git at work, and some coworkers insist on "commenting out" code that's no longer needed. I insist that we should delete it. Should we ever need it again, we have version control; and with proper commit messages, old code is easier to find too.
Re:The more..... (Score:3, Insightful)
I always keep in mind that the code I'm working on will be assembled, and the assembly is smaller and tighter with assembly oriented coding. So I do comment out working code and replace it with an alternate if I think it could be more efficient.
Re:The more..... (Score:5, Insightful)
If this is your goal, then you should just practice code-review of every commit and that'll achieve the same goal.
Only if every single developer that might look at the code reviews every single line. That doesn't scale beyond a small team.
In general it's best to delete unwanted code, but in some cases (e.g. where the code being changed is related to working a bug in a third-party's software, so it may not be possible to tell what the "right" code is apart from by experimentation) it can be useful to leave the deleted code around for a while to notify anyone fixing bugs in that area - provided a suitable comment explains why the code has been commented out. Leaving the code surrounded by "/*...*/", but with no explanation, is not helpful...
Re:The more..... (Score:5, Insightful)
If you're going to do this, YOU HAD BETTER COMMENT ON WHY YOU COMMENTED IT OUT. Too often I come across a block of code that is just commented out. A quick glance means it had a purpose, but for what reason, I'm not sure because apparently it isn't needed anymore. So then I ask "why?" But there is no answer to this question. This is possibly the worst feeling of working with code--the idea that something was there, now is not, but they left it in for some reason and you don't know why.
Now you're left with a couple scenarios:
It is like walking into an offline factory line. Most bystanders will simply think "I had better not touch this or it could cause serious problems" so the only people in full control was the last shift of factory workers for that line. Meanwhile similar lines are still operating so you're not exactly sure why this particular production line was halted.
Meanwhile if the code was in version control, I get a much clearer picture from diff on what exactly changed. If you just comment out a giant block and leave it there, check it in, then diff shows me that 2 lines had a comment added (in the case of block comments) or all the lines are commented out (modified, for single line comments). Now I have to diff again and skip to the version before that to get the picture I want, but now I may see other changes that may not be relevant.
Either way, if you are commenting out code for the first two scenarios I listed, you need to think again about your version control system and manage it better. Experimental or untested stuff should be branched, not visible to other devs otherwise people make mistake it for production code. If I see commented out code blocks, I'm simply going to delete them myself because I can't read your mind and never will be able to.
Re:How can ... (Score:5, Insightful)
No I feel you should comment it out for one version, or one iteration.
The problem with deleting code is that you lose functionality and information. Yes yes we have this ideal world where unit tests will ensure that the code only does what it is supposed to. Be that as it may, there is a reason...
Code that needs to be completely rewritten is crap code. We can argue how it came to be, but the reality is that every developer for one reason or another has created crap code. It is unavoidable. Crap code is code that does many things, but quite a bit of it incorrectly. It also is hard to get a grip on because of its complexity. For if it was easy to understand and easy to fix it would not require a rewrite, no?
Thus when you rewrite you are trying to simplify, and restructure. And because you don't have a handle of the original code you are going to introduce bugs in the new code. These bugs are new cases that you have not thought about and thus need thinking about. They are not critical bugs since the rewritten code is easy to understand and easy to fix. BUT these bugs need to be cross referenced with the original code. You need to see if these bugs are bugs, or actually the correct answer. Sometimes I was working on code and thought, "crap, the new code is right even though I thought this was a bug."
If you delete, and need to check it with version control you are adding time, effort and complexity of actions. And then instead of going back to do a cross reference you are going to introduce new bugs that can grow into weeds and cause this cycle of rewriting again.
I have found that often when I rewrite I learn new facets of my code and figure out the critical bugs. It is then I decide to stop the rewrite, and integrate the new code into the old code and fix things up. For the I see in the rewrite that the new rewritten code is going to introduce quite a bit of new code that is going to mean retesting, and assurance that things all work. And maybe this new rewritten code will not be better after all. Deleting the old code means me merging the refactored code, with the old code introducing even more complexity.
Commenting out is ugly, but it has made it easier to experimenting in a short order of time without causing version control havoc.
Re:How can ... (Score:5, Insightful)
If the code was replaced due to a change then the old one wouldn't work anymore anyway. If the old code was too clever(mostly too kludgy and not comprehensible) then do away with it. If the replacement causes any issues then use diff to find out what was changed.
I'm quite brutal when it comes to purging stuff like that. And I have been known to be rather sarcastic when I find stuff that isn't covered via unit tests. But that is an entirely different matter.
Maintaining code is hardly rocket surgery but normal housekeeping. There comes a time when the old pizza boxes have to go. Ideally before the roaches move in.
Re:The more..... (Score:5, Insightful)
I comment the code and leave it there for at least one minor version. If things are all right, I'll remove them. The code is in source control anyway.
That's a fine sentiment for a one-man dev team with a relatively small codebase, where you know what you're doing and can keep a handle on things, but for larger teams, people who leave commented-out code in place are annoying for everyone else.
As you said already, it's in source control anyway, so why bother leaving it there even for one version?
The counter arguments are that it causes additional diffs and commits that are unnecessary, and it gets in the way for other developers -- it harms readability.
And very often, despite best intentions, it ends up being left there for a lot longer than was needed. The original developer may not come back to that file for some time, and after just a few revisions, no-one is quite sure which bits of commented out code should be deleted and which should be left in place. Or maybe even maintained -- after all, if it's been left in place, that implies it might have to be re-instated, so if the code around it changes, the commented out code really ought to also be tweaked to keep it in line.
No-one has time to stop and figure it out either; the temptation is always just to leave it well alone -- if it's been left there, then there must have been a reason, but it's not my problem, so I won't touch it. End result, a year or two down the line, and we have huge quantities of dead code floating around, which probably wouldn't work anyway even if it was re-instated, and someone has to waste time with an excersise to go through the codebase and remove it all.
In your world of your own little projects, keeping dead code floating around may sound like a good idea, but in the real world it really isn't.
Re:That's what I do too, (Score:5, Insightful)
If the timeframe is borked, then I need to know so I can talk to the client about cutting features, postponing release, release in stages, etc. There is a lot you can do. But shoving out stuff that doesn't work or is unrelyable is something that will never be properly resolved.
If you desperately try to meet the release date and you actually somewhat succeed, then the books are closed on that release. But if you spend a lot of time after the release on bug fixing then eyebrows will be raised and your competence as a developer will be questioned. It's not your job to make management happy. It's management's job to make your job possible.
On unrelated news: Overtime is a clear indicator of management failure. Also massive overtime to meet deadlines never work and is never worth the effort. Been doing this for 15 years and I yet have to encounter one instance where this actually proved to do more good than harm.
Re:The rules (Score:4, Insightful)
1. Good rebuttal. No, seriously, proof by dogmatic assertion is great.
Of course it causes all these things. If you're refusing to modify existing code, then you can't refactor it to handle extra cases, which is going to lead to unnecessary duplication (and probably hideous cut/paste code). It will expand the size of the codebase, because this is what happens when you add code, which will eventually cause problems on some platforms (trust me - I've been there).
2. Turn in your coding license. Now. Except for a very small subset of coders working on very specialised projects and using formal proofs of correctness, anyone who claims their code contains (or should contain) no bugs when first written is a fool, and a dangerous fool at that.
3. That may be your experience. Rolling back version control history to find out where defects were introduced can be very useful, and if the tools are good enough (which they are) it isn't exactly difficult either.
As for the idea that you can't track all dependencies from the deleted code to the whole system, if that's true to the extent that you claim then your design is screwed.
Re:How can ... (Score:5, Insightful)
If rewrites are too complex you should split them up in phases. This is something few developers do, and something that can help you to test that the replacement code that you write does indeed what it's supposed to do. Between phases, testing is necessary - the more you can afford to test the new code, the less bugs you'll find later.
As a general rule, leaving commented code into commits that you make to the main repository is a bad practice. The general idea is that if you do things right, by the time you commit the code you should be pretty confident it does what it's supposed to do - and I might add that this is the main reason why I think rewrites are NOT for any developer. Attention to detail and thorough testing are a must.
When you commit commented code, you confuse other developers and add nothing of value to them. In most cases where I've seen devs do this, it's mainly because they are afraid they might need to roll back due to a lack of testing on their side.
And as a last note: version control is there to offer roll-back support, comments are not. It's about using the right tool for each job.
Re:How can ... (Score:5, Insightful)
Re:How can ... (Score:4, Insightful)
If rewrites are too complex you should split them up in phases.
Yep. In the VCS this you can easily do this using branches, doing all your development and testing outside of the trunk and then merging in a single commit back into the trunk.
The general idea is that if you do things right, by the time you commit the code you should be pretty confident it does what it's supposed to do
Not (necessarily) with branches. You can commit early and often since you're not changing the trunk. That means your commit log becomes a great timeline of the changes being made, you can almost always find older revisions of code to revert to and if anything goes wrong on your development platform you have a recent backup, which is always a good thing. If a commit is faulty then you can either correct or revert, but since no-one else is using your branch yet your faulty commit doesn't matter.
And as a last note: version control is there to offer roll-back support, comments are not. It's about using the right tool for each job.
Yep. In my view VCS is to provide rollback/backup/history. Comments are to explain actions being done to make code easier to read/follow, and in complex sections of code to explain the functionality to make them more intelligible (as an example to explain an algorithm being used, perhaps along with pseudocode for more complex ones). Comments should let you completely understand a program if you only have the single checkout, but don't need to give you any history.
Re:How can ... (Score:3, Insightful)
If you delete, and need to check it with version control you are adding time, effort and complexity of actions. And then instead of going back to do a cross reference you are going to introduce new bugs that can grow into weeds and cause this cycle of rewriting again.
Spoken as a fool who doesn't use Git. Seriously, get a grip man. Version control works. I use commits as my "save" feature, I branch a codebase 2 or three times per day. I keep merges (mostly) sequential. Here's how I switch back and forth between two different branches right in the same source directory, so that I can test old "deleted" code vs the new re-write:
$ checkout old-version-name
Now I can make an out of source build of the old version... Then make a new branch in the same source directory.
$ checkout -b new-version-name
Now I can make an out of source bulid of the new version... make some changes, and then save them.
$ git add . && git commit -m "Frobbed foo to make bar comply with baz"
Need to make a change in the old version, or look at some code?
$ checkout old-version-name
It's better than SVN or Mercurial because you don't have to manage a bunch of folders of different branches, the files change automagically when you switch between your local branches. My IDE alerts me if the file's I'm about to edit were changed out from under it and need to be reloaded, so if the files need reloading then that's what I do. I use the diff viewer if I need to reference the old code while editing the new, or make another clone of my working repository with the old branch if I'll be doing that a lot. Seriously, you need to learn about distributed version control -- I don't need to worry about not committing something that's not perfect yet for everyone to see, I just commit it locally, and rebase all the ugly changes so only pretty gets seen in public -- all these commits and branches are on my local machine, so I can actually USE the version control rather than be its slave.
Oh what's that? You use CVS or SVN and so you don't have a choice? BULLSHIT. Init a Git repo within the SVN checkout. Set git to ignore .svn folders and SVN to ignore .git. Make a git branch for "master" be the SVN branch you submit to the centralized repository, then you can branch your heart out and clone like there's no tomorrow, and merge into master, then make an SVN commit when you've got shit sorted. Accept that you've been a moron about version control all your life, and actually learn to use it. The Truth Shall Set You Free!
Spoken as a COMPLETE fool independent of version control.
In other words, an out-of-control fool.
Out of all the code you're working on, HOW WOULD YOU KNOW THERE'S DELETED INFORMATION IN A PREVIOUS VERSION OF ONE OF THE MANY FILES?
You can't know that unless there's something there to tell you. And you're NEVER going to spend much time looking into a comment like /* deleted old processing and rewrote. 2009/11/4, A. Programmer */
Yeah, you're going to dig through old versions of the source just to see what happened?
No the fuck you're not.
Re:How can ... (Score:5, Insightful)
"Code that needs to be completely rewritten is crap code. We can argue how it came to be, but the reality is that every developer for one reason or another has created crap code. It is unavoidable."
I have found this to be the case for all code that comes out of TI for their MSP430 embedded processors. They use a convoluted system for their code that only a nutjob would love. I have thrown out their drivers for interfaces and rewrote them by hand just to avoid their crap. You DO NOT declare a function for your device driver in the .h file of another driver. WTF is wrong with their programmers?
Re:The more..... (Score:5, Insightful)
> remove after it has shown to work
Seriously? Do you mean you don't have unit tests and quickie smoke screen tests for developers to run before they check in to prove their new code works and hasn't broken anything?
Tell me where you work, I don't ever want to buy one of your products.
There's unit tests, and then of course the ultimate test: the production environment. If you set up perfect unit tests every single time, then you won't notice the difference between the two. If you're human like the rest of us, then every so often an assumption you hadn't even realized you'd been making will slip through your unit tests and you've got a production bug. But hey congrats if you've never promoted a bug to production before.
Re:How can ... (Score:4, Insightful)
Explain why it was removed (Score:4, Insightful)
When removing code, consider commenting why it was removed. I've seen cases where code was removed then added back in causing bugs to reappear.
Before:
Log(“Shutting down”)
CloseLog();
Reboot();
After:
Log("Shutting down”)
Reboot();
Better:
// Don’t close the log file here because some other thread might still
// write to it during shutdown/reboot, which could cause an error dialog
// that prevents shutdown. The file will be closed anyway by the OS.
Log(“Shutting down”)
Reboot();