Sniff Out That Smelly Code
As time goes by, things go wrong with any body of code: we get lazy, bad developers contribute code, the original clean architecture gets forgotten, and so forth. Some bad code is easy to spot: it simply “smells” – once you see it, you know it’s bad.
Refactoring this sort of code to remove the smell isn’t often that difficult – and the time taken will be repaid next time you have to make modifications.
The following are my ten worst “odors” in code:
1. Long Functions
Over time functions often get longer and longer – extra functionality is added, edge cases are handled, and so forth. However, long functions are difficult to change, and difficult to understand.
Each function should ideally do exactly one thing: you should be able to sum it up in one sentence (and the method name should be a summary of that sentence).
If you find a function is getting too long, split it into sensible parts. Each part should follow the above rule.
2. Commented-Out Code
Some of these rules are open for debate. Not this one. I’ll go so far as to say that there is never an excuse for commenting out code.
- Are you trying to preserve some sort of history to the code? You should be using source control.
- You’re not sure if the code might be necessary: find out – and don’t commit until you’re sure.
- Perhaps you’re in the process of shotgun-debugging: step away from the code, and work out exactly what’s going on.
3. Copy-Pasted Code
Like commented out code, there’s really no excuse here. Programming IDEs should only support ‘cut’ and ‘paste’ – there’s no need for ‘copy’.
Instead of copying code, refactor it into a method, and call it. Copy-pasting is lazy – period.
4. Handled-but-Really-Unhandled Exceptions
I need to explain here: I’m not suggesting that every function should handle all exceptions that could be thrown. Rather, I think the handling of exceptions falls into two categories:
- The function can handle a particular exception, and takes some action to correct it.
- The function cannot handle a particular exception, and does not attempt to catch it.
What I’m referring to is code like this:
void Foo()
{
try
{
// ...
}
catch( Exception e )
{
// Throw exception away, or just log it.
}
}
In general, doing nothing with an exception is a mark of bad code. If the function can’t handle it, then it shouldn’t be caught. Alternatively, if it didn’t need to be thrown, then try to amend the throwing code.
(Especially bad is throwing and catching the same exception in a single function. Exceptions should not be used for flow control; in situations like this, the entire function needs to be re-thought).
5. Large Numbers of Parameters
Functions taking large numbers of parameters (say, more than half a dozen) are usually a bad idea, and are generally indicative of either a function trying to do too much, or poor class organisation.
Often a function will grow to accommodate new functionality, and hence grows parameters. Split the function into separate, smaller, simpler functions.
Alternatively, if the function absolutely cannot be split, consider introducing a new class which encapsulates some or all of the original parameters, and passing that class as an indirect parameter.
6. Non-Obvious Names
Variable or function names such as foo or bar, swearwords, names, or otherwise clever or amusing really don’t have a place in professional code. They might make you laugh, but when someone else tries to read your code, they won’t thank you. (And remember – that ‘other’ person reading your supposedly clever code might be yourself in six months’ time).
The use of i,j,k is prrobably a little more open to debate – however their use as loop indices can usually be avoided if your language or libraries support more generic iteration constructs.
7. Deep Nesting
Just as functions shouldn’t be too long, so they shouldn’t be too wide. There’s no hard-and-fast rule, but if your conditionals or loops are nested more than about three deep, you should consider refactoring your code. Either pull the inner parts of the code into separate methods, or pull the complex conditions into functions. A line of code should never exceed approximately seventy characters (the standard editor width).
8. Beacon Comments
Sometimes you don’t even need to smell bad code – the original developer has helpfully pointed it out in the comments! If you see comments like:
// This is hacky... // TODO: This is bad. FIX IT! // This code ought to be refactored
…then you know something needs to be done.
9. Narcolepsy
…by which I mean inappropriate sleep() ing. If your code requires an (essentially random) sleep interval to function correctly, it’s likely to fail if the user’s machine is especially old, especially new, or just busy. Try attaching to an event that signals when the situation you want to be in occurs.
Another related smell is relying on the speed of the computer, the speed of video frames, or similar, to achieve a particular timing effect.
10. Helper Classes
This is something I’m guilty of from time to time, and I’m looking for comments as to the best way to avoid it. Helper or utility classes always seem to grow in large bodies of code, usually containing unrelated static methods. These clumps should be analysed and split or moved into classes that have one task.
“Copy-pasting is lazy – period.” – crap – it is invaluable! I hate this so called code guru crap – sharing your wisdom! just ’cause you think it is right doesn’t make it so – you simply have an over inflated opinion of your abilities as a techy, period!
Very apt points, Mike Like.
I’m especially guilty of number 8 – I was looking over some old code just today and I swear every 4th function was commented “//Unhack this s**t asap”.
But nice article, this is one stumbler giving the thumbs up
Good List, Thanks.
I disagree with #4. I prefer not to handle exceptions because unhandled exceptions have the benefit of being noticed. Also it takes less time to write and maintain. Generally I find it easier just to throw errors in the backend – unless they brake something – and simply catch and log/display them in the frontend.
I’m not so sure that just using classes to pass parameters that make the code unwieldy improves anything. Steve McConnell in Code Complete makes a statement that this is a needless process, even going so far as to say that it is needless architecture. That being said, I do it sometimes too because the trade off (that Steve acknowledges as well) is that it makes the code more readable.
As far as utility classes…dunno. Each method ends up needing review to see if it can be re-factored. I haven’t run into this too much because I tend to chunk logic things into their own classes, even if the classes are small. But that also can lead to over complicating the architecture.
Ok article, found it by Stumble Upon.
–Joe
3. Copy-Pasted Code
Your advice is not correct if you are copying from one project and pasting in another one. This is really useful for entire functions that are already loosely coupled.
Hello, I like your points, I also try to follow this philosophy. About point 9 : I would really like what is a good reference to get a good timing effect (fixed, example : 1 second). I mean : I write in C#, as you, I see. I’ve tried classic windows.form.timers, system.threading.timers, multimedia timers, etc. All of them, after some minutes, and even worst on different machines, are totally unrealiable… the timer loses its real world timing. To which “event” could we believe, to get a real world timing effect ?
thanx for any reply and thought, i’m quite astonished (never thought that even powerful cpu could be affected by this type of problems…)
@ angry:
relax dude
Regarding #8. Could you please help to refactor/factor out the following comment?
// Ugly hack. I cannot use
// Application.Current.MainWindow.WindowState = WindowState.Minimized;
// because window reappears in the task bar and becomes completely black
// on restore.
NativeMethods.SendMessage(_hWnd, NativeMethods.WM_SYSCOMMAND,
new IntPtr(NativeMethods.SC_MINIMIZE), IntPtr.Zero);
Beautiful summary, I LOVE it!.
I do disagree with the literal interpretation of #3… while I agree that redundant code belongs in another function, I frequently copy-paste calls to those functions and replace parameters. I deal with with a lot of parsing logic, so I’ll make the same call dozens of times with different parameters. Even if you say to put those parameters in an array… I’ve still got to populate the array!
As for moving parameters to a class in #5… I’m not going to do that just to reduce parameter count on my function, but long parameter lists may be a red flag that some of the values are related closely enough that they deserve a class representation (at which point you need to consider moving the method into the new class as well).
To angry: chill dude.
@angry & @tyler — He’s saying that if you’re trying to do the same thing in several parts of your code, it’s better to just make that one thing into a function and call it multiple times… instead of having the same chunk of code pasted all over the place.
He’s not saying never use copy/paste ever again in any case or you’re a bad programmer… although if you can’t discern subtleties like that, maybe programming isn’t your forte.
I really agree with most of the notes… but with a few exceptions.
In regard to copy-paste its more a question of why you copy a piece of code. Sometimes its true the copied code should go into a helper class… (whops, item 10), but sometimes I personally think its better to have redundant code. If the code is separated from the logic of the original code, I prefer redundancy. Unless you have a well written and automated test system in place it is dangerous to have code that is shared across a too big area of your business logic. If you business logic contain several separate modules they should only share helper classes that is really clearly generic. If they are at least specialized they should be made double if two different business modules need them. Over time someone (your self?) will do a change to the class, and end up with the other part of the system failing.
This issue is clearly related to item 10 – the helper classes. I don’t consider helper classes a bad thing, but it is important that they are clearly generic. Any piece of special cases and stuff is bad.
In regards to exceptions most languages allow for catching specific exceptions. So if the function can handle a null pointer exception it should catch and handle this, but not catch all other exceptions.
But when you release the code (to the non-technical users) you should always catch the remaining exceptions, log the to a file (that you can look in, identifying the problem) and display a useful error message to the user.
I have an other item that I uses to identify bad code: Lots of comments. If each section of code starts with 15 lines of comments telling what the code will do, its usually because the following code is not well written. A single comment line or two may be okay to help with the overview – but generally the code should be readable in it self. I once knew a programmer who said: “Comments are excuses for bad code.” And in a sense I agree.
I greatly disagree with Deep Nesting. 3-deep and 70 characters are an ancient standard from the size of terminal screens. I’d say about 5 or 6 deep and 120 character are the maximums nowadays. Granted, it also changes from language to language – I tend to stick with extremely expressive languages when programming for fun, where doing such a thing makes it far easier to read.
I agree on some points and not so much on others:
1. I agree. Though commenting blocks of code in a function can help to browse it, a function that is too long can sometimes leave you wondering where one function starts and another ends.
2. I would disagree that commenting out code is a bad thing, though I do agree that once you have completed your changes and are ready to submit to source control, those comments should be removed.
3. I completely disagree here. It is sometimes much more time efficient to copy a similar function and change it’s implementation.
4. I suppose I have no objection to this. Though I can’t say I’ve ever seen an example of this.
5. Completely agree. More than four or five parameters is probably overkill and can be difficult to keep track of. I also agree that a class (aka struct) can be used to pass a large number of parameters.
6. Agreed. Though I often use i, j, k, etc as generic iterators unless more clarification is needed for readability.
7. I agree that too much nesting can be troublesome, but proper formatting, commenting, and spacing can help a lot here when breaking code out to separate functions doesn’t make sense.
8. I don’t know if the author is opposing the use of these kinds of comments or just noting the fact that they point out code that may need to be changed. Sometimes project time constraints require the use of these comments. Of course, an identifier should be used (like “TODO” or “HACK”) so that these can be easily found and fixed when time allows.
9. Agreed. This isn’t the ’80s anymore.
10. I agree. It’s usually not too difficult to categorize methods in such a way that only relevant methods are contained in a class.
11. Feel free to ignore any above rule if you know what you are doing.
Especially, when dealing with code that needs to be fast or small or compiled by a compiler that is not inversely proportional to the coder’s downfalls. Also applies if coding for fun as opposed for a living.
I think being anal about how the code looks and works (while not a bad thing in itself) is something comparable to premature optimization: it’s very important to recognize when it’s just not worth it. There’s much more overhead in designing a class and writing working code than just writing messy but working code. If it’s not clear what it does, then learn to code.
After these tongue-in-cheek food-for-thought words: I still thought the article was a good read, especially for the youngins and self-learners.
@angry.
omg you can’t be serious! c/p code is the worst code ever.
what happens if there’s a bug in the original code? you gotta fix it n times.
what happens if there’s an future optimization in the original code? you gotta fix it n times.
what happens if there’s an update in the original code? you gotta fix it n times.
if you c/p code three times or more ur doin it wrong!
i’ve gotta deal with maintaining crap code from offshore and these fools are guilty on ALL counts. sometimes they c/p 120+ lines just to change 3 lines at the end. I defy you to defend c/p code in that case!
Good information, I am only a follower of this website
Thanks for sharing the conclusions of your coding experience thus far Stu.
On Point 4, I feel that handling exceptions within the context that they occur is expedient and when judiciously implemented can result in a very robust application. Much like a device driver crashing shouldn’t crash the OS, a particular function, eg printing a report, shouldn’t crash the entire application. In effect, the handling of exceptions, will generally be a combination of many actions – log to file, display on screen, handle in function for sane results, etc – the choice of each action depending upon the context of the exception’s origin.
Sensible function and variable names, raised in Point 6, have been invaluable to me in my code revision experience. It has reduced the need for some comments, effectively cleaning up the appearance of the code, and allows one to look at a set of functions/classes in a given architecture and easily build the mental mapping of how things fit together – familiar things are easier to remember than exotic/uncommon ones. As the paradigm of the architechure becomes familiar/apparant, the actions (classes/functions) and their use too become apparent. Freeing up the mind from having to think of what things are, to, how they can be used.
Point 9 was the one that stood out the most, especially this line:
“Try attaching to an event that signals when the situation you want to be in occurs.”
Simple but effective. This can be especially handy for building intelligent UIs or applications where tasks need to be completed in a particular order and the return status of each raised event can dictate the direction of the application in a more dynamic manner, a manner controlled more by data than by code.
Utility classes/functions exist, they help glue it all together. If named intuitively and grouped appropriately their use should become apparent after a little study.
These are some of the things that I’ve discovered whilst writing software. Some of them may sit well with you, some may not. If you wish, integrate the ones that do and cast off the ones that don’t.
Peace and love to all things
some of the anti-patterns (http://en.wikipedia.org/wiki/Anti-pattern) explained
Oh, come on. Rule number 1 of programming is that there is almost no rule that applies to all situations.
Your rules may apply to some situations (and even given that, I disagree with some of them), but bolding ‘never’s is just being narrow headed.
Some of your points are good guide lines (I especially agree with 1,5,6,7 & 9 ), but even for #7 – think of situations such as real time programming, where the overhead of calling another method cannot be tolerated.
In addition, in many cases making the refactoring you suggest just doesn’t worth the trouble, even if ideally we would want it to happen. Keeping your code clean and easy to handle isn’t the top priority, but it is a mean for other goals (adding the feature on time etc.)
Also stumbled upon it
Some excellent points. As asaf points out, they’re not all necessarily applicable to all situations… but if most are kept in mind for most of the time, you should end up with generally cleaner code.
Most of it’s just down to laziness I think. If time was taken, most authors of “smelly code” would probably have done it how you suggest.
One last point – I saw in your “stumble” message about your ads not covering hosting and actually went in hunt of them to see if I could help out with a click… but I can’t seem to find them here or on the homepage. Whether I’ve just gotten so good at filtering ads out that I can’t see them when I try, I don’t know.
@angry – I’m not claiming to be a guru – I try to write as if for the “me” of say five years ago. Please do feel free to disagree – I’m always learning. I expect I’ll look back on these articles in a year or so with a certain amount of embarrassment.
@Tyler – agreed on copying between projects (I was thinking of one single project) – although I’d call copying between projects to reduce coupling “branching” (in the source control sense).
@Alex – agreed, not all smelly code can be removed. Where a hack is the only way to get things done, then fine. However I’d say that code that has a lot of them is smelly. One hack is a barely noticeable smell; a few dozen is downright stinky.
@jp, @coder, @Asaf – Agreed, these are really just guidelines… I was trying to put across the point that experienced developers develop a “nose” for bad code.
@Izkata – I think we both agree on the general idea, just not the exact figures.
@angry – the big problem I’ve had when programmers (including me) copy/paste code is that in many cases the pasted code will compile just fine but it doesn’t necessarily work as expected in its new context. Typing the code forces you to read it, which means you are much more likely to spot these sort of gotchas. The few extra seconds/minutes it takes to type in the code may save you a lot of debugging time. Plus, if the code is really identical, it should be factored out.
Just code, fellows!
Use your skills at your best. Thw wood would be a very silent place if only nightingales where allowed to sing….
Number 9 is a biggie in my book. There are just numerous subtle bugs that can surface when you try to create code that depends explicitly on time to function correctly. Unless you are building a real time clock or something that measures time directly it almost always better to use a boolean flag to to check that the proper function has been performed instead of timeouts, etc.
“Long functions”
Generally that one sentence describing it’s functionality shouldn’t involve the word ‘and’.
I copy and paste all the time while coding. Usually I copy-paste something and then modify it in a way that’s not easy to put in to a function. It’s completely different code, but it started with a copy of other code to save me a bit of typing.
“Handled-but-Really-Unhandled Exceptions” happens a lot with Java because of the checked exceptions and crazy library developers making their functions throw lots of useless exceptions that you are sure won’t be thrown.
The Copy Paste idea is just to much.
I mean if your refactoring your code your ARE copy and pasting the pieces you want to abstract into a new method. Sometimes I’ll grab an entire class that I want to break up, copy it into a new file, then proceed with hacking.
I’m bemused to read people defending copy and paste of code – obviously its not 100% bad, but it is 99%+ bad.
Code re-use is such a simple principle, but even in a professional environment its common to see people have not learnt this lesson.
To those who still copy and paste without due care – YOUR DOING IT WRONG!
I read point 3 differently to the way several commenters appear to have done. I took it to be saying that copying whole chunks of code and placing them elsewhere within the project without modification is a bad idea. I agree whole-heartedly with that interpretation. It may be faster right now to copy that code chunk if you need the same effect elsewhere, but you’ll regret it as soon as the code needs bug-fixing or updating, as pointed out above.
I’ve done it to myself, and had it done to me by other people when maintaining their code – I’m sure we all have. Avoid the temptation to be “quicker” and you’ll be happier in the long run.
@Izkata and other people who think that deep nesting is a code-formatting issue, this is a misconception. You should avoid deep nesting, particularly of conditionals, because it is a sign of deep *complexity*.
It’s not that the code might run off the side of the screen, it’s that the code has vastly more possible paths than a programmer can hold in his poor little head.
See http://en.wikipedia.org/wiki/Cyclomatic_complexity
@John Farrell – I have no problem with cut-and-paste (or copy-and-paste where the original is later deleted) – it’s the duplication of code I object to.
Angry: you’re an idiot, any person that has coded longer then two days knows that repetitive copying and pasting of code chunks is a terrible idea. So for example you have a switch case statement you use over and over again to return some value given a string – what happens when all of a sudden you have to change it to return another value for a different case or remove a case that is no longer useful/valid? If you’ve copy and pasted code several times across several files you’ll have to find and change each one. And hey, guess what? You probably *wont* be the one doing the changes. Idiots like you are why I have to spend half my work day refactoring code. So before you start ranting about “code-guru crap” why don’t you actually think about why you might want to do what the writer suggests. It is ironic that you accuse the writer of arrogance when the only arrogance I see is yours in thinking your limited experience lends you the wisdom to make judgments about good practices. You’re probably some idiot hobbyist or first semester CECS student.
I code in ABAP which comes from the COBOL side, not the C side. However, what you have said applies to ABAP as well. Especially hard coding in values which change on every 11 – 12 years!
Keep up the good stuff!
I have commented out code, because I am planning for later, but want to get everything else working first.
This list is pretty good. Most of the items are old standbys, but it is good to put everything in one place and get a bit of discussion going. A hard question is how to correct some of these things once they have been identified (hence the need for TODO and HACK comments).
I would like to make two comments. First names of “variables” and “functions”. It has been my experience that class members should have appropriate names as dictated in the rules. But if your talking about local variables I think it is better to develop a naming naming convention that reduces the problem of reading. Variables like i,j, and k are vastly superior to something longer. These are standard conventions that every programmer should understand and no one should ever have to wonder what “i” means.
I think local variables should be short and easily understood from their context and I think giving them long natural english names just slows everyone down. When coding we all have standard implementation idioms that we use every day. People on the team should all understand these idioms so that seeing it once cues them into a way of reading. For example, I have a object that is often used called a StoreDescribeContext. When we use this object locally we use the first letter from each work (sdc) as *the* standard local variable name. The use of such conventions speeds up communication, removes confusion (e.g. do I have to come up with a new name for a temporary StoreDescribeContext every time I want to use one?) and saves screen real estate. I think if you methods are really the “right size” and they do “just one thing” the use of this type of implementation idiom helps code.
The second thing I would like to say is about exception handling. I am programming mostly in .NET (so the compiler does not check for uncaught exceptions). The programmer can never really know what exception might be thrown (unless you want to do a hierarchical tour of the system documentation every time you call a method) and so generic catches at not uncommon. I would like to state that this practice is bad and we should always catch only the exceptions we can handle. I prefer to explicitly test my assumptions with Asserts (and the biggy here is for null pointer exceptions) and once the assumptions all pass you are only left with exceptions you can handle and those that you have to re-throw anyway.
Excellent Stuff. Though I’m not sure I’d really want COPY disabled — like others have said, there are legitimate uses for it. And with everything, if you get anal about it you’re probably missing something else the size of the elephant, for example, when you do a completely factored, totally bug free and elegant solution to entirely the wrong problem!
The only reason you should not leave commented code is if you are refactoring and the old “// THIS IS HACK” function is no longer needed. If you are working for a client who likes to change things every other day “I like this, but no lets go back to the old way, oh wait i liked it the other way better but can you add this in? oh i changed my mind we don’t need that. oh today my accountant called and says we do need that!”
well if you always delete the old code, we would have to rewrite it again, costing more money and more time.
I disagree with everybody saying that commenting code is bad. Read your own code months later (or a couple of years later), and one line of comment within a function or procedure, can save you valuable time in remembering why you did this or that.
Don’t be arrogant thinking you will remember every single line of code you wrote, because you won’t
And may I add, that you will be maintaining code done by somebody else, that might know more than you, or somebody more junior than you will maintain yours. We’ve all been there.
You get to dig into stuff you’ve never seen before, and without comments (and I don’t mean writing a book inside the code, but one liners in plain english or whatever human language you speak), you, or whoever comes after you, will waste lots of time trying to understand what is there
Copy-pasting is lazy – period. So true.
Just on point 2, I often find myself commenting out perfectly good code that I need to retain because my clients often vascillate on whether they want a particular feature/function. I’ve had cases where I’m told to remove a particular useless block of logic in the morning, and by afternoon the world might end without it. My strategy is to comment out the block with an explanation as to why it got canned:
//The bossman told me to take this out in the morning – 08:00am – 01-01-2009
//It does this that and the other, but if you need it again just un-comment
//the bugger
//SupposedlyUselessFunction (1, 2, 3, 4);
I don’t really think it’s that much of a sin in this context. Just saving myself grief from real world idiosyncrasies.
Number 3: Another ok use of copy and paste is if you plan to immidiately refactor it. Sometimes when writing new functionality, you know you should delegate some piece of repeated logic to a seperate method, but you can’t wrap your mind around exactly what it should do. This is more common if you have variations or special cases you need to deal with. Instead of letting my brain spin in circles, I find it better to just take the quick-and-dirty approach and copy-paste code. Once I get the functionality working, I know I have all variations coded in front of me, and can more easily determine how to pull out the repeated bits into seperate methods.
Number 8: I think it’s ok to mark your code as a hack, ugly, or needing refactoring IF it’s a last resort and you would otherwise just leave it. Using a special marker like TODO is helpful, espeically if your IDE parses these and builds a list for you (Eclipse). Also leave a comment indicate why you think there’s a problem and even a suggested refactor if you have something in mind.
The author might argue you should never leave code like this if you can fix it, but I can think of a few reasons: you don’t have time (deadlines; end of the day or week, and you don’t want to forget to finish the job), it’s just not worth it, and the bite the first bullet approach. The idea with biting the first bullet is that refactoring intends to make future changes easier, but you should wait until 1 or 2 of said changes are required before doing the refactoring. This lets you identify the sorts of changes that will be common and refactor to make those specific changes easier. Otherwise you can end up with speculative generality. See Martin Fowler for more on refactoring techniques.
@marcus uy – I think that’s the first situation the author identified: trying to keep code history. You should use source code control instead, it’s much cleaner and will help you in many other ways. One exception might be that the changes are so quick that you haven’t committed one before you made the other. I think in that case, it’s ok to leave the commented code, and clean it up the next time you make changes to the file.
Lol,
For all you developers out there, I believe the next version of any IDE should have the cut/copy/paste functionality removed. I have seen some crappy code out there, and cut/copy/paste is often the culprit.
If you had to copy the code by hand, chances are you’d take the time to think about the design and try to improve it instead of copying it.
I’m subscribing to this blog
Whether you agree or disagree with these being true coding sins, each of these points should at least be covered before release date, without a doubt.
secretGeek talks about copy/paste:
http://www.secretgeek.net/copy_paste_dont_do_it.asp
Very well written article. I agree with all the points though I’ve been guilty of some in the past – particularly 8.
- thumbs up on stumble.
I love the number one rule!
One job (even so small) should be handled in one function!
Not everything is quite so black and white. Copy / Paste is good sometimes bad others. No good coder should rule out anything available to them.
I always remember one of my lectures giving us the ‘Never, never, never, under any circumstances use goto’s in C / C++’ then in the _very_ next sentence proceeded to praise the wonders of the switch statement!
Single point of maintenance for the code, boyz. Some very well thought out points in this one – glad I ‘Stumbled Upon’ this…
I have one rule of thumb: You should have no rules of thumb. After a while, you start to treat them as laws, then a little after that you have forgotten why, but you still follow them.
“No nesting beyond 3 deep”…..
“No more than 6 parameters”….
“No commented code”…..
What you are actually talking about is a matter of style. The problem is, instead of comparing / contrasting style, you are making pointed remarks about code quality if it doesn’t match YOUR style. This type of commentary smacks of ego and condescension. Since when do you set the standard by which code should be judged?
Tell you what: If you’re working on open source, it’s open. So if you don’t like my code, YOU GO FIX IT!
I might argue that the best application has to start smelly. That coding with abandon at the outset and then refactoring during subsequent iterations actually yields modular code of a equal/higher quality in less time. I think a lot of hte overhead associated with oo programming comes from a failure to embrace the unpredictability of most application lifespans. Programming has similarities to plain old writing. Even the best authors don’t crank out finished works. Programming involves a lot of editing.
This is a great article and my guess is that programmers who take offense to some of these points probably realize their code could use improvement. That said, I don’t think author’s rough drafts are “smelly” and neither do I think it’s an apt term to describe code that simply hasn’t been polished. I further agree that leaving code unpolished is entirely appropriate in certain situations… there may simply not be the budget there to refine it.
Anyway, all that said, great list. Thanks stusmith.
I use copy paste all the time
Not to avoid having to call a function, but im so lazy when it gets to writing out a switch case. I just copy an old one, change the argument, and adjust the case and outcome. When beign a sloppy person, c/p a case switch avoids a lot of syntax errors.
Also c/p a variable name helps. Especially if u programming say JAVA, which is case sensitive. It solves a lot of syntax errors too
Great stuff! I agree with just about everything. Sometimes, especially when I am working with C and structs, I get extremely long lines of code (in width of course). I tend to assign a pointer to these if theyre too long in order to make them shorter and clearer, but sometimes that confuses other people who have to read the code, even when using a comment. Any thoughts on that one in either direction?
The #3 has a obvious mistake, if an IDE has only “cut” and “paste” functionalities the developer could “cut” and paste any times as necessary. Even if the “copy” was not supported, the cut can do the same just with one more paste, for the original content place.
Obviously it’s is not the point of #3 but …
[...] Smith of Hackification.com listed his 10 worst code smells: 1. Long Functions 2. Commented-Out Code 3. Copy-Pasted Code 4. Handled-but-Really-Unhandled [...]
^ LOL at above post
There is an exception (no pun intended) to point #3:
Let’s say you are creating a class and publishing it as a .NET assembly to a third party. This third party will not have access to the class’s code; they will only be able to instantiate the class(s) and execute its/their methods. They will only be aware of the methods themselves, the parameters that the methods require to execute and the result that the methods return, if any.
Now, if this third party sequentially calls several of your referenced methods from their own code, they may wish to know something more about how the methods operates:
public class ThirdPartyClass {
public void DoSomething() {
private YourClass yourClass = new YourClass();
yourClass.Method1();
yourClass.Method2();
yourClass.Method3();
}
}
For example, let’s say Method1 connects to a database. The third party may wish to handle any potential exceptions that may arise from this. The only way they can identify which exception they need to catch is to force their code to throw it, then amend the code, which is very inefficient from a design perspective. Let’s say the exception turns out to be a SqlException:
public class ThirdPartyClass {
public void DoSomething() {
try {
private YourClass yourClass = new YourClass();
yourClass.Method1();
yourClass.Method2();
yourClass.Method3();
}
catch(SqlException ex)
// do something with the exception
}
}
}
This is fine, but it will also catch any potential SqlExceptions thrown from Method2 and Method3 (they may not want this), unless Method1 is wrapped in its own try – catch statement; again, bad design.
Ideally, Method1 would throw its own custom exception and display this exception information in it’s metadata documentation:
public class YourClass {
/// Blah blah blah
/// Method1Exception
public void Method1() {
try {
// Do something
}
catch(Exception ex) {
throw new Method1Exception(ex);
}
}
}
Now the third party can target specific exceptions from your class:
public class ThirdPartyClass {
public void DoSomething() {
try {
private YourClass yourClass = new YourClass();
yourClass.Method1();
yourClass.Method2();
yourClass.Method3();
}
catch(Method1Exception ex)
// do something with the exception
}
}
}
Method1 can also display metadata information on any exceptions arising from methods called from within Method1, so that they can be targetted.
“Narcolepsy” for inappropriate sleep()ing (#9)
Really funny, thank you!
narcolepsy: +1
commented out code: anyone defending on account of ‘preservation’ confuses his source file (your precious gold) with a scrap yard.
— ‘We’d have to write it again’?! (*SHOCK*) Aren’t you using a revision control system for that? Barf…
— ‘Our users are vascillating between requirements’ — we use branches/patchsets for that purpose.
Learn to use version control. Do not (ever) comment out code (except during unit testing and before commit).
I willingly admit, I mostly code for fun and to create things that often only I use, so I am probably guilty of a lot of those things and don’t consider myself a ‘code guru’ or whatever. However, my mostly used language is PHP in web development type projects with MYSQL and I have to say that I firmly disagree with the copy and paste issue. I constantly am copying code from one page to another or one part of a page to another. Not because I am leaving it exactly as it is, but because the basic structure is there that I need. A good example is a MYSQL query. Whether I’m updating, selecting, or deleting, or whatever, each of those query types has a basic structure, and I use mostly the same vairables defining the queries and whatnot. So when I have a query that is updating 10 fields in one table on one page, and I have another part of a page or another whole page that is updating say 8 fields on a different table maybe, to me copying the 10 field query, updating the one word values and field names (by a nice double click to highlight the word) and deleting the 2 unneeded field updates is way easier and less time consuming than re-writing the entire update query with all it’s various fun coding symbols ({}[];”\’$?). This is just one example of an incident where I think that copy and paste is not only ok, but more effecient. That’s just me though, good article, and go ahead and tell me if you think there is a better way to accomplish such a feat. Myabe my carpals just aren’t as masachistic as yours…
Your nuts with some of this crap. Over the top idealistic. Give it a rest.
[...] 13, 2009 Coding Guidelines 1. Long [...]
Hi
First point the need or lack there of for copy and paste is I believe related to the IDE/editor and weither it supports auto complete or not. I prefer a simpler editor and use copy and paste for function templates etc…
I agree comments are bad in general largely because of seeing code like this.
for (i=0;i<3600;i++) /* 60 minutes */
Basically because the comment don’t get updated with the code.
so it should be written more like
for(i=0;i<60*NUM_SEC_IN_MIN;i++)
Regarding Function argument I agree because of the stack overhead incurred.
However also due to stack overhead I only partially agree with function breakup, unless function inlining is supported with the compiler.
I agree with almost every point, however even a copy operation is useful in an IDE of course not to duplicate a piece of code which is bad since you have to be DRY (don’t repeat yourself). but sometimes you really need it, for example in EJB2 if you are using xDoclet you may need to keep copying the function meta tags repeatedly. or for example to copy the XML for creating a struts action and then refine it… there are many situations that need a Ctrl + C
I have wrote an article about efficient software development please visit it:
http://omarkababji.weebly.com/1/post/2009/08/efficient-software-development-12.html
Omar.
Laziness is a programming virtue.
Nothing worse than having to go through line after line of somebody ELSE’s ill-written code…
Great article!!!
http://pythontr.jimdo.com/
– KIZILSUNGUR
[...] Sniff Out That Smelly Code (tags: programming code tips refactoring development coding oop) [...]