share
Stack OverflowWhat are Code Smells? What is the best way to correct them?
[+502] [162] Rob Cooper
[2008-09-22 11:34:19]
[ language-agnostic coding-style code-smell ]
[ http://stackoverflow.com/questions/114342]

OK, so I know what a code smell is, and the Wikipedia Article [1] is pretty clear in its definition:

In computer programming, code smell is any symptom in the source code of a computer program that indicates something may be wrong. It generally indicates that the code should be refactored or the overall design should be reexamined. The term appears to have been coined by Kent Beck on WardsWiki. Usage of the term increased after it was featured in Refactoring. Improving the Design of Existing Code.

I know it also provides a list of common code smells. But I thought it would be great if we could get clear list of not only what code smells there are, but also how to correct them.

One smell per answer, please.

(57) DO NOT add rules to your question. DO be grateful that people attempt to answer at all :) - OJ.
(52) The rules are there to try and help improve the quality of the thread and stop people typing before thinking. You will thank me later when you have a thread you can use as a resource. I am always grateful for input :) - Rob Cooper
@Ande, this is precisely why I said smell per post. This means we can treat answers more granular. This is how we drive up quality (as well as quantity) I keep trying to explain but people just seem to want to answer fast rather than well. - Rob Cooper
Question updated to include formatting, you have to understand, I can not clean up all the questions as they are flooding in :) - Rob Cooper
@RobCooper: Done as you have requested. I wish at some point a posting style guide were to be made. Perhaps with your rep you wouldn't immediately get it closed for being non-programming. - Ande
(1) The rules have nothing to do with the question. If you do not see them in action, think harder why people do not follow them, try nicer ways to guide them to common resources like faq. But who are you to force us follow your rules for answering the question? - Serhat Özgel
(9) The FAQ doesnt cover things like this. Im trying to provide a thread that others will find useful. For that we need to a little bit of control. And it worked. People have been great, we have some great answers that are readable and offer solutions. Job done. I am no one, but my plan was a success. - Rob Cooper
(29) Btw, the correct response to a smell is to hunt for the kinds of mistake it heralds, not to remove the smell. The treatment for gangrene is not deodorant! - Steve Jessop
Well, surely logic would dictate if you remove the smell, you know the mistake and have corrected it. You cant cover up coding smells with deodorant :P :D - Rob Cooper
(8) I think you risk doing exactly that with a "fix the symptom" approach. The question should be "what was wrong with my thinking when I wrote this, that I decided it was a good idea?", not "I've broken a rule: if I change the code to obey the rule then it's fixed". - Steve Jessop
(1) Pretty interesting reading this. All the the top voted answers are covered in "Code Complete". Time to put that on the required reading list. - Mike Robinson
This question and the answers are a proof of how many code problems can be avoided by subscribing to decent coding standards. - CMR
[+430] [2008-09-22 11:37:31] GEOCHET

Huge methods/functions. This is always a sure sign of impending failure.

Huge methods should be refactored into smaller methods and functions, with more generic uses.

See also these related questions on SO:

[1] http://stackoverflow.com/questions/20981/how-many-lines-of-code-is-too-many
[2] http://stackoverflow.com/questions/129599/best-rule-for-maximum-function-size

I'd say greater genericness is not that strictly necessary. Just having code separated into smaller, more self-contained chunks with meaningful names is a huge win in terms of complexity reduction. Also, smaller methods tend to be inherently more generic regardless. - Wedge
Extract Method refactoring not only makes the code manageable, but also improves readability of the original algorithm, provided you give good names to extract methods. - Ilya Ryzhenkov
I work for a client that has a shopping cart, and they have rather complex item validation due to promotions, campaigns, and the like. It's all in one big ol' class called ItemValidations.cs. Oh, the horror! It causes us more grief than the rest of the web app combined. - MrBoJangles
(71) I do believe this can be taken too far though. I worked on one program that the methods were so small and generic that it seemed like a someone fired a shotgun at the classes and you were reading the little pieces that remained. It was tough to follow the logic. - bruceatk
(7) If your language supports it, it's a good idea to use nested or anonymous functions so that parts of the logic aren't exposed to the rest of the class or global scope. (C and PHP have this problem). - too much php
(1) @bruceatk: agree. It is a code smell, but not the most important by far. Probably it's the one we can all agree on. - peterchen
So glad this answer reaches pole position... I have suffered so much from very long (very very long) functions... and their proud and egoistical authors, monsters to work with... (this is a personnal statement of course). - Stephane Rolland
(2) Uncle Bob recently tweeted "Big methods are where classes go to hide." I think that's one of the finest statements about large methods that I've read in a long time. - realistschuckle
(3) On the flipside, I have a co-worker who makes a habit out of one-line methods that just call other methods. Tracing this code drives me insane as the call-stack gets huge. Even if you are conscious of this, it's still easy to overdo it. Often times I'll create another method with the anticipation that certain amount of logic will be implemented within to provide my desired output but it ends up only being one or two lines, and the method only gets called once. It's easy enough to refactor this, but that means you can't forget and/or run out of time. - alexD
1
[+411] [2008-09-22 12:32:41] Stefan Rusek

Commented out code. I realize that this one is a lot of people favorite thing to do, but it is always wrong.

We are in an age when it is easy to setup a source code versioning system. If code is worth keeping, then check it in, it is saved now and forever. If you want to replace it with a new version delete it:

  • The old version will be around if you need it.
  • Commented out code makes code hard to read since it still looks like code, and takes up the same space as real code.
  • After a few changes to the original code, the commented out version is way out of date

I once saw a function that had over a hundred lines of commented out code, when I removed the commented out code, it was only 2 lines long.

Sometimes we comment out code during debugging and development. This is OK, just make sure to delete it before it is checked in.


(83) Yeah, until you want to locate an old snippet of code you deleted six months and one hundred versions ago. Remember some unique feature of the text and search on that? Iffy. I had an experience like that recently, and I did eventually find the snippet; took lots of time. Easier to comment out. - Cyberherbalist
(6) For finding old code, a commit database is a great solution. dschneller.blogspot.com/2007/09/… - chuckrector
(2) Cyberherbalist, I think you obviously are not using any sort of source control. Or are you actually talking about COBOL? - Jon Limjap
Or use the Strategy pattern or something similar. - xmjx
(1) I agree with Cyberherbalist, it's a better solution for lazy people. @Jon Limjap: how would you search that code? - Cristian Ciupitu
I agree this clutters up the source but is this really a code smell? Per the supplied definition: "...indicates something may be wrong. It generally indicates that the code should be refactored or the overall design should be reexamined." - Luther Baker
(33) It depends. Code that no longer has any relevance should be deleted. Code that might be needed, or that indicates something that needs doing, can remain in comments as a reminder. Of course, this only works if you avidly prune your comments to make sure only the relevant ones stay. - Marcus Downing
I think it's not very wrong to have commented out code in your code as long as it's not in the comment. If you need the code again (after you remove it), you can always write it again. That way you don't get problems with the old code not working with the new code. - Peter Stuifzand
(2) I prefer to keep code commented out when working on legacy code with no documentation. If I need to change/improve something I keep the old code around until I'm sure that the replacement works correctly. This way I can quickly see if I missed something (rather than looking in source control). - rslite
(11) Sometimes that commented out code is the best way to comment the new code, so you don't repeat the madness. - bruceatk
(2) @bruceatk, I would disagree, it is better to say WHY you removed the old code, or at the very least comment why the code is commented in addition to the commented code. Leaving commented code in a project isn't bad in moderation, but it can easily get out of hand. - James McMahon
@nemo, I'm just commenting about the times that I have seen code changed and then rechanged back later on repeating a similar mistake that was fixed in the first place and would have been obvious if the comments had included some of the offending code and why it was being replaced. - bruceatk
(2) Delete code that has become obsolete. Comment it out if you are just experimenting. Development houses that don't experiment won't succeed. - Max Howell
(1) OMG - it may not be the worst code smell, but it sure as heck is my biggest pet peeve!! Just delete it already! - Jess Chadwick
Sometimes it is just really useful to comment the odd line somewhere when trying to find a bug. As long as you try and remember delete the commented lines when you're done, no probs. - DisgruntledGoat
(20) It's a code smell. In a lot of cases it shows a lack of confidence in the change and knowledge of the code. Someone who has read through all the code and completely understands the change and knows it is a good change will delete any unecessary code. - Quibblesome
Sometimes, I am using someone else's library and see that someone commented out a printf that would be useful in debugging (example: FFMPEG project). I agree it's a code smell but I don't get too hung up over it. It can give a clue into the coder's psyche :\ - thomasrutter
(1) Given an editor that allows syntax coloring and comment folding, I really don't how it is a bad thing. Sure, removing these at the end to tidy things up is good, but even if it stayed, it really wouldn't make much difference if folded. - Sylverdrag
(2) I've seen files that were 90% commented out code. It should have all been deleted. - Stefan Rusek
(1) One problem with source control rather than commented out code is that there is no awareness that any code was there so nobody would go looking for it, especially as the code evolves.. We really need a different third mechanism for this. - Uri
(2) Uri, I agree. Here's a thought on a new tool... It would be great if there were VCR like controls...rewind, pause, forward...so you can playback the changes. Basically, I never understood why undo histories have to die when rebooting. Save to file and persist on reboot...and let it integrate with svn. - joedevon
(1) I paste my about to be deleted code into a text file if I need it in the future even with version control. Keeps code clean, near by locally, and if somebody besides me needs it they can dig through the old versions ;) - srand
(4) @Uri: It's completely OK to replace delete code with a small comment saying: "Code to froombazzle removed in rev. 3080. If it's readded, don't forget to make foo() grangle." That's much more readable than keeping the code in comments. But even that only makes sense if there's a high probability you'll need the code. - sleske
(3) For the most part I hate commented out code. Usually in code reviews I'll open defects on blocks of such code. However, there's one case I run into from time to time that I treat as an exception to the rule. Sometimes the naive or obvious way to code something is wrong. If we're talking about a few lines, I'll comment them out, then add a remark saying something like "can't do it this way, the fuzzler doesn't support widgification" or "this works but doesn't scale". Helps the next guy avoid the same trap. - anelson
@joedevon: IIRC eclipes does that with 'Local History'. - Macke
(1) If you can't search and quickly browse changes to a file in your version control system, then your version control system is broken. - Brian Campbell
(1) I also, just like @srand, move old out commented code to a separate file. I call it "notes.txt", and when I search for old code I find it in that file. And I have no out commented code in my real source code files. -- In "notes.txt" I also explain why the code is not in use and how one might use it. And I sometimes add keywoards so the code snippets are easier to find later. - LeoMaheo
2
[+390] [2008-09-22 11:38:24] GEOCHET

Duplicate or copy/pasted code. When you start seeing things that could easily be reused that aren't, something is very wrong.

Refactoring to make reusable assemblies is the only cure.

See the Once and Only Once principle, and the Don't Repeat Yourself principle.

Simian [1], a code similarity detection tool works wonders for fixing this.

duplo [2] is a good open source project that provides a free alternative.

[1] http://www.redhillconsulting.com.au/products/simian
[2] http://duplo.giants.ch/

Simian, a code similarity detection tool works wonders for fixing this. redhillconsulting.com.au/products/simian - Paul Shannon
(2) There is an open source tool in Java that helps a lot in finding this pattern: pmd.sourceforge.net/cpd.html - Camilo Díaz
@Rich B:Very strongly second on SIMIAN. please add it to your answer. - pointernil
@pointernil: Done and done. Thanks for the confirmation. - GEOCHET
duplo is a good open source project that provides a free alternative. - torial
(11) I think this should be the #1 code smell. Long methods can be refactored easily; duplicated code is more of a pain, especially if the code isn't exactly the same but almost. Also, long methods can happen to anyone gradually, but copy-and-paste indicates that you're dealing with an extremely poor programmer; it's really fundamental to programming that whenever you have more than one instance of the same set of lines, you put it in a function. - Kyralessa
my favorite rule of thumb is that if you ever find yourself copy/pasting more than 2 lines of code, then you need to stop and think about finding some other way to do it. - GSto
(1) To balance: Making small pieces of code reusable usually triples its complexity. - peterchen
See CloneDR, semanticdesigns.com/Products/CloneDR for a tool that will detect near-miss duplicate code ("not exactly the same but almost") using language structure for many languages, including C, C++, Java, C#, Python, COBOL, JavaScript. CloneDR finds more "interesting" clones than Simian. See examples as the web site. - Ira Baxter
(1) @kyralessa: It seems this IS the number one smell: It's the only answer among the top rated (300+) that has ZERO negative ratings. Definition of consensus. - Nikolai Ruhe
3
[+275] [2008-09-22 11:40:07] TraumaPony

Methods with a ridiculous (e.g. 7+) amount of parameters. This usually means that there should be a new class introduced (which, when passed, is called an indirect parameter.)


Any guidelines on what entails 'ridiculous amounts of parameters'? I sometimes wrap large amounts of parameters up in a class, but I tend to do this because the language doesn't support named default parameters rather than due to the amount of parameters (a non-issue in most IDEs really) - workmad3
(4) This is doubly true if you find the same parameters being passed to multiple methods. Then if something needs to change you can fix it in one spot instead of having to touch every method signature and call. - Eric Burnett
(1) I personally find the worst is tons of params being passed by REF to something even though they often contain references to each other! Drives me nuts to see huge classes being passed through by ref to get to one member. - Jason Short
(1) Some examples would be good. - Cristian Ciupitu
(1) Don't pass any then, just use member variables. :) - bruceatk
@Cristian Ciupitu: if you had a function that took for example FirstName, LastName, Email, Telephone, DOB, Address as parameters, it's a sign you should have a Person or User class. - DisgruntledGoat
@workmad3: when you have to type in the same method for several times and end up getting fed up because of all the parameters even on an IDE only THEN it is a rediculous amount of parameters. good enough guideline? :) - Spoike
(68) If you wrap the parameters in a class, then don't you just end up with a new class that has a constructor with 7+ parameters? How is this an improvement? - recursive
Agree with recursive. I suppose you could have a setter for each member variable, and call those after construction. But that would be ridiculous. - demoncodemonkey
(1) Whenever I have multiple parameters, I ask any of them should be a new class. Even with one parameter, I ask if that parameter should be the 'this'. - Jay Bazuzi
(47) Alan Perlis said this quite beautifully: "If you have a procedure with 10 parameters, you probably missed some." - Gorpik
@Christian Ciupitu: Example? You mean like the entire Subversion code base? One random example: svn.collab.net/repos/svn/trunk/subversion/libsvn_fs/fs-loader.c see function svn_fs_merge. (It horrified me at first, but it's not as bad in practice as it looks at first blush because it's consistently formatted and intelligently commented.) - bendin
(2) 7+ ??? I'd put the margin by 3+ at most! After that it's not really useable anyways. - Matthias Hryniszak
I agree with this with the exception of public APIs which can't be changed. Sometimes then having many arguments for a particular function call is simpler than having to document and use tens of functions when a single public API can suffice. - Billy ONeal
(1) @bruceatk globals are better; @recursive very recursive point :) but if you're manipulating a similar set of related variables in several places, I think a new class is definitely a win - John Ferguson
(5) @recusive: and all the silly 35+ persons who upvoted that completely silly comment... Refactoring 10 parameters+ to a class is much cleaner and can be done in a very easy to read way. Ever heard of what a "fluent interface" is? It is a specific application of the builder pattern. See my answer with 22 upvotes here for enlightenment about fluent interfaces and to understand why your comment and the 35 people who voted up your comment are COMPLETELY WRONG stackoverflow.com/questions/2848938 This is getting more and more used in serious APIs (like the Google ones). - SyntaxT3rr0r
@Gorpik only if you're coding in c. ;) - Evan Plaice
@Webinator:I'm really sorry for you, the exact thing you say can be simplified in one term:"creating 10 procedures that return a class that should be destroyed later and calls a constructor that has 10 parameters we tried to hide with a class.".it is a nice term because you turn one line into a class that has the exact line you tried to hide.it is awful. - Behrooz
If your language supports keyword arguments / named parameters, this changes considerably - long parameter lists become easier to use than parameter classes. - orip
At a place where I used to work, someone made a function that accepted 12 Strings (and nothing else but Strings). - LeoMaheo
4
[+256] [2008-09-22 11:43:20] TraumaPony

Avoid abbreviations.

Variable names such as x, xx, xx2, foo etc (obviously if you are using Cartesian coordinates, 'x' is perfectly appropriate.) Rename.


(1) I wish i could vote 2x for this :) - anbanm
(20) Except for when it is appropriate - i,j,k are great for small loops. x,y,z are great for coordinates in small functions. But when in a larger setting, sometimes its better to say which x or y it is. - DGM
imho ijk is a bad habit, you should always specify what you are counting, you will forget at some point and ijk doesn't help you figure out what you are actually looping. - Jim Ford
Code Complete, again, is a great reference for variable naming! :) - Camilo Díaz
i,j,k and s,c,h are definitely bad habits. For looping, it takes little effort to use a meaningful name. Once you start using forms lik "blahIndex", "blahCount", or "currentSomeType" as habit it becomes just as easy. Also, I hate 1 character variable names when I'm trying to use search or replace. - Wedge
(1) I'd say single-character variables are fine in short loops, when the body is just one or two lines. But any longer, and it becomes hard to understand. (Eg. a loop for initialising an array or whatever.) - jfs
(3) @Wedge: you should search&replace only in that region (method/function), not in the whole file or project. - Cristian Ciupitu
(53) I go by a different idea. Global variable and function names should be very well thought out. Local names should be any old mess, i, m, u, v, tmp, whatever. If you're using more than about 5 local variables, you need more functions, not better names. - Ali
@Ali: interesting point of view - Cristian Ciupitu
(52) The name should be as long as the distance you are from it's declaration, so I agree with Ali. - bruceatk
(5) I agree with your point, however, it only applies to inexperienced coders who tend to be poor at variable naming. Using long variable names for small scopes is a code smell in itself. Having so many variables in a scope that single character variables reduce readability means you need to refactor. - Max Howell
This is easily the best tip on here. - Allyn
(9) I often use "for(int i = 0; i < SIZE; i++)". When short names are the convention, I think it is correct to use them. Every programmer knows what "i" means in the context of for statements. - luiscubal
(3) A agree wuth luiscubal - "i" and "j" for indexing are in my opinion perfect. Or like Linus used to call this: "Short and sweet and to the point" :D - Matthias Hryniszak
This is one of my biggest peeves in the universe. There is no excuse for bad variable names! - Michael Brown
nitpick: the example given is not an abbreviation. Sometimes in big frameworks, abbreviating things like namespaces is necessary. In .NET code, instead of an abbreviation, like in Hungarian notation, I will use the whole type name, e.g. buttonClick instead of btnClick. With code completion, the extra characters don't make it longer to type, and some controls are more easily understood - John Ferguson
(1) what about 'foo' 'bar' 'baz' for the academic/book world. would it kill to provide some real-world examples (especially examples that don't involve 'cats' 'dogs' or 'cars'). If programming is an artistic medium then the institutions that teach it are still stuck drawing stick figures. - Evan Plaice
(1) I think whether or not you're using Cartesian coordinates, 'x' and 'y' are the worst variable names ever. they're the reason algebra is so difficult to kids. The names 'horizontal' and 'vertical' (or 'horiz' and 'vert' ... or even 'h' and 'v'!) are soooo much better! - DaveDev
Did you ever saw Haskell source code? They are using single-letter names everywhere. - FUZxxl
5
[+216] [2008-09-22 11:58:02] bcash

Empty catch blocks. Especially if the exception being ignored is java.lang.Exception/System.Exception.

If a block of code could throw multiple exceptions, there should be multiple catches. Each handling the appropriate exception accordingly.

An empty catch might mean the developer doesn't have an understanding of logic in the try, and the empty catch was added to pass the compiler. At the very least they should contain some kind of logging logic.


(11) A catch block could legitimately suppress an exception, but then it needs a comment explaining why it is doing so. - Johan
(1) Yes! I got burned by this just a few weeks ago. One of the hardest debug sessions in my life. You see, the try-block was very very long... - Antti Rasinen
Seen these so many times it makes me cringe. Last example was 20+ try/catch all blank. Obviously the original coder had no clue what could go wrong and was just trying to prevent an exception in the event log or something... bad bad bad - Jason Short
Provided one defines a block containing a comment as non-empty, I'd say it's hard to argue against this being a smell. Eclipse has a setting to enforce this (including the comment proviso). - Sam Stokes
See my take on this at stackoverflow.com/questions/126158/… - Hugh Allen
(38) I disagree with you. If a code can throw many types of exception, you should do as many catches as you need to treat all exceptions, meaning that, if all you do is logging, or maybe showing a MessageBox, there should be only one catch. More than one falls into the duplicate code category. - Leahn Novash
(1) I agree with Johan, sometimes in web development, if an exception occurs in a non-crucial component, I would suppress it by having an empty catch block. i.e.: if the DOM can't find a control in another repeater to update the time, we can catch this and ignore it, it was only a nicety anyway! - Pat
A valid situation for this is writing a unit test that checks that an exception is thrown. If the exception is thrown and caught then nothing needs to be done as this is a passing test, otherwise a fail() method is called at the end of the try{} block. - DavidWinterbottom
(1) I had to do this once, in .NET I was dealing with a COM object I that for some reason would throw an exception but would return a value afterwards. In order to prevent the program from crashing in what was a more or less acceptable condition I had to use an empty catch block to skoosh the error. - tekiegreg
(3) Somebody should have told this to Java designers so that they wouldn't encourage (effectively) this behavior by throws clause. - Mehrdad Afshari
@DavidWinterbottom: Even in the case of a unit test, it is important for the test to pass with exception for the correct exception. If a test results in raising an ImportError when an IndexError was expected, the test has failed. - TokenMacGuy
There is one case in which a try clause might not call for a catch block, when the clause is being used for cleanup, as in try-finally. exception handling, in this uncommon case, is not handled by the given try-clause. Not every language supports this, and in a few cases, the language might have something that makes better sense, such as python with blocks - TokenMacGuy
(5) There's one extremly easy solution to this code smell - change the compiler! :D Java's compiler is the only one main stream compiler excesively hunting the programmer down to write useless code. - Matthias Hryniszak
(2) Empty catch/except blocks aren't code smells, they are code putrids. - Nick Hodges
6
[+204] [2008-09-22 11:54:27] Gamecat

Comments that focus on what the code does and not why. The code tells you what it does (okay, there are exceptions, but that is another smell). Try to use the comment to explain why.


(21) There is a school of thought that any comment is a bad thing as this just covers up the fact that your functionality is not clear enough, your variables are not named correctly and your tests are specified in enough detail - Paul Shannon
(6) Problem with those peoples is that they assume good APIs. If you have ever programmed AT-megas in C, you know why you need to comment what you do and why. (It mostly looks like DDRB |= (1 << foo)|(1 << bar); with horrendous side effects) - Tetha
(1) API's is a different story. In most cases you don't have the code to read. - Gamecat
(29) I disagree with the no comments school of thought. It will suggest everyone drop the comments, think they are actually writing understandable code ( which everyone does of course ??? ) and then we will all be completely confused when we go to read it. - Brian G
When there are lots of ways to do something, and you're Mister Nuanced Greybeard and know the slick trick, explaining how it works -- for instance a link to the wikipedia article on Duff's device, if you are constructing one -- is a polite thing to do for the kids. - davidnicol
(6) If you're programming in perl that is more or less your only choice :D - Anders Rune Jensen
(17) I disagree with this to some extent. If I'm using someone else's code, or fixing it, I don't always want to have to go through it line by line to figure out what it's doing. I prefer to have comments on methods describing what they do, and if the method is complex, a comment on each section describing what it's doing and why. - notJim
@notJim That makes skimming the code significantly easier. I make liberal use of comments and #pragmas in my code and I can almost always easily find the methods / blocks that I need. - kubi
@notJim @kubi: why did you think you cannot use the code to explain what it does clearly? as @Paul Shannon mentions above, code can be self explanatory if you variables are named well, and you designed or refactored your code well. if comments explain what instead of why, it is duplication. and duplication needs to be gotten rid of! - ragu.pattabi
7
[+188] [2008-09-22 17:58:20] Mark Stock

Pacman ifs

nested ifs [1]

if (cond1) {
    /* Something */
    if (cond2) {
        /* Something else */
        if (cond3) {
            /* Something else, again */
            if (cond4) {
                /* Something else, again */
                if (cond5) {
                    /* Something else, again */
                    if (cond6) {
                        /* Something else, again */
                        if (cond7) {
                            /* Something else, again */
                            if (cond8) {
                                /* Something else, again */
                                if (cond9) {
                                    /* Something else, again */
                                    if (cond10) {
                                        /* Something else, again */
                                        if (cond11) {
                                            /* Something else, again */
                                            if (cond12) {
                                                /* Something else, again */
                                                if (cond13) {
                                                    /* Something else, again */
                                                    if (cond14) {
                                                        /* Something else, again */
                                                        if (cond15) {
                                                            /* Something else, again */
                                                            if (cond16) {
                                                                /* Something else, again */
                                                                if (cond17) {
                                                                    /* Something else, again */
                                                                    if (cond18) {
                                                                        /* Something else, again */
                                                                        if (cond19) {
                                                                            /* Something else, again */
                                                                            if (cond20) {
                                                                                /* Something else, again */
                                                                                if {
                                                                                    /* And goes on... */

a severe stench emanates when a horizontal scroll bar appears

[1] http://stackoverflow.com/questions/114342/what-are-code-smells-what-is-the-best-way-to-correct-them#115975

I would say the horizontal scroll bars are appearing because of poor formatting with the indentation, not a code smell... That amount of if/else statements probably does indicate smelly code, but not because of how far across the screen it goes :) - Grundlefleck
(3) I think there are too many parentheses in here... seems like you have 2 closing parens for each opening paren. Should be: <pre> if {cond1) { } else if (cond2) { } else ... </pre> I think what you're aiming for is <pre> if (cond1) { if (cond2) { if (cond3) { ... } } } </pre> - David
This is also a Cyclomatic Complexity trap. - moffdub
(1) I call this "Mountains of Madness" (turn the code 90° CCW to see why) - Sklivvz
(14) This code makes my nostrils want to leave my face. - RodgerB
There are 15 more close braces than there are open braces in the code; I wrote a longer entry below (way below) on this, but that might easily be overlooked. - Jonathan Leffler
(126) "nested ifs" is the wrong title - they are not nested as written. The closing curly braces after the else ends it, and there should be no indentation of the elses. - Hamish Downer
(1) I salute the effort that went into making this post. Unless you just copied it out of your repository. :) - Nathan Feger
(1) it was worth the iffart (: - Mark Stock
Oh. Painful on the eyes, this post :-) - petr k.
(2) As a general rule, I try to have a max width of my code with 80 characters as maximum. - David The Man
(2) OK, I admit it...I like the "else if" control structure, and routinely have them two or three deep in my PHP code. What's the better way to implement the same logic? - flamingLogos
(1) > What's the better way to implement the same logic? First off, don't fix what ain't broke. I like "else if" too, but in moderation. Instead, maybe a switch statement, maybe an array of functions, maybe a table of rules, maybe event driven logic, maybe something else... refactor. - Mark Stock
You broke the golden rule if you have more than 2 "else if's" you should stop indenting. - bruceatk
(3) The cleanest way I have found to deal with this is to "return home early" as suggested at debuggable.com/posts/… - Rodney Amato
(32) I don't indent else if at all. If your indentation followed the same logic as the depth of braces, these are all at the same level. Personal choice I guess. - thomasrutter
(2) Where does the "Pacman ifs" name come from? Apart from the fact that your indentation is horrible, I use "Pacman ifs" all the time when switch statements can't be used. With proper indentation, there's nothing wrong in it. So, care to explain why they are bad? - luiscubal
Did someone format the error out of existence? Some comments mention indentation -- is this for massive if trees, or just lots of else if clauses? - Sean McMillan
Raven edited Sep 26 at 0:05: "applied more realistic formatting, but still don't understand what "Pacman" means" You killed Pacman! rollback - Mark Stock
(5) What is the point of such indenting? It does not reflect the structure of the code at all. - Glorphindale
I've seen code indented 42 levels (as far as I can remember). Someone showed it to me because it was so terribly terrible. It was a 10 000 lines long JavaScript file. From a startup company. It is currently dying, that startup, as far as I've understood. - LeoMaheo
Funniest (and yet saddest) thing I've seen all night. Hahaha. Sadly I had inherited a Delphi project back in 1999 like this. Glad those days are over. - Volomike
8
[+170] [2008-09-23 12:42:51] John Ferguson

Magic numbers

If code has lots of numbers all the way through it will be a pain to change them and you may miss something. Those numbers might be documented or commented, but comments and code can very easily get out of sync with each other. When you next read the code will you remember what the number means or why it was chosen?

Fix this by replacing the numbers with constants that have meaningful names. But don't make the names too long. It's up to you whether to import these constants from another file or limit them to the immediate scope.

Similarly for excessive amounts of string literals in the code, either use well-named constants or read them from a resource file. This can also aid internationalisation/translation efforts.


I want to comment this down because I first heard the term "Magic numbers" from an unpleasent fellow. Alas, this is very true, Magic numbers littering code is just a bad idea. - MacX.dmg
(16) There's always that one piece of code with: #define FORTY_TWO (42) //no magic numbers! - hexium
+1. This is IMHO one of the worst smells simply because it's among the easiest to avoid. - dsimcha
Yea, I hate these. Usually it's a magic string with magical values that should really be replaced by an Enum. - dotjoe
How I wish I hadn't accidentally community wiki'd this when I posted it! I know things become CW over time anyway, but I missed a serious amount of rep. - John Ferguson
(3) @hexuim I would also use your example as a code smell called 'using a preprocessor to extend the language.' That type of stuff makes me sick. There's a const type for a reason. - Evan Plaice
@Evan Plaice: hexium's not quite 'extending the language' with #define FORTY_TWO should always be allowed, even if you have a policy of no #defines :) An excessive use of macros should be avoided though. I haven't used C/C++ for a while so I don't know if the preprocessors will pick up macro errors at compilation time either. - John Ferguson
(1) @John Ferguson There's always the case where a pascal programmer who switched to C does something like '#define begin {' and '#define end; }'. Ironically, I'm actually working on a python preprocessor module (pypreprocessor) but it only supports non-value #define directives with #ifdef blocks. If it ever supports text replacement #define directives they'll be turned off by default. People are too tempted to do dirty and unnatural things with them. - Evan Plaice
9
[+159] [2008-09-22 13:16:10] aku

Not being able to understand what given piece of code does in less than 15 seconds

99 chances out of 100 that this code is wrong. Either it's too complicated or just badly engineered.

Cure:

Find the code author, make him to explain what the darn code does until he starts to cry "I wrote it yesterday, how can I remember what it does?! I would never write such code again! I promise!!!"

Alternate cure:

Refactor to make the code plain. Everything has a good name.


LOL! nice! I am sure we have all encountered code like this, sometimes even our own! :) - Rob Cooper
I couldn't agree more. Code needs to describe itself. - moffdub
There is still the 1 chance out of 100 that it is the language or framework which makes something which should be simple ridiculously complicated. I.e. making a control adapt to it's container's size in VB6... - Benjol
I've been pondering what someone else wrote on a different thread: Well-written code can tell us what it does but it cannot tell us what it was intended to do. Comments are not obsolete. - MrBoJangles
Sometimes the problem itself is complex. It happens all the time with overly algorithmic problems. - Mehrdad Afshari
Benjol, Mehrdad... It may be that the code is that complex. But you can still understand what the code does in 15 seconds or less if the coder commented and/or documented it well. - Kyralessa
(38) What is your code is actually doing something that's part of a really complex algorithm? - Wouter Lievens
(6) @Wouter: It should still only take 15 seconds to understand what it does. An algorithm that complicated should still, if refactored properly, have not only a simple purpose but also should be broken up into simple functions. - Brian
(5) @Wouter: The function name and the name of the algorithm should probably let you know what's being done in 15 seconds, if you're working on the same project. - Andrei Krotkov
(39) I'll admit it: It took me more than 15 seconds to understand the quicksort algorithm. Does that mean I shouldn't use it any more? - nikie
(16) -1, You can't understand a piece of code in isolation of what it's part of. I dare you to spend not 15 minutes but 15 hours starting at the source code for a project like git and being able to understand what it does. - hasen j
(1) If i cant understand 100 lines of code in 15 seconds, I don't see how I would understand 10 functions of 10 lines of code in 10 seconds, much less how I'm supposed to put them altogether to achieve the same result - Eric
(2) @hasen j, there is a difference between "how it does" and "what it does" while "how" part can take a while to understand, "what" part should be obvious from comments\names. As for GIT code base it is a perfect example of crazy-nobody-understand code. - aku
@nikie, is that because the algorithm itself was complex, or because the implementation you saw used a bunch of single-letter variable names? - Kyralessa
(5) I think that the 15 seconds is just tounge-in-cheek. The point is that you are supposed to be able to understand the code quickly for what it is and not spend an hour to figure out that some code re-implements linked lists. The time is not important, just the cleanliness. - Robert Massaioli
@nikie If it took you longer than 15 seconds to understand that a quicksort was being used, then maybe that quicksort should have been refactored into a separate function (like, maybe quicksort(input_list)). - quanticle
I've found this to be a major issue with C and C++ written by guys who have been using one of those two languages for a bit too long. For some reason squeezing everything into 80 columns is more important than making variable names more descriptive than "i", "j", "nd", "ibf", "obf", "os_tbl", "is_tbl" and of course with very little in terms of comments ("is_tbl" might have /* in table */ attached to it which of course doesn't really tell the next guy that it's a table used for formatting data received from another host). - mludd
(1) @Brian: Clearly you haven't worked on very complex algorithms and/or business logic. - Rob
@quanticle That's not what he was saying... - muntoo
10
[+159] [2008-09-22 11:52:11] aku

Huge code blocks in switch statements. Move code to separate functions. Consider using a function table (dictionary).


This is especially true with my current work on Symbian. A lot of the Symbian frameworks are designed around a single function callback that takes a possibly huge enumeration as a value. The function consists of a switch statement handling the entire range... can get 50+ loc with just functions :( - workmad3
workmad3 you just didn't see Motorola p2k code. Single function with 1 switch in 5000 (five thousand) lines of code - isn't cool? - aku
(15) Should an exception be made in the case of something like a state machine? To my mind, these are most effectively and clearly expressed as mile-long switch statements. - fluffels
In java we have made good experiences on converting primitive based collections into enum-based, and then subsequently moving the code blocks into methods implemented by the enum objects. I see this as a variant of the strategy pattern... - dhiller
I insist on only one statement per case. - Jay Bazuzi
@Jay: sometimes this just isn't possible and on the other hand not worth creating an own function/method for it - Atmocreations
11
[+154] [2008-09-22 13:48:55] MrZebra

Reusing Variables

Using the same variable to mean different things in different parts of the same thing, just to save a couple of bytes on the stack (or even just because you couldn't be bothered declaring a new variable). It's very confusing - don't do it!
Declare a new variable - the compiler is smart enough to place them in the same memory location for you if their uses don't overlap.

Example

var x = value;
if (x > y) doSomething();

x = someOtherValue;
if (x == 'Doh') doSomethingElse();

(4) This point is good. But it would benefit enormously from an example. - Max Howell
I don't think that this is that big of a deal iff the variable is appropriately named. Reusing (or using) a variable called 'a': bad. Reusing a variable called user: not that big of a deal. - Allyn
Here's the most common variable reuse, i. in the first for-loop in some function, i iterates over, say, Widgets, but in the second for loop, further down the function, it iterates over Widget Gadgets! CONFUSING! - TokenMacGuy
I would VERY careful that you don't accidentally reuse the wrong variable. This is good IF you can make sure the old variables go out of scope. There are definitely situations where variable reuse is a bad idea, but there are plenty where it has a tangible benefit. - Mike Pone
I re-use variables sometimes, not to save stack space, but to make a function less cluttered. (to save editor/brain space). - hasen j
(2) Doesn't save any stack space. Any decent compiler that does flow analysis should be able to see when a variable is no longer used and replace it with another. Hell, the idea of "local variable" is no longer really relevant at the compiler level. There are 6 GP registers and some memory to spill into. - Fraser
example given :) - Jason
(5) @Allyn, wrong. Reusing variables is always wrong. You're using the same variable for two different purposes. This makes the code harder to understand, and makes the code more difficult to refactor and otherwise edit. - Kyralessa
(2) If your method is so big that you find variable reuse helpful, then your method is too big. Refactor it (using e.g. Extract Method), and then you won't need to reuse variables. (Of course, if you're already reusing variables, you'll find it hard to refactor. That should tell you something about how bad it is to reuse variables.) - Kyralessa
minimize variable's scope as small as possible will prevent this :) - Vimvq1987
12
[+147] [2008-09-22 12:50:14] Coincoin

Premature optimization

If you read application level code littered with bit shifts instead of multiplication, and similar optimization tidbits, consider educating the author about the tradeoff between optimization and readability.


Not to mention accompanied code duplication (I've observed that they are often present along with the bit-hacks to save pushing another call). Code duplication is a nice invitation to bugs, and they are usually even harder to find in optimized code. - Statement
(1) Oh, yes indeed. At a project in school, some of the team members refused to use OOP in PHP because they think it's too slow. (And the project we are doing is never going live anyway!) - David The Man
(2) A friend of mine once said "Premature optimization is the root of all evil" I believe it's a true statement. - mxg
(12) @mxg, is Donald Knuth your friend? - Lev
(1) I do believe people take this too serious and then wait too long to worry about performance. There needs to be a happy medium and the effort needs to be put into finding the real performance bottlenecks and not perceived performance hits. - bruceatk
(2) I know some developers who spend weeks prematurely optimising. It's sad. They could have written so much more code. - Max Howell
(21) I'd say that an "algorithm optimisation" is almost always worth doing on the first or second pass of coding, whereas "micro-optimisations" should be left until you have finished with all functionality and the only thing left is performance. - Vatine
(8) There is also the "premature optimization is the root of all evil is the root of all evil" thing. If you leave optimization for later, it may be too late when you find out. - luiscubal
(5) Besides, doesn't a decent modern compiler know how to handle a lot of bit-shifty optimizations anyway? - Jeremy Powell
(1) @Jeremy exactly, a compiler will pretty much do away with any sort of bit operation optimization for arithmetic. Of course, I don't think a compiler will use the xor swap for ya :-p - Junier
(1) @Junier: Actually, temp variable swap is faster nowadays than xor swap. I'm pretty sure GCC can recognize xor swaps and replace them with temp variable swaps. - dsimcha
Parsed any network packets lately?::sigh::. There should be a coding standard about rounding to the nearest byte for non-flag data, especially types that get munged into static types like ints and floats. - Evan Plaice
There's a standard story in all sorts of computer book; the plot goes, "A team of veterans, including me, had a system that was running slowly. We thought about things, and we were sure it was one thing. But then we began testing things, and we found that it was in another area altogether, and would not have been improved by fixing what we thought was the problem." This story happens over and over and it means that premature optimization is usually solving the wrong problem by optimizing irrelevant code. - JonathanHayward
It is so ironic that right now it is just below answer about getting and re-getting the same property. - golergka
@golerka - I agree this could be called premature optimization, but at the same time some people might find the optimized version clearer especially in a language without property syntax. - Coincoin
Multiplication is actually faster on modern processors, IIRC. :P - muntoo
13
[+147] [2008-09-22 12:25:38] paul

Getting and re-getting the same property.

e.g.

if(getValue() != null && getValue().length() > 0 
    && !getValue().startWith("Hugo") ...)

Who knows what is going on inside getValue()? Could be something costly.

I would prefer:

String value = getValue();
if(value != null && value.length() > 0 
    && !value.startsWith("Hugo") ...)

Edit

I would agree with those commentators who say leave it up to the JIT to optimise the call. This will work in most cases. However, this became a bit of a problem for me in a framework where some getters did not simply pick up a property but in fact performed a hash table lookup or even disk or network action (can't remember which now). I guess the JIT can't do much there.

My point is that you don't always know what the getter does. Perhaps a naming convention such as getX for properties and fetchX for other actions might help. I just think getting/fetching a value once and then working with a copy of the value avoids the problem (unless you really know what is going on in the getter, of course)


Doesn't really change. When you compile in Release mode (.Net) the compiler will optimise that for you. - Patrick Desjardins
Also depends on the object you are interfacing with - it may already have proper caching built in... - DGM
(1) If you're using an interpreted language, there's no compiler to help you :) - Dan Harper
(7) I understand where you are coming from but there is something to be said about not introducing additional temporary variables... This is what Fowler argues in Refactoring. - Paul Osborne
(22) @Daok We can introduce another code smell then: relying on compiler optimization instead of writing proper code - Sklivvz
(4) Beware, i have seen (bad)code, where the "getter" function had side effects. So optimizing the code resulted in new bugs. - Gamecat
(1) This is a great and often overlooked point. Well put. I find it sad that the first 2 comments to this answer missed the point. Paul's point is sound, but for your example, is not the case IMO. Gamecat is right, but it seems fair to assume most getters are safe. You just have to apply some knowledge. - Max Howell
@Gamecat: I agree. You need to decide whether you want the result from the function (i.e. a variable) or to repeat the functionality. The above code wouldn't work as expected a string tokenizer or DB call. - DisgruntledGoat
(2) Surely an optimizer wouldn't refactor out a function call? As Gamecat said, it could (but shouldn't) have side effects. It can't be done - Gerry Coll
(1) You need a new method! if (Foo(getValue()). Now your code is clearer and there's only one query and no new local - Jay Bazuzi
(1) Know what is a method and what is a property. If it's a property do whatever you want, if it's a method run once and get a temp copy of it. - dr. evil
(5) This is also an example of Time of Check Time of Use (TOCTOU) lurking bug. - johnny
I dissagree on this one - at least not with getters. I concur with Fowler's opinion. Let the jitter do the job. - Matthias Hryniszak
(4) Your sample code isn't reading properties, it's calling methods. In languages that have first-class properties (e.g. C#), the convention is that a property getter shouldn't do anything particularly expensive, and should never have side effects. (If it violates those conventions, then that's a smell.) So reading a property repeatedly won't be a problem in the vast majority of cases. In the rest, profile, then fix. - Joe White
Hmm. C++ specific but correct usage of const and the fact that local class members are implicitly inlined allows the compiler to optimise this away completely. In fact using a temp would be quite smelly as it may suggest a bad getter doing more than just getting! - lttlrck
Comments about leaving it to the optimizer or compiler are wrong. The compiler may not be (and probably isn't) smart enough to notice that the return value is the same. And with threads added, this can be a problem. But add in a getter doing database lookup (my PHP code for example) and nothing in the world can optimize better than caching the value in the temporary. - iconiK
I'd use a temporary in this case. I don't mind a thoughtfully named temporary for that kind of stuff. It'll add one line to the length of your code, but if your function is short enough (as it should) and clear enough, it won't matter that much. You can declare the temp const if your language allows. Using a temporary also allows me and future readers to be sure there is not hidden useless performance hit. Allowing non-totally-trivial property getters may also help keeping the overall design clear. (OTOH overly slow property getters smell indeed bad.) - user192472
Is this a codesmell or Premature optimization? - k3b
I think you are performan a premature optimization by trying to avoid getX() calls. - Joe Zitzelberger
+1, I prefer this as it makes it much easier to read the code! - firefox1986
(1) About convetion for side effects : In C# there is such a convention -- object.X when there are no conceptual side effects, object.GetX() when there are side effects. - Coincoin
14
[+144] [2008-09-22 12:27:39] Manrico Corazzi

Overengineered design

This is common when introducing ten thousands frameworks and then handling everything indirectly, even for very simple chores.

So you have an ORM and a MVC framework, and several different layers in your application: DAO, BO, Entities, Renderers, Factories, at least a couple of contexts, interfaces crawl all over your packages, you have adapters even for two classes, proxies, delegate methods... to the point that there isn't even a single class which does not extend or implement something else.

In this case you'd better prune some dead branches: remove interfaces wherever they don't provide useful class interchangeability, remove man-in-the-middle classes, specialize too generic methods, throw in some generics/templates where you wrote your own classes that are only wrappers for collections and don't really add any value to your design.

NOTE: of course this does not apply to larger, ever changing applications where the layers and the intermediate objects are really useful to decouple stuff that otherwise would be handled by shotgun surgery.


(3) Interfaces should be preserved where they reduce coupling between sub-system boundaries. They make unit test testing feasible. If you can't find sub-system boundaries do not remove interfaces: Either the code is a big ball of mud (pattern) Or you aren't competent to perform this surgery. - Tim Williscroft
When you have an interface IObj and ObjImpl and you access directly to some methods in ObjImpls because "you don't wanna clutter the interface" then there's something inherently bad in the fact that you threw in an interface in the the first place, imho... - Manrico Corazzi
(1) I would vote this +5 if I could. I see this on a very regular basis in the regular course of my job. - Greg D
(1) Only if it damages readibility. - luiscubal
This is why I've changed my tune to "program to an IMPLEMENTATION, not an abstraction". Then when the time is right, use your IDE to distill an interface/abstract class ONLY IF YOU NEED TO. With modern refactoring tools there's no need to be an interface nazi from the get-go and in fact interfaces in C#/Java seem to be an abused feature of each respective language. - Repo Man
15
[+133] [2008-09-22 22:48:27] Marcel Tjandraatmadja

Negatives

They are a burden on the human mind.

Double negatives

I ran into a piece of code such as:

if( ! value != 1 )

Quite confusing to read! I suspect the original programmer was debugging and changing the logic made the program work; however was too lazy to properly change to:

if( value == 1 )

Else is a negative

When you see else you have to mentally negate the original condition. If the original condition already includes a negative, then you have to work extra hard. Negate the condition and swap the conditional clauses.

If the else clause is the "happy path", i.e. the non- error case, then your brain has to work to follow the flow of code. Use Guard Clauses instead.

Single negatives

Even single negatives require mental effort. So it's easier to read:

if (IsSummer())

than

if (!IsWinter())

Also


I'd go a step further here, and call out unnecessary negatives. For example: if (myVar != null) { // do stuff } else { // do other stuff } I would refactor this by making the condition use an == operator, and switch the code blocks. In other words, using "not" operators unnecessarily smells. - Lucas Wilson-Richter
(49) Worse still when the variable name has a negative connotation if (NotSystemUpdatable == false) ... - onedaywhen
I just had to work on a program that had " if (!isAddRelationship)" to add a relationship. - bruceatk
People actually right such things? I could believe it if it was an unintended fix for something, where they didn't read it write and just added the !. - Max Howell
(1) BEGIN { sub true() { !0 }; sub false() {!true}; } - davidnicol
(18) You have to be careful with those kind of changes though. !value != 1 is not !(value != 1), it's (!value) != 1. In this case they turn out to be the same, but you have to be careful with operator precedence when you "fix" code like this, it's real easy to change the meaning of the code. - Ferruccio
Ferruccio raises a very valid point - I've done something very similar in the name of "fixing bad code"... - Pat
in the same tradition of huge collections of strung together conditionals. - ojblass
(27) and also !IsWinter is not the same as IsSummer or at least if Winter is meant to be one half of the year and summer the other. !IsWinter could be IsSummer but also IsFall or IsSpring... - Eugenio Miró
(2) You ain't use no double negatives! - Yoo
Even worse: overloaded !, ==, or != operators that make !(val==1) and (val!=1) behave differently - mark4o
(7) I disagree with the single negatives comment. Your code snippet is quite trivial. If !IsWinter does not equate to IsSummer. It equates to IsSummer or IsAutumn or IsSpring. In such a case the !IsWinter is much tighter code that is easy to comprehend. - Jason Irwin
+1 for "else is a negative" - hasen j
(2) Actually I disagree, it is better to fail fast by putting the negative case in the "if" - sabbour
16
[+115] [2008-09-25 00:44:38] community_owned

Methods with unexplained boolean arguments

Methods with boolean boolean arguments tend to hurt readability. A common example I've seen is the following:

 myfile = CreateFile("foo.txt", true);

In this example, it's reasonable to assume that this snippet creates a file called "foo.txt", but we have no idea what the "true" means. However, if the snippet was instead:

 myfile = CreateTempFile("foo.txt");

You'd have a much better idea of what the coder intended. With that in mind, it's generally a good idea to break up methods with boolean arguments into two methods. So that

 File CreateFile(String name, boolean isTemp);

becomes

 File CreateFile(String name);
 File CreateTempFile(String name);

You can also get arround this by creating an enum:

myFile = CreateFile("foo.txt", FileType.Temp);

Oh, the joy of intellisense, ctrl + space and ctrl + K + P ;) - Sandor Davidhazi
(68) You can also get arround this by creating an enum. myFile = CreateFile("foo.txt", FileType.Temp); - Martin Brown
I much prefer Martins proposed solution to this issue. It's neater, easier, and straightforward and means you won't have a two big blocks of copy/paste code with a single parameter set differently. Out of curiosity, What happens when you want to make a ReadOnly file? CreateReadOnlyFile(filename)?? - Pat
(6) I like the enum better, too many functions can clutter things quite a bit. But it's a balancing act. - Anders Rune Jensen
+1 for the enum. Would it be correct to edit the answer and incorporating the enum? - Lena Schimmel
(28) +1 for enum +1 for languages with named parameters! - Ash Kim
I think it'd be correct to edit, yeah. The smell is the boolean arg, the fix has been altered by consensus. But that's me. - BryCoBat
Well, it is CW. :::Adds Martin's text::: - Brian
i guess it's fine, if it's well commented: myFile = CreateFile("foo.txt", true /*isTemp*/); - Arnis L.
I like the one with enumeration. Very nice solution indeed! - Matthias Hryniszak
In Python, this is avoided by named arguments: create_file("foo.txt", is_temp=True) - Michael
(2) If you have to call an existing API that uses this style, you should introduce a local variable for clarity. bool isTemp = true; CreateFile("foo.bar", isTemp); - Doug McClean
@Pat:why on earth should somebody create a readonly file? - Behrooz
@Behrooz: Why not? Why don't you go ask M$ why they would enable such a thing as read-only files? I can think of several instances where I've had to create read-only files, and several instances where I've had to limit the perms on files to specific security groups as well, which results in a read-only file for some processes, which by this reasoning, would mean that I should have functions called CreateReadOnlyFileForUsers() and CreateReadOnlyFileForEveryone()? There's a reason that permissions are presented the way they are in modern operating systems, so I could call CreateFile(file, perms) - Pat
@Pat:I didn't think about that situation.so ReadOnly files are OK. - Behrooz
(1) python, I love you! - Gabi Purcaru
enum's are nice. When you don't have control over the API to change it, C# 4.0 named parameters are very nice. i.e. CreateFile("foo.txt", isTemp: true); - Scott Wegner
(1) @Behrooz: I see your confusion. If you Create a file that is readonly, then what would you do with it? But when CreateFile doesn't necessarily create a file because it's actually only creating a file handle, then things become clearer. "Creating" a readonly file means: create a handle to an existing file that provides readonly access. (Incorrectly named methods is the root cause of many bugs!) - Craig Young
@Craig Young:thanks for explanation. - Behrooz
-1: boolean arguments are fine if they are keyword arguments (in languages with keyword arguments...), of if they are the only argument, or it's obvious what they mean. This answer is misleading; it would be quite fine if it said "unexplained boolean arguments"... actually let me change that... there, edited, no -1 - ninjagecko
Java's JFrame has a method setVisible which takes a single boolean parameter, and that is an example where it is perfectly fine and intuitively to use a boolean argument. Almost all other cases is a sign of a poor API where the implementer has prioritized making the function easy to write rather than easy to use. - hlovdal
17
[+92] [2008-09-22 11:44:43] Rasmus Faber

Static variables or the design patterns variant: Singletons

How to refactor:

  1. Find the largest scope in which the variable lives, and move it there as a non-static variable (if it is a singleton, convert it to a non-singleton and make an instance to pass around).

  2. You should now get lots of compile errors (missing reference to the static variable/singleton). For each one, decide whether it makes best sense to inject the reference as a instance member in the class (either in the constructor, if it is a mandatory dependency, or in a setter-method, if it is an optional dependency) or to pass the reference in the method-call. Make the change. This will probably propagate outwards until you reach the outer scope where the variable now lives.

A dependency injection framework, such as Guice, can make this easier. The DI framework can be configured to create only one instance of the class and pass it as a constructor parameter to all classes which use it. In Guice you would annotate the singleton class with @Singleton [1] and that would be it.

[1] http://code.google.com/p/google-guice/wiki/Scopes

EXACTLY the kind of answer I was gunning for, thank you Rasmus, +1! :) - Rob Cooper
(1) Wait. Are you saying static variables are a 'code smell'? - TraumaPony
Edited to clarify. Good point Trauma. - Rob Cooper
@TraumaPony: yes. Relying on global state is a code smell (imho). - Rasmus Faber
(1) @Rob Cooper: Smelly code is not necessarily bad code, but it is code that you should investigate closer to decide whether this particular use of the smelly code is okay. You need a really good reason to use global state. Ergo: any use of static variables is a code smell. - Rasmus Faber
Rasmus: That only really applies to OO languages. - TraumaPony
(10) Good one. Singleton is one of the hardest code smell to get rid of. Beginners always use them because it's the first pattern they learn and it's so attractive. - Coincoin
@TraumaPony: That is true, or at least it is much more work to avoid using static variables in non-OO languages, so it might not be as large a smell there. - Rasmus Faber
Definitely, Coin. It seems that most beginners to design patterns think 'global variables are easy, but are bad. Singletons are easy, and are a design pattern so must be good. Ergo, I'll replace all these bad globals with singletons). I should start a 'stop singleton abuse' campaign at some point :) - workmad3
+1 I thought we got rid of static variables when we moved beyond FORTRAN programming. The 1970's want their design patterns back. - sixlettervariables
(52) And what exactly is wrong with a Singleton? I'm not a very experienced coder, but I've coded my fair share of OOP, and I've used some very useful Singletons. Replacing them with variables that I would need to pass as parameters just sounds like masochism to me :) - Florian
(22) So you're saying I should pass an instance of my logger to every class in my app? - jpeacock
(7) jpeacock, No, there are obviously valid reasons to use a singleton. Problem is when you start using it for a LOT of things. - Jon Limjap
It's called "Singletonitis" :) I wouldn't investigate every Singleton though, it's a very common and useful pattern. - Sklivvz
jpeacock: see stackoverflow.com/questions/114342/… - Rasmus Faber
Thanks for the clarification :) Apparently I've been lucky enough to not smell this in the wild so was having trouble relating. - jpeacock
jpeacock, most loggers actually use a factory method - Chris Marasti-Georg
(2) Would singletons not make sense in situations where there is only one instance of a class per execution and it is used by almost every other class in the project? - fluffels
(2) static variables == bad. Singletons == good. Singletons can be managed to be thread-safe and namespace-safe, two of the biggest problems with statics - mxg
@mxg - what if static variables provide a correct and easy-to-use way to implement a singleton? e.g. in C# you can use a static with an initializer to implement a lazy init singleton without having to worry about double-checked locking. - Daniel Earwicker
(2) I've been through a loop with this. I used singletons naively, then decided they were evil, but now I've realized they often make perfect sense for anything that can be cached per process. - Daniel Earwicker
(18) -1 for lumping the Singleton pattern with static variables. I agree that static variables should be avoided, or at least hidden via getter/setter at some appropriate scope. But using a Singleton is a perfectly viable design pattern, and the Gang of Four would agree. That doesn't mean it cannot be abused, but that's not the same thing as a code smell. - Matt Davis
(1) -1, global variables are not TEH DEVIL ok? Sometimes they're useful. And, imho, singletons are just stupid a work-around to please people/compilers that belong in the "pure oop" campus. - hasen j
(2) Maybe you have a point, but your argumentation is weak. - peterchen
18
[+85] [2008-09-22 12:16:48] aku

Presence of types whose names start\end with "Utility", "Helper" or "Manager"

This is a very good sign of presence of types violating SRP (Single Responsibility Principle).

Analyze such types carefully, Often you can find that single class contains a bunch of unrelated methods - split it. Sometimes you can find that some method is used only once (often happens after refactoring), consider purge such methods.


LOL! I saw this and instantly thought "guilty" :D - Rob Cooper
[trying to delete AkuUberHelpers.cs file while colleagues are away] - aku
What! Are you telling me this "Everything.cs" file I carry around at every new place I work is bad? Seriously, I agree. Although, you should put some solutions for the usual case: utility functions used in hundreds of places. - Coincoin
I'd also suggest that 'manager' classes are a smell along these lines. A class that is purely managing another class is frequently a design issue that could be refactored out. - workmad3
(1) Or "Do" or "Run", and sometimes "manager". - Wedge
Yes!! I so agree with this smell. This typically derives from bad OOD, and things like DAOs and ActiveRecords (use classes as data holders, then provide Managers for those classes). When designing classes, concentrate on behaviours, not data. - Sklivvz
Hey, I don't actually know what SRP stands for, could you edit and include a link plz? - deemer
deemer, SRP stands for Single Responsibility Principle, google will tell you more - aku
I don't agree with "Do", it's useful for classes that implement e.g. Foo() so that it calls an abstract DoFoo(). I would add libraries with "Utils" in their names. - Lev
It's a tough one to avoid, but an important one. - Jay Bazuzi
This specific smell bugs the heck out of me in django. managers are actually not do-it-all type classes, and have a very narrow responsibility (constructing SQL queries from fragments), but you could never guess that function from the name of the class. - TokenMacGuy
(17) Manager class is not just a cause of bad code, but also a cause of bad workplaces. - Yoo
"Utility" and "Helper" classes are like the junk draw in my kitchen. Twice a year I have to toss out what ever got collected in it. I would be better off to just get rid of the draw itself. - Ryan Pedersen
19
[+82] [2008-09-22 11:46:48] tzot

Inconsistent naming of variables, methods, functions etc (mixing CamelCase with hpwsHungarian with everything_separated_with_underscores)


(43) welcome to PHP :P - grom
(2) _yes, I HATE_IT when That m_happens; - johnc
(2) +1: I agree, but I think it's ok to mix things depending on what your doing. For example, when I use C++: ThisIsAFunction(), this_is_a_class_member_variable_, this_is_a_local, kThisIsAConstant just like Google's C++ style guide: google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Naming - Tom
Linux Kernel devs? Hi, this is Stack Overflow calling. - Fraser
(1) I really = WantTo(WantTo::Kill, new Sob(Sob::Create, mGoogleStyleGuide)); // pretty consistent and readable - iconiK
20
[+82] [2008-09-22 13:45:24] Rasmus Faber

Just a general comment about code smells:

Both of my answers have received several comments like this: "But sometimes XX is the right thing to do" or "If you always replace YY with ZZ you are going to end up with an overengineering pile of ...".

I think these remarks mistake the meaning of a code smell: a code smell is not the same as an error - if they were, we would probably just make the compiler find them and return an error.

A code smell is nothing more than something that suggests that here is a possible refactoring. Smells may be more or less strong, and it is usually impossible to make hard and fast rules about them.

Sometimes a method with six arguments may be the best solution, I don't think I would like a method with seven arguments, but I would oppose a coding standard that forbid them. In some applications, a static variable might make perfect sense, but I wouldn't like that a large application hid its entire internal dependency structure in a big clump of static variables.

To summarize: code smells are simple heuristics that indicate that you might want to consider refactoring and suggest a possible appropriate refactoring.


21
[+76] [2008-09-22 11:45:06] Paul Shannon

Primitive Obsession

Always using "int" and "double" where you should have a class such as "Money" or "OrderValue" so that you can apply different logic or rounding. It also ensure method parameters are better structured.

string is the most common object of such obsession.

...

Judging by the comments, an example is called for. Here's one I saw a couple weeks ago.

We have an application that processes Word document content. All through our app, we had string parameters that took file paths to those documents. We found a bug in one place where, due to a poorly-named parameter, we were passing in a file name rather than a file path, causing the app to look for the file (and not find it) in the default application path.

The immediate problem was the poorly named parameter. But the real problem was passing strings around. The solution was to use a class like this:

public class FileLocation
{
    readonly string fullPath;

    public FileLocation(string fullPath)
    {
        this.fullPath = fullPath;
    }

    public string FullPath
    {
        get { return fullPath; }
    }
}

Your first reaction may be that this is a class that hardly does anything. You're right; but that's not the point. The point is that now instead of

public void ProcessDocument(string file)

we have

public void ProcessDocument(FileLocation file)

As long as we're constructing FileLocation objects correctly in the one place that generates them, we can pass them through the rest of the app with no worries, because now we can't possibly confuse them with file names, or directories, or any other type of string. This is the most fundamental reason to correct primitive obsession.


(25) using double for money is especially bad. - grom
Using Strings for everything is another example. - Eric Normand
(1) I agree but this can be misued if the whole framework relies on this because it introduces a lot of complexity where none is really needed. It's the whole OOP argument over again :) - Anders Rune Jensen
It also depends on your language. E.g. in C++ you can create a money-type that just behaves like you would want it to (passed by value, works with operators). In Java, this can get ugly pretty soon. - Lena Schimmel
(19) I think the alternative is much, much worse (i.e. no primitives, EVERYTHING is typedef-ed). This is a sure-fire way to render code completely unreadable. Long-time C++ devs seem to be the worst about this. - Matt Peterson
(9) Object Obsession is equally bad. i.e. insisting on an object for everything, even for instance, when an int would clearly suffice. - hasen j
(1) Before you do that, consider that databases only have a fixed amount of variable types. - Egor Pavlikhin
@Matt, not really. A typedef and/or class can make this more readable and consistent. - iconiK
@grom @Brian Say, if money is designed as a class with $s and cents, wouldn't that field be rounded every time, resulting a loss of precision? - CMR
22
[+71] [2008-10-21 23:06:54] hlovdal

Smell: Testing of "normal" code instead of exceptions.

Problem: The "normal operation" or most commonly/naturally executed code is put inside if bodies or attached as an else body to some error checking.

Solution: Unless it is impossible or there is a very good reason for not to, always test for exceptions and write your code so that the normal case is written without any extra indentation.

Examples:

void bad_handle_data(char *data, size_t length)
{
        if (check_CRC(data, length) == OK) {
                /*
                 * 300
                 * lines
                 * of
                 * data
                 * handling
                 */
        } else {
                printf("Error: CRC check failed\n");
        }
}


void good_handle_data(char *data, size_t length)
{
        if (check_CRC(data, length) != OK) {
                printf("Error: CRC check failed\n");
                return;
        }
        /*
         * 300
         * lines
         * of
         * data
         * handling
         */
}


void bad_search_and_print_something(struct something array[], size_t length, int criteria_1, int criteria_2, int criteria_3)
{
        int i;
        for (i=0; i<length; i++) {
                if (array[i].member_1 == criteria_1) {
                        if (array[i].member_2 == criteria_2) {
                                if (array[i].member_3 == criteria_3) {
                                        printf("Found macth for (%d,%d,%d) at index %d\n", criteria_1, criteria_2, criteria_3, i);
                                }
                        }
                }
        }
}


void good_search_and_print_something(struct something array[], size_t length, int criteria_1, int criteria_2, int criteria_3)
{
        int i;
        for (i=0; i<length; i++) {
                if (array[i].member_1 != criteria_1) {
                        continue;
                }
                if (array[i].member_2 != criteria_2) {
                        continue;
                }
                if (array[i].member_3 != criteria_3) {
                        continue;
                }
                printf("Found macth for (%d,%d,%d) at index %d\n", criteria_1, criteria_2, criteria_3, i);
        }
}


Rule of thumb: Never test the normal case, test exceptions.


(9) +1 for the examples; i'm sure sure about the general rule. - user51568
(2) i've been programming for about 30 years now, always questioning my style, and still sometimes adjusting it. This rule of testing for the exception came only recently to me after wondering for years how to best check for errors in a function's code. - Thomas Tempelmann
(1) it's pretty obvious when this code smell happens - the level of indenting usually is greater than 2 or 3 levels. Invariably at that point something is wrong, and can usually be fixed exactly as you describe. - Taylor Gautier
(8) This is not always a good candidate for a "smell". Testing for a good condition accomplishes two things: (1) keeps the most likely (good) code branch in the processor's pipeline, so the most common case executes faster and (2) avoids complex flow control (i.e. multiple returns, which are bad for performance, since they pollute prefetcher's cache). There are still tasks left where low-level performance is important, but not critical enough to go down to the assembly level :) - Rom
(1) @Rom, what? That is such a stupid argument. What will you save by doing that? 10 cycles (modern processors run at over 1 million clocks a second, even embedded ones - that's 100 MHz). What's the trade-off? A lot harder to read. I have a tad feeling it really isn't worth it. And GCC is also likely to optimize it, even more if you tell it what part is expected to happen, if you care than much. - iconiK
(2) @iconiK: I think you meant "billion" rather than "million". GCC (or any other compiler) does not optimize it away since it doesn't know which branch you care about more. And the missed cache hit costs significantly more than 10 cycles: around 100, depending on CPU vs. memory speed. In tight loops it's a lot. In video, 3d, audio, scientific or similarly heavy calculations, it's better to keep this stuff in mind. Also, don't call other people's arguments 'stupid': some people don't like that - Rom
(2) If you have another 300 lines of code in the same method then you have bigger code smells than what you just said. - Victor Hurdugaci
Absolutely, the 300 lines is a bad smell by itself, and only gets worse by having a useless extra indentation level. - hlovdal
-1 for a return statement 300 lines before the end of the method, with the label of the "good" example. - Joe Zitzelberger
Good one! I agree. This one seems to be related to the post on negatives, in that you should clearly separate your negative cases from the 90% use case. This could even be related to the post about using switch, in that large decision structures should be avoided if possible. - Neil Traft
23
[+70] [2008-12-07 09:07:00] Norman Ramsey

Treating Booleans as magical, unlike other values. When I see

if (p)
  return true;
else
  return false;

I know I'm seeing a rookie coder and prepare myself accordingly. I strongly prefer

return p;

(4) If prefer to see: return p != 0 // or != NULL, whatever fits better - Thomas Tempelmann
I meant 'p' to stand for a predicate, e.g., x <= 0, not a variable. But a variable would be OK too; in my shop we're into C99 and <stdbool.h>. For a variable declared type bool, checking != 0 would be incredibly distracting. - Norman Ramsey
You could event add things such as if (predicate()) return true; else return false; - bltxd
(6) The obvious exception is when you're writing a toBoolean() method. :) - pcorcoran
@pcorcoran If you have a value p that evaluates to true, you should still just use return p == true;. - Kristian Antonsen
@KristianAntonsen If boolean coercion is what you're looking for, I'd just use return !!p;. Or return Boolean(p); if maximum readability is your aim. It's the if statement that I find smelly, not the soft truthiness of the response. (That's a separate discussion entirely, and one which can be played both ways in dynamically typed languages.) - pcorcoran
24
[+68] [2008-09-23 06:23:06] seanb

Testing a bool for equality with true

bool someVar
....
if(someVar == true)
{
   doStuff();
}

The compiler will probably fix it, so it doesn't represent a huge problem in itself, but it indicates that whoever wrote it struggled with, or never learnt the basics, and your nose should be on extra high alert.


...I saw this for the first time two days ago. I was...amused. - Swati
(6) My favourite variation on this was along the lines of: if (((X==true) && (Y==true)) || ((X==false) && (Y==false)))... - Roddy
Someone at my workplace told me recently in a code review this was done so you could invert the definition of true and false somehow. I think this might have been true when TRUE was #defined to 1 and FALSE #defined to 0. Not sure how you would do it with true/false as keywords. - Doug T.
Anyone who would invert the definition of true and false should be horsewhipped, tarred and feathered, and blacklisted. - George V. Reilly
(31) One minor exception to the code above. Some languages, like JavaScript, have a notion of falsy. if (!x) will be true for x=0, x=null, x=undefined, etc. Checking if (x === false) to distinguish it from if (x === undefined) would be legitimate. - George V. Reilly
(10) The code smell here is the use of flag variables, not testing for equality with true (or false). What is there for the compiler to "fix" in this case? I'll never understand why some people get so worked up over this. - Christopher
Note: Ugly and scary things sometimes happen when you switch between the two forms in VB6, particularly when the boolean came from an external function. Oh, the joys of having not someBool and someBool be equal. - Brian
Nice catch with this code smell! :D I like the Roddy's example best :D It's such a beauty :D - Matthias Hryniszak
(4) @Christopher: The smell is the comparison with true. Flag variables are fine, but why use 'if(someVar==true)' when 'if(someVar)' would be enough. The variable name should be good enough that you can simply read it as nearly English. Similarly, for tests against false, you should just use !someVar. - krdluzni
I've actually done this when dealing with Nullable bools: "someVar == true" is much more readable than "someVar ?? false". - CaptainCasey
(3) Count me in the other camp. I find comparison with true to aid legibility (and omitting it "smells" of premature optimization:-) Maybe because I was raised on C, where we would #define true 1 #define false 0 and you know the trouble you can sometimes get into if you code if (x) and not if (x == true) - Mawg
(2) @mawg: actually, you have it the wrong way round. In C 0 is false and everything else is true. Testing a boolean variable against true would fail for any value of true other than 1. - JeremyP
(1) "someVar == true" can be a bug when switching between vb and c++ for example. in vb true is represented as -1, in c++ as +1. "someVar != false" is only a smell but not a bug because in most systems false=0 - k3b
@JeremyP: Except in C99 where a _Bool type is guaranteed to be either 0 or 1. The standard definition of bool in <stdbool.h> expands to _Bool, and true and false expand to 1 and 0. - dreamlax
@dreamlax: But a boolean expression does not have to be a _Bool, it can be any type that can be converted to an int. - JeremyP
@JeremyP: Any expression involving relational operators <, <=, ==, != etc always result in 0 or 1. Any assignment to a _Bool type always result in 0 or 1 too. But, you are right: if and while always check for non-zero expression rather than a boolean type (which is different in C++, which does). - dreamlax
25
[+66] [2008-09-22 11:47:46] NotJarvis

Any thread which depends on a sleep(n) call where n is not 0 (to release other threads).

For an example of why - see here: http://www.flounder.com/badprogram.htm#Sleep

Basically - the time you set for a sleep(n) call can always be wrong.

To avoid this sort of thing, coders should be using a more "waitFor a specific event" structure than "sleep for some arbitrary time waiting for other things" which nearly always can be wrong...


sleep(0) to allow other threads to run is perfectly OK and the time can never be wrong. - Uhall
yield() is clearer in this case... - Greg Rogers
I've used it in a Windows Service where I didn't want the currently running process to overload the CPU. I had a timer, but the code that ran when triggered could be a bit overwhelming. I call the sleep method if I think I'm overloading the CPU. Is that wrong? Is there a better way? - Micky McQuade
Uhall - that's why I specifically said Sleep(n) I should have made it clearer (Post will be amended) - NotJarvis
Micky - the point here is your app will not know if another app is overwhelming the CPU. If it's sleeping, waking up to check whether resource is free, then maybe waiting some more, thats okay. If your app is just "assuming" the CPU will be more free at sleep(n) it will fail under certain conditions - NotJarvis
(2) If n is sufficienly large (i.e. 5 minutes or something), and it doesn't need 100% accuracy, sleep() can be a better solution than setting an event on the system clock. - Fraser
(3) This can be called "Race condition smell". - Glorphindale
(2) Sleep(1) should be another exception - give up time slice and don't get the next one if the thread has highest priority. - peterchen
What about a game loop with low graphical requirements (i.e., one that can render a frame every time the game logic is stepped through)? The purpose there is to sleep some calculated duration of time based on how ahead-of-schedule the loop is in order to maintain consistent average speed (i.e., the speed may get a little off from frame to frame, but will be adjusted accordingly in the following frame). This is how I was taught to implement a game loop. - apollodude217
26
[+57] [2008-09-23 10:24:40] Ash Kim

More of a pet peeve: not conveying role when naming variables.

For example:

User user1;
User user2;

instead of:

User sender;
User recipient;

Also, expressing a role with respect to the wrong context. Class attributes should be named with respect to their class.

Method parameters should be named with respect to their role within the method NOT the role of the passed arguments with respect to the calling code.


(2) This is such a basic thing that is easy to overlook, but I agree with you completely. Good descriptive names for all variables will go along way to making your code self documenting. The caveat is that variables need to be terse, otherwise your algorithm can begin to look like a novella. - James McMahon
(1) I wouldn't shortsell it as a petpeev. Variable naming (all naming) is very important and I think your example is a great one. Someone tried to improve naming (it is better than "a" and "b"), but they are just being more wordy rather than conveying additional meaning. Good one! - Goosey
27
[+55] [2008-09-22 13:11:55] Sam Wessel

Inappropriate Intimacy

When classes access each other's fields or methods too much.

Some suggestions how to refactor/avoid:

[1] http://www.refactoring.com/catalog/replaceInheritanceWithDelegation.html

This smell is a bit to vague to be a good answer in this SO thread (although it' is an important smell). - Jay Bazuzi
Textbook answer. me no like. - hasen j
It was our company's Smell Of The Week when this question was asked, and I took the definition from our whiteboard, which was probably taken from a textbook. - Sam Wessel
28
[+39] [2008-09-22 11:59:20] moobaa

Comments that precede code such as the following:

// The following is a bit hacky, but seems to work...

...or...

// Quick fix for <xxx>

(35) Is a hack necessarily a code smell? I would certainly say an UNCOMMENTED hack maybe.. But a commented hack.. Not so sure? Sure it may not be great code, but is it a smell if its encapsulated and identified? - Rob Cooper
Yes, it is. If it's a 'quick fix', then it hasnt' been fully thought through. - TraumaPony
(18) I would say that such comments are deliberate code smell. They are meant to say "fix this when you have time". - slim
(11) hacks aren't code smells. A hack that takes 2 lines of code's equivalent might well be 50 code files to perform elegantly. - Quibblesome
Definitely a smell if there are a lot of them in a single component, as it would indicate that there is something wrong low down that is manifesting in a lot of quick hacks to get it working on a tight schedule. - workmad3
(1) Sometimes hacks are not just "quick fixes" sometimes hacks are getting the damn code to work where its not quite supposed to (especially when working with external components).. - Rob Cooper
Most commented hacks I have seen have been followed by the non-hacky code that wasn't working, that nobody had the time/knowledge to understand why it wasn't. I don't count those as code smell out-right, but I do search for them once a week, to see if the simple code is easy to fix yet. - Alex Lyman
Also: // This should never happen. It invariably does happen, and at the worst possible moment! - richq
I think this depends on who wrote the comment. A senior developer writing a //HACK comment probably has a very different idea of hack than a junior. - Sklivvz
(3) What should be done is //TODO: Rewrite - This is a dirty smelly hack - Chris Cudmore
And yet sometimes the quick solutions you come up with on the spot turn out to be the best solutions when you have time to re-examine the problem and think about it more. - James McMahon
Yes definitely a smell, though I'm as guilty of sin of writing stuff like //TODO: Find a better way to run this on several occasions - tekiegreg
There is a frightening LOT of these comments in the WPF reference source... - Christian Klauser
Hack's clearly are a code smell. But it's better to have a hacky solution then to have no solution at all. - Rene Saarsoo
What is the smell, exactly? - Jay Bazuzi
The sense that something may be amiss. - moobaa
I've learned the problem with this in my own code. After a few days, when I'm looking for something to do to the code, I happen upon such a comment and decide to revisit the problem. Only thing is, After reading the solution I came up with, the result seems perfectly reasonable and understandable, And I left myself no clues as to what I thought was wrong with it. The only thing I can do at this point is delete the //hackish comment. - TokenMacGuy
@TokenMacGuy -- exactly the solution I was going to propose. @nemo -- in that case, remove or update the comment. - RolandTumble
I've encountered comments like that in productions systems. And even ones that stated "I don't know how this code works - I just wrote it down" :D - Matthias Hryniszak
This is not a good one. Often it may just not be worth the effort to refactor an entire module, or worse application, or a LOT worse, OS' early decisions when a properly documented hack will do. An excellent example: really old Windows application, maybe 16-bit. Rewrite that monster will be a LOT more effort and risky than adding a hack. - iconiK
29
[+38] [2008-09-22 12:11:04] Rasmus Faber

Switch statements

Switch statements might be okay, but they are a smell that shows you should consider using inheritance or a State or Strategy pattern.

One example: (obvious, but I have seen something like this several time):

switch(GetType().ToString())
{
  case "NormalCustomer":
    // Something
    break;
  case "PreferredCustomer":
     // Something else
    break;
}

A bit less obvious:

switch(this.location.Type){
  case Local:
    // Something
    break;
  case Foreign:
    // Something else
    break;   
 }

How to refactor

  1. Move the switch to a separate method.

  2. Do you already have an appropriate inheritance hierarchy?

    2a. If you do, you might only need to move the individual case-statements to individual subclasses.

    2b. If not: introduce inheritance - either directly, or by using the State- or Strategy-pattern.

Edit: I am not arguing that you should replace all switches with polymorphism. Only that switches are a place where you should ask yourself whether polymorphism might make the code simpler. If replacing the switch would make the code more complex ("overengineered"), just don't replace it.


(13) This seems far too general to me - switch statements have many completely valid uses and this "smell" seems likely to encourage over-engineering. Perhaps it could be clarified with an example? - Simon Steele
(5) Using polymorphism or function arrays everyplace there could be a switch is a bad overenginnering habit. IF you have the same switch statement scattered everywhere, THAT should be a warning. - Coincoin
Switch statements are not equal in all languages. Compare VB, C++ and Java. What language used should be a factor. - WolfmanDragon
In an OO language, a switch statement should be regarded with suspicion - they are often [but not always] an indicator of a missing class or enum. In all languages, they may indicate a set of hard-coded rules that might should be soft-coded - Steven A. Lowe
(4) Using a pattern when code is enough is code smell. There is no problem in those switch statements. A strategy pattern would be 2x more code, 2x more bugs, etc. You're being OOP purist when you suggest the switch to be moved to another method. Besides, what would that method contain, if not a switch? - Leahn Novash
Leahn: In general you would get less code, since you can remove the switch logic. The first example could be refactored to two virtual methods on each of the subclasses, containing "Something" resp. "Something else". That is less code, not more. - Rasmus Faber
The Visitor pattern is also a good way to get rid of these switch statements. Especially those based on GetType().Name - Martin Brown
Applies only to lanuages with objects. - Norman Ramsey
I once found a switch statement that "smelled". I once was able to remove a switch statement hundreds of lines long that was checking a generic object's type and then casting to the type by having the objects implement a common Interface. - adam0101
The same code smell is found when one sees conditionals testing for class type ("instanceof" in java or "is" in delphi/c#). Code like that has been driving me crazy many times. - Matthias Hryniszak
Switches can also be solved in Factory Pattern (or Factory Method, if not that complex). - Alex Bagnolini
30
[+32] [2008-09-22 13:22:02] ilitirit

In languages that support OO, switching on type (of any kind) is a code smell that usually points to poor design. The solution is to derive from a common base with an abstract or virtual method (or a similar construct, depending on your language)

eg.

class Person
{
    public virtual void Action()
    {
        // Perform default action
    }
}

class Bob : Person
{
    public override void Action()
    {
        // Perform action for Bill
    }
}

class Jill : Person
{
    public override void Action()
    {
        // Perform action for Jill
    }
}

Then, instead of doing the switch statement, you just call childNode.Action()


(1) Double dispatch is a PitA though. And not the tasty sort with tzatziki sauce. - Shog9
Agreed. Acceptable as a refactoring step (Switch on flag column -> switch on type -> proper inheritance) - Sklivvz
In C++, making a function virtual can kill performance, so switching on a type and then making a non-virtual function call can be much faster. However, this is only an issue where speed is critical. - Graeme Perrow
This is similar to the Command pattern. I found that it works nice most of the time, but you won't be able to do this that easily if your methods have different signatures. It is still doable (generics), but hardly worth the trouble in most cases. - Leahn Novash
I think your comment is supposed to say Bob. Or your class is supposed to say Bill :) - Sean
@Graeme - Unless you can get the type at compile time, switching on type is almost certainly going to be slower than a virtual call. - Eclipse
Wouldn't this be solved with the Visitor pattern? @Shog9 - true. - Repo Man
31
[+29] [2008-09-22 13:28:19] Vilmantas Baranauskas

Not directly code, but it smells when VCS [1] log messages are empty or has such pattern:

"Changed MyClass.java.".

Fix: write useful comments telling why the change has been done, not that it was done.

E.g.: "Fixed bug #7658: NPE when invoking display of user."

[1] http://en.wikipedia.org/wiki/Revision%5Fcontrol

+1. I hate it, when I have to find and ask someone about the changes he/she did, only because he/she failed to explain them in the log message. - Paulius Maruška
(3) I agree that its annoying and poor practice - but its not a code smell. - kenj0418
I would argue it is a code smell, because it makes that code change smelly.. Often poor log messages indicate poor code changes (changing many things at once, not wanting to draw attention) - Goosey
One might consider commit hooks checking the comments for stuff like "In order to" at the beginning - this forces users to think at least a little bit before they click "commit". - Matthias Hryniszak
32
[+29] [2008-09-22 12:06:27] TraumaPony

Catching exceptions in the same method that threw them. This can indicate that exceptions are being used for control flow, which is a big no-no. Use a break or goto instead.


(4) This post could use some extra information about why this is a big no-no. I am not arguing it isn't, I just think that it isn't immediately obvious why this creates problems. I'd upvote it with the additional info. - JohnFx
Agreed... I'm guessing something along the lines of exception mechanisms are usually very inefficient once activated so shouldn't be used for 'standard' control flow (I've done this once... used an EOF exception to indicate I'd finished reading the file... not good code but I was young :P) - workmad3
(1) I think goto isn't a good answer, because it makes code harder to read - you see a label but have no way of knowing what points to it. - Tommy Herbert
(3) @ Tommy Herbert That's the same with a method. You have no way of knowing what points to it. - TraumaPony
(3) Exceptions caught within the same function aren't always bad. Exceptions in truly exceptional circumstances can keep the main code flow clearer without using stuff like goto. - Greg Rogers
Gotos stink even more than catching exceptions in the same function that threw them. - Martin Cote
gotos are a greater sin, try again. - Echostorm
(2) .....No, they're not. - TraumaPony
do programming languages still have gotos ? :S - Rismo
They, in fact, do. - GWLlosa
(2) @Martin, @Echostorm - In most languages, gotos are definitely more efficient than exceptions. - Andrei Krotkov
(2) If you need to break out of three nested loops AND you need to release resources before returning from the function AND you don't have destructors (think C), what else can you do but goto? Introducing a boolean and testing it in each loop adds no clarity. - George V. Reilly
Python uses exceptions for control flow (StopIteration for instance). - hasen j
yuck. ugly kludge WITH the big performance hit. really, how hard is it to include breaks? Exceptions are for error handling/recovery only. - Evan Plaice
33
[+26] [2008-09-22 14:56:24] Shane

Steve McConnell's book "Code Complete: A Practical Handbook of Software Construction" is essentially a 900 page answer to this question. It's an outstanding book.

http://www.amazon.com/Code-Complete-Practical-Handbook-Construction/dp/0735619670 [1]

[1] http://rads.stackoverflow.com/amzn/click/0735619670

34
[+26] [2008-09-23 00:33:27] Adam Pierce

Dumb comments or comments which are not updated when the code changes:

// Check all widgets (stops at 20 since we can never
// have more than 20 widgets).
    for(int i = 0; i < 55 && widget[i]; i++)
        processWidget(widget[i]);

(9) Surely also a case for using constants instead of magic numbers - John Ferguson
(3) "When the code and the comments disagree, both are probably wrong." - Norm Schryer - hlovdal
The best way to actually get rid of those comments is to follow the Simple Design principle stating that the moment you write an empty line and then a comment it sounds like you need a new method. - Matthias Hryniszak
35
[+24] [2008-09-22 12:05:15] Ticex

Large Classes

Large classes, that have more than one responsibility. Should then be separated.


@Ticex: Perhaps you should rephrase: "Large Classes that have more than one responsibility should be broken into separate classes with one responsibility each." You can get downmodded for bad grammar. Everyones Swedish will be awful here though! :) - Ande
Agreed. Didn't we all see classes like "Database" or "ApplicationManager"? - Matthias Hryniszak
36
[+24] [2008-09-22 12:31:57] AviD

Reinventing the wheel.
For instance, I love doing code reviews and finding some brand-spanking-new shiny version of 3DES. (Happens more often than you'd think! Even in JavaScript!)
"Whaaat? We MUST encrypt the CC/pwd/etc! And 3DES is SOOO easy to implement!" It's always a challenge to find the subtle flaws that make their encryption trivially breakable...

How to correct it - quite simply, use the platform provided wheels. Or, if there is REALLY an ACTUAL reason not to use that, find a trusted, reviewed module already built by somebody who knows what he/she was doing.
In the above example, almost every modern language provides built-in libraries for strong encryption, much better than you can do on your own. Or you could use OpenSSL.
Same goes for other wheels, don't make it up on your own. It's stinky.


There is one reason, if you are paranoid enough (not saying it is a valid reason). If you use a public encryption library and someone finds out how to break it from another place using it, he just got access to your data as a side-effect. If you're paranoid... - Leahn Novash
That's actually an excellent example of when NOT to do it - rest assured you WILL NOT build a better encryption library. Kerckhoff's law - simplified can be noted as "obscurity is not security". If someone can break the public libraries - he can break yours. Heck, I can break yours too. - AviD
I completely agree with the example, but in general there are points where risk of using a third party module is high (mostly due to project deadlines and the module being new and untried) that it makes sense to reinvent the wheel to meet the deadline. - Mehrdad Afshari
I would say to that - nothing more new and untried than code you just wrote and hasnt passed QA yet... not to mention riskier, and if you're pressed to meet deadlines you might accept more long-term risk. However, I DO agree that SOMETIMES it makes sense - IF this is your core functionality. - AviD
To add to my previous comment, Jeff Atwood had a good post on this (regarding this very site... ;-) ) - codinghorror.com/blog/archives/001172.html. his point is: "If it's a core business function -- do it yourself, no matter what." (Although I would argue that his example is off a bit) - AviD
(3) Crypto is something of a special case, because it's so hard to get perfect, and the effects of screwing it up are huge. Imperfect crypto is worse than no crypto at all, because you may think you're safe when you're not... - BryCoBat
(Also adding to a previous comment...) For anything but crypto, Jeff's point applies. Do it yourself if it's core to what you're doing, otherwise find a 3rd party lib. And still run it through your QA, even if it's 3rd party. - BryCoBat
But ONLY if its core to what you're doing, AND assuming this is actually your core competency (and not just you happen to like doing it - I can almost GUARANTEE that I can break any HTML sanitization scheme you build yourself). - AviD
Wheels are stinky? Maybe if they're wheels of cheese. - George V. Reilly
And the Joel Spolsky post that Jeff references is here: joelonsoftware.com/articles/fog0000000007.html - RolandTumble
@BryCoBat: The point applies to crypto as well. Do crypto yourself if it's your core functionality, otherwise use functionality from somewhere else. However, the definition of "core functionality" is much stricter when applied to crypto, so only a small number of companies meat that criteria. And many of them are, in fact, using their own crypto functions. Somebody had to write them, after all. - Brian
Heh, actually there are many vendors of cryptography products, for which crypto is NOT their core competency... hence, you find so called security products which are quite insecure, and crypto modules that do it poorly.... - AviD
hmmmm. kinda makes me feel like writing a HTML parser that uses RegEx to parse the XML. oh snap ;) - Evan Plaice
37
[+22] [2008-09-22 12:02:35] aku

Presence of GOTO statement.

Usually it means that either algorithm too complicated or function control flow is screwed.

No general practice unfortunately. Each case should be analyzed individually.


Usually solved with a "break" or "continue" - Sklivvz
(1) goto still has uses in error handling in C - deemer
In C goto can be used to implement a finally block - closing file handles etc before returning. A returnval variable is also often used. But apart from that I don't know a good use for goto. - Hamish Downer
(5) I hate when people say this. Goto is useful in some cases, where it clearly is the best solution for the problem. It just doesn't happen very often, and in the other cases it's almost always the worst solution to the problem. - Anders Rune Jensen
(4) Microsoft themselves use GOTOs in their suggested practices, notably in retrieval of HTTP documents. GOTOs can be useful at some points, and I totally agree with Anders. I hate when people just think it's always bad. - Andrei Krotkov
There are stackoverflow questions specifically directed towards the usage of GOTO. - luiscubal
10 PRINT "Refactor BASIC to remove GOTO";20 PRINT "Pulling out hair";30 GOTO 10 - Greg
I used to be one of those "no goto obsessed" people. I have since changed. In SQL the only way to perform a block of code is by using LABELS and GOTO. - Cape Cod Gunny
@Anders Rune Jensen, this topic is about code smells and not about "things you should never use". GOTO can be handy in simple languages like C or SQL but too often it's being misused - aku
38
[+21] [2008-09-22 18:00:06] Apocalisp

Excessive Inheritance

Many newcomers to object-oriented programming immediately pick up on the idea of inheritance. It's intuitive because it is superficially isomorphic to the way we organize concepts into hierarchies. Unfortunately, it's often the case that this is the only abstraction that they end up using, so you end up with class hierarchies like:

Buzz
  BuzzImpl
    FizzBuzz
      BatchFizzBuzz
        BatchFizzBuzzWithFoo
        BatchFizzBuzzWithBar
        BatchFizzBuzzWithNeitherFooNorBar
      FizzBuzzThatSendsEmail
    BuckFizzBuzz
      BuckFizzBuzzWithFoo
...etc.

To fix, use composition rather than inheritance. Instead of having FizzBuzz inherit from Buzz, have it take a Buzz in its constructor.


oooh, does all that derive from an abstract base class and does it include multiple interfaces on each level too? ::sigh:: - Evan Plaice
Oh yeah, I forgot BuzzBase - Apocalisp
39
[+21] [2008-09-22 13:34:06] torial

Can't believe this hasn't been provided: high cyclomatic complexity [1] for a method/function. Anything over 20 (with some exceptions like dispatcher methods) should have functions extracted out.

Source Monitor [2] is an excellent tool for gathering source code metrics like this.

[1] http://en.wikipedia.org/wiki/Cyclomatic%5Fcomplexity
[2] http://www.campwoodsw.com/sourcemonitor.html

(4) The what? :) Sorry, but could you clarify a little? - Statement
I added some links to a good description of cyclomatic complexity. - torial
(2) Smell should be something, that you can instantly see when you look at the code. But you don't look at the code and say: "Hey, this thing has cyclomatic complexity 25!" - Rene Saarsoo
I disagree -- you get a sense of the cyclomatic complexity just by looking at code. At least I do. It might just come from doing it a lot. - torial
This could be rewritten as "high amounts of branching in a function", but I am in favor of raising awareness of the term 'Cyclomatic Complexity' so that when I use it I am less likely to get confused looks. - Goosey
You could just say "branching" and avoid the looks. - James M.
20 you say? Well, for all I know I can't figure out in a reasonable amount of time anything that's greater than 5... - Matthias Hryniszak
That may be opportunity, I've had a lot of exposure to some high CC code! If you don't want to rely on your gut, use tools like Source Monitor -- which I use for finding possible areas of concern. - torial
@Rene Saarsoo: Your IDE may be able to tell you the control flow is too complex. Then it's a smell. - Matthijs Bierman
40
[+18] [2009-03-23 20:09:56] Comptrol

This is from " Practical Common Lisp [1]"

A friend of mine was once interviewing an engineer for a programming job and asked him a typical interview question: how do you know when a function or method is too big? Well, said the candidate, I don't like any method to be bigger than my head. You mean you can't keep all the details in your head? No, I mean I put my head up against my monitor, and the code shouldn't be bigger than my head.

[1] http://www.gigamonkeys.com/book/practical-a-simple-database.html

@Comptrol: That's an excellent way of putting it... :) - t0mm13b
+1 Akin to rule of thumb this is rule of head! - CMR
41
[+18] [2008-09-28 19:36:51] Vilmantas Baranauskas

Unused variables or fields.

Remove them. There are helper tools (different IDE [1]s, checkstyle [2], etc.) which may inform you if you have some.

[1] http://www.jetbrains.com/idea/
[2] http://checkstyle.sourceforge.net/

42
[+17] [2008-09-23 15:45:31] Gord

Using magic numbers for return values.

int orderID = DAL.GetOrderIDForUser(1);

if(orderID == -1) 
    ShowNoOrdersPage();
else
    ShowOrder(orderID);

It is just a matter of time before you end up with a -2 or -3.


(1) If the magic, out-of-band numbers are encapsulated in an enum, fine. As literals, they smell. - George V. Reilly
@George, are you suggesting that the method be rewritten as Enum result = DAL.GetOrderIDForUser(1)? - CMR
No, I'm suggesting that you have something like enum {NoOrders = -1} - George V. Reilly
43
[+17] [2008-09-23 21:22:40] 18Rabbit

Putting in a "temporary" fix.

Temporary fixes have a funny way of becoming permanent because you never seem to have the time/inclination/memory to go back and fix them.

If you're going to fix something, fix it the right way the first time.

If it seems like it will be a huge undertaking to change it then maybe you need to re-evaluate why it needs to be changed. If it's unavoidable that it must be changed then put in the best fix that you can in the time allotted and assume that it will be permanent (because it will be).


44
[+17] [2008-09-22 14:57:43] Nathan Long

Variable Scope too large.

Global variables make it hard to keep track of which function modified something and when.

Refactor so that variables exist only within a function. Functions should pass information to each other via arguments and return values.


In C/C++ it is overzealous to say that globals should never be used, but I think that every one should be very well defined. I also like to give globals a name in the form g_variableName so that it is obvious to the variable user that the object has global scope. - Paul Osborne
Where the language supports it, also consider keeping variable scope to single code blocks. Among other benefits you can then extract that block out as a method more easily than if all the variables are declared at the start of the method. - John Ferguson
Actually, global variables often times can be much easier to trace given the fact that you can easily do a global search and fine all of the places it is used. This is virtually impossible if you are passing the data from function to function instead. Not to mention, in event driven applications global/class-level variables are a must in order to maintain state. - alexD
45
[+16] [2008-09-22 13:19:23] Vilmantas Baranauskas

Wrong spelling of class and method names. Look up in a dictionary if not sure or use plugin in your IDE to check spelling for you.


Does anyone know of a spell check plugin for VisualStudio that is any good? - Martin Brown
46
[+15] [2008-09-22 16:39:16] Camilo Díaz

With database access

String concatenation, specially used for giant prepared SQL-statements, when there are lines like:

String select = "select ..."
// Lots of code here
select += "and c.customer_id = ? "
select += "and da.order_id in (?, ?, ?)"

And worse, then tracking the position of the index:

preparedSt.setInt(++cnt, 15);
preparedSt.setString(++cnt, 15);

With objects properties

Repeatedly accessing beans or value objects properties like this:

customer.getOrders().get(0).getPaymentDetail().getAmount();

In very simple boolean logic

Seeing nested 'ifs' like this when single-level if's or a switch statement will suffice:

if (cond1) {
    /* Something */
} else {
    if (cond2) {
        /* Something else */
    } else {
        if (cond3) {
            /* Something else, again */
        } else {
            /* And goes on... */
        }
    }
}

Using two boolean variables to refer to a simple, unique condition, that can be deduced just by one of them:

boolean finished, nextIsPending;
if (task.isRunning()) {
    finished = false;
    nextIsPending = true;
}

Why is string concatenation bad here? It really depends alot on the language, but in Java, for instance, a StringBuilder is actually going to have a larger performance hit then concatenation in smaller cases. - James McMahon
If you just declare one long string with concatenation JVM will optimize to one big string buiilder anyways. Its when you have alot of looping and recursion when that would be worse. - Egg
@James @Egg, I assume the answer means that it can be a static/final String, rather than constructing every time. - CMR
47
[+15] [2008-09-22 12:41:38] MusiGenesis

Gratuitous (unnecessary) use of multithreading.

I've taken over many multithreaded applications, and I never needed a code smell to know there was a problem. Multithreaded applications are usually incredibly unreliable, and fail frequently in impossible-to-reproduce ways. I can't count the number of times I've seen an application start the execution of a long-running task on a separate thread, and then go into polling mode on the main thread (usually using Thread.Sleep(n)) and wait for the long-running task to complete.


(1) Just a counter-example: Putting a long task into it's own thread can be useful if it allows the main thread to process windowing messages, and report status/progress more correctly. It can still be tricky to hand off the messages between threads correctly, though. - deemer
48
[+15] [2009-01-18 15:17:16] orj

Classes or structs with lots of member variables.

A class or struct with more than about a dozen member variables probably hasn't been correctly factored into sub-components/classes.

eg:

class Person
{
    string Name;
    string AddressLine1;
    string AddressLine2;
    string AddressLine3;
    string Addressline4;
    string City;
    string ZipCode;
    string State;
    string Country;
    string SpouseName;
    string ChildName1;
    string ChildName2;
    string ChildName3;
    int Age;
    // and on and on and on
}

Should be:

class Address
{
    string[] AddressLines;
    string ZipCode;
    string State;
    string Country;
}

class Person 
{
    string Name;
    Address Address;
    Person Spouse;
    Person[] Children;
    int Age;
}

And this is just one contrived example.


+1. learnt this the hard way :) - obelix
49
[+14] [2008-09-23 03:10:17] keparo

Code Smell (noun)

This is a general criticism of poorly written or poorly designed software.

Example usage: "I was disappointed when I saw the source code. Everything basically works, but that code smells".

There are a lot of reasons why software might qualify as "smelly". People have listed quite a few specifics here.. Things like having overly complicated data structures, global variables and goto statements. But while these are all symptoms of smelly code, the truth is that there isn't a hard and fast rule. In programming, any specific problem could be solved a handful of ways, but not every answer is as good as the next.

Some basic principles

We value code that is easy to read. Most programmers will probably spend the majority of their time reading and editing existing code, even if it is code that they wrote themselves.

Along the same lines, reusable code is also considered valuable. This doesn't mean that code is copied and pasted.. It means that code has been organized into a logical system, allowing specific tasks to be performed by the same piece of code (with maybe just a few differences each time, like a value in each calculation).

We value simplicity. We should be able to make single changes to a program by editing code in one place, or by editing a specific module of code.

We value brevity.

Smelly code is hard to read, hard to reuse, hard to maintain, and is fragile. Small changes may cause things to break, and there is little value in the code beyond its one time use.

Code that simply "works" isn't very difficult to write. Many of us were writing simple programs as teenagers. On the other hand, a good software developer will create code that is readable, maintainable, reusable, robust, and potentially long-lived.


50
[+14] [2008-09-22 11:44:28] tzot

for index 0 to len-1 style looping over a list in languages where iterators exist.


This is only partially a code smell. Sometimes you need an index because you are going through more than one list. - torial
zip :: Iterator<A> x Iterator<B> x ... x Iterator<Z> -> Iterator<A x B x ... x Z> (Tuple of iterators to iterator on tuples) is your friend, in this case. Or something like "cartesian_product". Or whatever you exactly need. - Tetha
(4) This isn't a code smell. This depends heavily on language, performance requirements and culture. - Quibblesome
(3) As the question says, a code smell means you have to look closer. Generally it is better to use iterators for readability reasons. This does not mean you should never manually iterate, just means you should be cautious about doing so. - Guvante
Your objections are understandable; may I ask whether you've never worked with novice programmers who carry their old habits from other languages to new ones? - tzot
(6) Of course, you would probably prefer for (mytype<myparam>::const_iterator it = object.begin(); it != object.end() ++it), right? :D - user51568
@stefan.ciobaca: Actually, I'm more of a for item in collection: person ;) - tzot
(5) Languages have idioms. Those idioms should be the preferred style. - George V. Reilly
(1) In C#, using the for loop is significantly faster than the foreach statement. Also, deleting an item using the foreach loop is not allowed, making the use of the for loop mandatory. - Marcel
51
[+13] [2008-09-22 12:10:45] aku

C# specific: use of ref keyword.

Often makes program behavior unclear and complicated, can cause unpredicted side effects.

Consider returning new value rather than modify existing one i.e. instead of:

void PopulateList(ref List<Foo> foos);

use

List<Foo> GetListValues();

(1) This is usually a bit harder than your example. Mostly ref keywords are used to return more than one value. The out keyword is usually a bit clearer (only returns a value and doesn't pass it in) but it still smells. But this is a language problem as well as a code smell. I'd like python-ish tuples. - Mendelt
yeah, tuples rulz! - aku
Mendelt: Agree, the example is too simple. The trap with that smell is it usually looks justified. If you need a ref, and it's not for a justified optimization (append to a list), it's probably wrong. - Coincoin
Good one, I have often seen ref used by developers moving to C# from other languages (C++ specifically) where they don't understand that you're already passing objects by reference. - Simon Steele
The reason why returning is better is not because the other code is complicated, it is because you can chain stuff imo. - Anders Rune Jensen
I wonder why ref was included in the language. - user51568
(1) Ref is important for passing value types. You don't need ref to append to a List<>, but you need it to efficiently pass a struct{} - Dave Moore
1) Object are passed for default by reference 2) Not always method are used for initialize lists, many times the same list have to be updated (adding or removing values), so passing by ref is a must, imagine what means duplicate a very big list in memory just because you can't understand what updateInitialList(list, newData); do. - kentaromiura
Your example is a great example of a bad use for ref. However, ref in and of itself is not a bad thing, and I think you should make your smell a bit more specific. For example, ref can be tremendously useful when you need to be able to update a value type such that those updates persist beyond the scope of your function (and yes, there are good reasons to use value types and structs rather than classes.) Ref is also very useful when you need to be able to set an original object reference to null via a function...somewhat rare, but sometimes also very useful. - jrista
52
[+13] [2008-09-22 12:28:14] aku

Returning more data that needed

Example:

List<Foo> Foos; // returns List<T> to provide access to List.Count property

Often this leads to misuse of data structures and unwanted data modifications.

Consider providing as much data as needed.

IEnumerable<Foo> Foos;  // Returns iterable collections of Foos.
int FooCount; // Returns number of Foo objects.

(1) Hear hear! This is a source of very subtle bugs and a bad code smell IMHO. - Mike
(2) You shouldn't return anything writable if you don't accept alien writes. Wrap into read-only collection as appropriate. - Ilya Ryzhenkov
@Ilya +1 for diction. Also, @aku, how do I return two values? - CMR
53
[+13] [2008-09-22 12:29:09] Coincoin

Excessive use of code outlining

It's the new big. People use code outlining to hide their behemoth classes or functions. If you need any outlining at all to read your code, it should be a warning sign.

Consider the following:

  • Extract all types into their own file
  • Refactor the main class until it's small enough
  • You can use the partial keyword (C#), or any equivalent mechanism, in cases where you have to implement a lot of interface methods, or expose a lot of events

While I don't use it excessively, I think of code outlining as a convenience, not really a necessity. I much prefer to look at only the method, property, or constructor I am working on, without having other clutter in the way. I use a very strict pattern for code outlining that gives me nice control over what is visible on my screen. I usually have my fields visible, along with the method I am working on, and everything else is collapsed or hidden within a collapsed region. Gives me great focus. Very useful, and the #region stuff is hardly intrusive compared to doc comments and the like. - jrista
54
[+13] [2009-04-22 17:14:05] John

Magic Strings

I've seen Magic numbers mentioned here more than once and I wholeheartedly agree. One often overlooked smell I haven't seen mentioned is Magic Strings.

Example:


public string GetState()
{
  return "TN";
}

Solution:


public string GetState()
{
  return States.TN;
}

Always be weary of code that is working with hard coded string data just as you would with hard coded numeric data. Consider making the string a constant variable with a comment explaining what it represents as a minimum. There is a really good chance that this should actually be in an Enumeration.

Side note: It's not technically a smell but it greatly annoys me to see something like this as well:


if(0 == foo)
{
  DoSomething();
}

(5) if(0 == foo) This is a good practice for C++ developer. Because you don't want to accidentally set a variable in an if statement if you leave off the extra =. Remember C++ does not check to make sure all if evaluate to a bool. C# does. - David Basarab
(1) I Agree with the problem, but i think the solution might benefit from adding a note about localization. - TokenMacGuy
55
[+12] [2008-09-22 13:46:37] MrZebra

Overuse of casting

Lots of casting between (potentially) unrelated types makes it difficult to tell what type variable pointed to actually is, and is usually a sign that someone is trying to be too clever with memory usage. Use a union if you really want to do this.


56
[+11] [2008-09-22 11:39:50] GEOCHET

Interface overuse. Any time anyone thinks that interfaces are the answer to all their problems and decides to use the same pattern everywhere, there is something wrong at a low level.

Time to sit these people down and make them understand that they can code simple, clear classes without interfaces too.


Agreed however some test frameworks require interfaces for mocking objects which adds to the number of interfaces. - Paul Whelan
(1) Use a different test framework. - Apocalisp
@apcalisp: Agreed. If your testing framework is making you change your coding style (and not in a good way) then it is time to find an alternative. - GEOCHET
(1) To be fair, how do you do your dependency injection if you can't mock out the complex classes? I admit I'm not a fan of creating an interface whose only implementations are the production class and the mock object, but if the alternative is no tests then I'll live with the extra interface. - richq
@RichQ: I stand by my assertion. Code patterns should not be dictated by your test environment. If you find yourself making every class using an interface pattern, that is a good indication something is wrong. - GEOCHET
Agree, but heavy use of interfaces at logical integration points, between Data Layers, Business layers, and UI is acceptable. - Chris Ballance
While this may be an inconvenience, many senior software architects would disagree with this general sentiment. Unfortunately, it does involve a paradigm shift that is necessarily difficult to understand at first. petercoad.com/download/bookpdfs/… - Luther Baker
Interfaces can be abused, but they are also an invaluable tool in partitioning your code so that you can reduce your dependencies and make the code more tastable. - George V. Reilly
(3) I dissagree. In most cases usage of interfaces is a sign of decoupling whereas inheritance is a bad smell of not knowing what it implies. Even tag interfaces are good in many cases (you wouldn't force anyone to inherit from your base class just to be able to say "if it's derived from this class then I'll go ahead - otherwise do nothing", right?) - Matthias Hryniszak
57
[+11] [2008-12-07 09:13:32] Norman Ramsey

Comments often make code smell. Steve McConnell said it before me, but

  1. Comments on functions and code can almost all be eliminated by choosing names and types well.
  2. Comments on data are often helpful. Beginners can be told to comment every type definition and variable declaration. This is overkill but is a good rule of thumb and is way better than commenting code.
  3. Even better comments are just ones that describe the representation invariants of data and say what abstraction the data is intended to represent.
  4. If the representation invariant is not completely trivial, the best documentation is an internal function which validates that the data actually satisfy the representation invariant. Good example: ordered binary trees. Better: balanced, ordered binary trees. In this case you write the code to check the invariant and the comment just says that every value of the given type satisfies the invariant.

Summary: strive to move information from comments into code!


assertion checks are also a great way to move information to code, e.g. to declare thorny loop invariants. - user192472
58
[+11] [2008-09-23 02:47:46] JohnMcG

Smell: Operator overloaded to perform a non-intuitive function. e.g. operator^ used to convert a string to uppercase.

Problem: Very likely clients will use it incorrectly.

Solution: Convert to a function with a sensible name.


+1 because this is what I thought when I was confronted the first time with std's stream operators: WTF!!! - Flinsch
59
[+10] [2008-09-23 00:03:30] Robert Paulson

Too Many [out] Parameters (.NET)

When a method contains out parameters, especially when there are more than one out parameter, consider returning a class instead.

// Uses a bool to signal success, and also returns width and height.
bool GetCoordinates( MyObject element, out int width, out int height )

Replace with a single return parameter, or perhaps a single out parameter.

bool GetCoordinates( MyObject element, out Rectangle coordinates )

Alternatively you could return a null reference. Bonus points if the class implements the Null Object pattern [1]. This allows you to get rid of the boolean as the class itself can signal a valid state.

Rectangle GetCoordinates( MyObject element )

Further, if it makes sense, have a specialised class for the return value. While not always applicable, if the return value is not a simple true/false for success then it may be more appropriate to return a composite of the returned object plus state. It makes caller's code easier to read and maintain.

class ReturnedCoordinates
{
  Rectangle Result { get; set; }
  CoordinateType CoordinateType { get; set; }
  GetCoordinateState SuccessState { get; set; }
}

ReturnedCoordinates GetCoordinates( MyObject element )

Admittedly overuse of this can lead to further bad smells.

A Good [out] Pattern

Note that [out] parameters are still useful, especially in the following Tester- [2] Doer [3] pattern.

// In the Int32 class.
bool TryParse(string toParse, out int result)

this is far more efficient than

// BAD CODE - don't do this.
int value = 0;
try 
{
    value = int.Parse(toParse);
}
catch {}

when you expect the input string toParse is probably not valid.

Summary

In many cases, the presence of any [out] parameters indicates a bad smell. Out parameters make the code harder to read, understand, and maintain.

[1] http://exciton.cs.rice.edu/javaresources/DesignPatterns/NullPattern.htm
[2] http://blogs.msdn.com/kcwalina/archive/2005/03/16/396787.aspx
[3] http://dotnetperls.com/Content/Tester-Doer.aspx

60
[+10] [2008-09-22 12:03:14] Manrico Corazzi

Conditional spree

Complex behaviour implemented by intricated if...then...elseif... blocks or long switch...case copy/pasted all over the class(es).

Suggested refactoring: Replace Conditional with Polymorphism [1].

NOTE: overusing this strategy is also "code smell".

NOTE2: I love the Kent Beck quotation from one of his books on Extreme Programming.

If it smells change it (Grandma Beck on childrens rearing).

(or something like that, I don't have the book handy right now).

EDIT: For a comprehensive list have you considered this post on Coding Horror [2]?

[1] http://www.refactoring.com/catalog/replaceConditionalWithPolymorphism.html
[2] http://www.codinghorror.com/blog/archives/000589.html

61
[+10] [2008-09-22 13:12:23] Pascal T.

The first wiki ever (http://c2.com) has a lot of stuff about refactorings and code smells.

http://c2.com/cgi/wiki?CodeSmell

This is my main source of information about refactorings.


62
[+10] [2009-07-26 04:11:34] akf

Side-effecting getters:

public Object getMyObject() {
     doSomethingThatModifiesSomething();

     return myObject;
}

That's even beyond a code smell in my opinion. - dstibbe
(1) Right.. this is a misunderstanding of the concept of a getter. - lttlrck
(1) There is only one reason to do so: lazy initialization. - Arne Burmeister
....And caching - Josh Smeaton
63
[+9] [2010-10-30 19:16:00] smirkingman

Negated booleans.

Wrong:

Dim NotReady as Boolean
...
If Not NotReady Then

Right:

Dim Ready as Boolean
...
If Ready then

(2) That`s not really a code smell, just bad style. - Mark Peters
I don't agree. I like to make all my booleans false by default. I would probably implement your example with an IsUnready - jpbochi
(5) Well, if you understand "If Not IsUnReady" better than "If IsReady", then I can't argue. - smirkingman
64
[+9] [2008-09-22 12:41:31] aku

Checking for file existence before trying to access it (instead of I/O error handling).

Common error of novice programmers. Instead of handling I/O exceptions, they check file for existence.

Forget about File.Exists-like methods unless you use files as markers\locking objects. Always handle file I/O errors when trying to read/write some meaningful data.


(3) Apparently, it's a common error of experienced programmers as well, since I and many others do this all the time. Exceptions have a surprisingly large amount of overhead to them, and it is not unreasonable to avoid exceptions with a simple (and cheap) IO call. - MusiGenesis
(2) I hear this all the time from fellow devs. Exceptions?!, oh!, ah!. Funny to see their faces when file disappears right after File.Exists check :) - aku
Heh, who cares about performance in an exceptional scenario? As an aside I do a File.Exists so I can throw a more meaningful exception. "File xyz could not be found" as opposed to: "There was a problem with file xyz". - Quibblesome
In a similar manner, the same applies for functions to check if the file is accessible (i.e. if it exists but you just don't have any rights). Checks like that are bound to fail since a file can change status between the check and the access, and from the exception it's possible to get exact error. - Michael Stum
this is a recommended best practice by microsoft and others - check for possible exception conditions beforehand; this does not mean that checking for them afterwards is unnecessary though - Steven A. Lowe
I'd give this +3 if I could - Thomas Tempelmann
You have to handle exceptions from the file system, no question. - George V. Reilly
Exceptions are not free: hundreds, perhaps thousands of cycles. Disk I/O takes hundreds of thousands of cycles. CPUs: nanoseconds; disk seek times: milliseconds -- six orders of magnitude. - George V. Reilly
(1) re: "Exceptions are slow, so do this". Disk IO (which will be required for File.Exists) is FAR slower, and (as pointed out) you still are going to have to handle the exceptional case. I don't see the logic in trying to justify 2 Disk IO calls + exceptions over 1 Disk IO call + exceptions for purposes of speed. - Goosey
65
[+9] [2008-09-22 14:15:23] David Hill

In C#, invoking GC.Collect repeatedly, for any reason. This is a serious pet-peeve for me, and is almost always indicative of "Let the garbage collector handle it!" syndrome.

Yes, in C# we have a garbage-collector. No, it doesn't make up for messy allocation/deallocation logic.

To correct: disable the GC.Collect code, use perfmon, CLR memory counters, and find those leaks. Make sure that dispose is implemented properly, and called properly.

Use "using" statements whenever possible. Refactor if necessary to make "using" statements work for you.

They call it a self-tuning garbage collector for a reason, trust it to do it's job properly.


66
[+9] [2008-09-22 11:58:21] aku

Inconsistent enum members.

I know 2 variations of this problem:

  1. enum members that actually belong to different domains (good example is .NET BindingFlags).

  2. Tricky enum members:

    enum MathOp
    {
     Plus,
     Minus,
     Empty
    }
    

This often happens when enum values used in UI.

Cure:

  1. Group enum values into different logically related enums.
  2. Don't mix logic and presentation

Aku, brilliant mate. Exactly what I am looking for. - Rob Cooper
67
[+9] [2008-09-23 03:10:15] rjurney

Exception handling blocks which say only:

/* We are SO SCREWED! */


Yep, I do something like that often: I write: "// oops, this was unexpected" :) - Thomas Tempelmann
@Thomas: If it was unexpected, why did you code for it? ;P - Tordek
@Tordek unexpected <> impossible - Paralife
68
[+9] [2008-09-22 23:36:54] Georgi

Trying to be more precise in an error case by parsing the error message.

I often saw something like

try {
    File f = new File("anyfile.txt");
    if(file.isDirectory()) {
        throw new IOException("File is a directory!");
    }
    file.open();
}
catch(IOException ex) {
    if(ex.getMessage().indexOf("File is a directory")>=0) {
        System.out.println("The file is a directory!");
    }
    else if(ex.getMessage().indexOf("File does not exist")>=0) {
        System.out.println("The file does not exist!");
    }
}

The strange thing is, if you change the error message, the behavior of the code will change ;-)

How to avoid:

Split the code-block and react to the errors individually. More to type but definitely worth it.


This always catches people out when they start supporting foreign languages as the error messages are often dependant on the current culture. - Martin Brown
This smell is often caused by exception classes not having enough detail on them and people throwing general rather than specific exceptions. - Martin Brown
how to aviod: add an errorcode to the exception. these codes should beconstant over different cultures - k3b
69
[+9] [2008-09-23 05:47:08] RodgerB

Smell: Long lines of code

My definition of a long line of code (because I'm a .NET developer), is any line of code that requires a horizontal scroll bar to be viewed in the editor in Visual Studio (without collapsing the toolbox or the Solution Explorer pane). The developer should visualise the poor programmer working at a low resolution, with a seemingly never ending horizontal scroll bar.

Example:

Dim cn As New SqlConnection(ConfigurationManager.ConnectionStrings("DatabaseConnectionString").ConnectionString)

Solution: Use New Lines

Break up your code into appropriately sized pieces, not bite sized, not generous, but just right.

Example:

Dim cn As New SqlConnection( _
ConfigurationManager.ConnectionStrings("DatabaseConnectionString") _
.ConnectionString)

(2) I have the impression that splitting into more lines code like that is like using deodorant. - user51568
(1) My opinion is quite the contrary.. Splitting a single line of code into many lines just makes it harder to read. - Miky Dinescu
(2) I dissagree with this one. It'd have to be a veeeeeeeeeery long line (like 500+ characters) for me to actually consider splitting it. Also debugging such code is harder (the pointer keeps jumping from one place to the other - generally it's a nightmare). If kept as a single line it's a single method call. If one would like to trim the line down it'd be through refactoring and proper naming convention rather than the use of underscore and/or newline character. - Matthias Hryniszak
(1) @Matthias. A 500+ character line is pure nightmare to me. That means more than 5 screen widths to scroll in order to know what your line is doing and whether it is not doing more (or less) that you'd think it does by looking at the first screenful. Don't you never need to reread our code ? (And won't coworkers ever need to read your code ?) If you really want that to hold in a single line, make it a single neat function call to a new function. - user192472
OK, let's make one thing clear: if a line contains just strings it'd really have to be really long to split it. But in the example above splitting it into 3 lines is in personal opinion BS and makes a clean construction of a new object unreadable. I've seen some crazy constructs like this (for example class name on one line with dot and the method name on the next line) and believe me it's not something you'd like to read at all. - Matthias Hryniszak
@fred-hh. Ok, I've probably pushed it a bit to far with those 500+ characters :) But definitely going off-screen when stuff is obvious would be my preference as opposed to forcible line splitting just for the heck of it. - Matthias Hryniszak
500 char is 2 screen widths for me, And i use this method to hide nightmares off the screen. - Behrooz
70
[+8] [2008-09-24 15:15:09] Brian G

How about code without indentation. I seen a friend of mine with a master's degree write software without indentation and using variables like x, x2 and y. He actually applied to a position at Microsoft and sent them a code sample like this. How fast do you think they tossed it in the garbage???

Code is for humans to read.

Please indent.

What would you do if you received un-indented code as a part of an interview?


(1) Another idea: when using identifiers, spell them correctly. When using them multiple times, spell them identically. Even in comments, where compilers won't check it. - reinierpost
71
[+8] [2010-10-29 09:31:22] cHao

Printing "Your password may (only|not) contain the characters...". Ever. Ditto for comments and most other non-key text fields. The existence of such restrictions is a sure sign that the data is being mishandled somewhere, unless the code is just preemptively being a pain in the user's ass.

To fix:

  • If you restrict user input, there should be a valid business reason why. "Preventing SQL injection/XSS/some-other-bug" is not a valid reason; if you're having that problem, then you're handling the data wrong. Read up on escaping and/or quoting and/or prepared statements, and apply what you read. Properly.
  • Hash passwords. (Once they're hashed, it doesn't matter what the original chars were.)

not a code smell - Dan
To a large extent, it is. If you ever have the need to display such a message, it's almost certainly because your app has an SQL/XSS/etc vulnerability just waiting to happen. And if it's on a password prompt, you're (1) making it easier for attackers to guess passwords, since you're restricting the input domain, and (2) more than likely storing passwords unhashed. Code smells aren't style issues (like, say, Hungarian notation or spaces/tabs or whatever); they're quirks in the code whose very existence hints at a much larger problem. - cHao
it's a security issue, not a code design issue :) - Dan
Just means the smell's a little different. It's still a smell. :) - cHao
72
[+7] [2008-09-24 13:46:51] Justsalt

Unnamed boolean parameters to functions, especially when there is more than one. Here is such a function declaration (in a C-like pseudo language):

cookBreakfast(boolean withEggs, boolean withToast, boolean withJuiceNotMilk);

The function call is incomprehensible:

cookBreakfast(true, false, true);

Solution: use enums or named parameters instead. How this is done will be language dependent.

cookBreakfast(eEggsYes, eToastNo, eJuice);

or

cookBreakfast( withEggs => true, withToast => false, withJuiceNotMilk => true);

or

BreakfastOrder bo;
bo.withEggs = true; bo.withToast = false; bo.withJuiceNotMilk = true;
cookBreakast(bo);

73
[+7] [2008-09-22 23:26:39] Robert Paulson

Reassigning Parameters in Method Body

Reassigning parameters in the method body is a bad smell. It's a source of confusion and can be a source of errors when the code is edited later.

Was the programmer trying to alter the caller's reference, or were they just lazy and unimaginative? Was it a mistake?

void Foo( MyClass x )
{
  if( x.SomeProperty ) ....

  // ...    
  if( someCondition ) { // yuck!
     x = new MyClass(); // reassign's local reference x, parameter x is lost
  }

  // ...
  DoSomething(x); // which x should this be?
}

To fix, create a new variable, or consider refactoring such that reassignment is not necessary.


74
[+7] [2008-09-22 21:54:27] srclontz

Useless logging....

try {

}
catch (Exception e)
  log.error(e.printStackTrace());
}

Instead try to think about what sort of error might occur, and put something useful in the logs. This could be something like, "Properties File Not Found" or "Unable To Connect To Database"

Try to catch specific errors, rather than a general exception so that when it fails, the program you wrote won't be immediately blamed. For example, if there is a connection error, put it in plain english, "Database Connection Error".

Better yet....handle the error in the code without necessarily making it to the catch block.


I don't agree!!! You should call this "eating exceptions", at least in the example the author is logging an exception before eating it. He should also rethrow it though. - Sklivvz
(5) I disagree. This is entirely appropriate within production code. We do this and get the logs from our customers and fix issues found in the field that we couldn't find in our test lab. - torial
Err..return type of printStackTrace() is void :/ - shyam
@shyamsundar: Not in non-existant language x! - Brian
well,... I see code like that all the time in Java because some language designer figured out that it'd be great to have a declarative way of notifying people of exceptions being thrown from the code they use and forced one to deal with those. My suggestion is that you move to a modern language like Scala or Groovy if you're into this dynamic way of doing things and leave the poor grandma (java) in peace. - Matthias Hryniszak
75
[+7] [2009-01-09 12:49:00] Khainestar

Assumptive code.

Code that assumes something has happened before it, or assumes that a value has been set. I know some compilers like to tell you about it but I have seen this very widespread in PHP in particular. An example.

if ( $foo == 'bar' ) {
    $bar = true;
}

if ( $bar ) {
    // code...
}

This becomes a huge problem when poor structure doesn't create objects. Then later code starts using the objects, or worse someone directly sets values into an object that doesn't exist and PHP helpfully creates a standard object, with the value in it. So later checks for is_object return true.

Solution.

If you are going to start using an object make sure that it actually exists.

$object->foo='bar';

Will create an object but it won't be the object that you think it is. Accessors are there for a reason. Use them when ever possible. This also removes the problem of assuming something is there to use as the script will error out and then it has to be fixed.


76
[+7] [2009-01-09 14:05:50] Wix

A common thing I see with new programmers is having 20 includes at the top of a header file. I found that the developer is trying to do too much in one class/file (depending on language) or they are calling everything in an assembly to simply use one object/method/whatever in it.


77
[+7] [2009-04-14 15:48:13] Khainestar

Another one that has raised its head lately. Three state booleans. Yep you heard me right, three state booleans. Database fields set to be a boolean but allow null. So you can get true, false and null from them. Then code that checks not only for value but also type. Since 3 state booleans don't exist this causes some major headaches.


True, False, and FileNotFound! thedailywtf.com/Articles/What_Is_Truth_0x3f_.aspx - Kip
Yes/No/IDontKnow :D LOL that's a great one :) - Matthias Hryniszak
but 3-state booleans do exist ;-) - user192472
I've seen boolean database fields where true was used correctly, null was treated as false, and false wasn't even used in the code. I didn't dare try putting an actual false value in there; who knows what would have exploded. - Jarett
78
[+7] [2008-10-06 20:24:30] Osama ALASSIRY

I hate it when I see code like this:

if (a==b){
  //Do This
  if (c==d) {
    //Do That
  }
  else
  {
    //Something Else
  }
}
else
{
  //Do This Again
  if (c==d) {
    //Do That Other Thing
  }
  else
  {
    //Something Else Again
  }
}

especially if most of the code is common between the cases.

it looks much nicer if we convert the code to separate functions instead of copy paste, and write something simpler like:

dothis();
if (c==d) 
    if (a==b) dothat() else dothatotherthing();
else
    dosomethingelse();

All we need to do is analyze the logic, and simplify the code. encapsulating code in small functions with descriptive names is always a good thing to do.


-1 for not following the question's rules - you're supposed to offer a solution (i.e. a fix) to the problem. - Thomas Tempelmann
fixed :) I parsed "it would be great" as the way to fix it being optional, thanks. - Osama ALASSIRY
79
[+7] [2008-09-22 12:47:13] Nazgul

Lot of static methods

This means that those methods do not belong to the class in which they are. So move them to a relevant class.


This one is a duplicate of the one that talks about singletons. - Matthias Hryniszak
in c# static classes with static methods are the only way to implement extentionsmethods - k3b
80
[+7] [2008-09-22 13:58:14] MrZebra

Overuse of meaningless variable names

If you see a function that has lots of variables like "a, b, c, x, y, z", it can indicate that the function's logic was poorly thought out. Clear variable names tend to show that the author has a more clear understanding of the operation of the function, and also assist the reader's understanding. It should be refactored by renaming the variables to something more descriptive.


Conversely, encoding a function's meaning in variable names can be a clue that the function is poorly typed. When the types make the meaning obvious, there's no need for excessive naming. - Apocalisp
Only sometimes true. I see no problem in using "i, j, k" as "for" indexes, math style. Also not all variables have such an important meaning that you need to give them a proper name. Using scratch variable names is useful to make the code legible (this variable is important, this is not) - Sklivvz
Sklivvz: To clarify, I too see no problem with using i, j, k as indexes, or for unimportant variables. This is why I said "overuse", not just "use". What I'm saying is that if ALL your variables look like that, then you have problems. - MrZebra
The only thing worse than a meaningless variable name is a misleading variable name. - Alan Hensel
Well I agree in some languages, but you're writing in c++ where there are no good looping constructs (except for boost) then it's a pain to come up with good names and iter is just fine. - Anders Rune Jensen
This is related to the code-written-by-mathematician smell. Often if you take the time to go look up the obscure paper describing the algorithm, you find that the letters (even the ones used as indexes) have very intuitive names, and suddenly the algorithm is actually readable. I think they do this on purpose, otherwise nobody would bother to read their stupid paper. :) - James M.
81
[+7] [2008-09-22 13:14:36] Omar Kooheji

Methods that are exactly the same except for one parameter.

I recently picked up an applications to review that had 20 methods, every pair of methods were exactly the same except they were processing two different types of data...

I refactored this into a base class with the majority of the functionality and two child classess that only overrode the processing that was different between the two types of data.

Much easier to understand and if a change to the way things were processed was required I usually only had to make the change in one place in the base class.


I think in your example you were totally correct in what you were doing, but lets not discount that method overloading is fine in itself, but the design should be right (i.e. no code dupe etc). - Rob Cooper
82
[+6] [2008-09-22 17:17:51] Michael Lang

C# (perhaps smelly in Java too): Use of collections of type object

To me this smells really funny and indicates that the purpose of a collection may have not been thought out very well. As far as I know, these should only crop up in implementations of something like a completely generic property bag paired with some helper method that performs an attempt to cast and retrieve.

Otherwise this usually indicates the objects going into the collection should implement a common interface which in turn would be the type of the collection elements.


Well... here's the thing: In c# (and other .NET languages) it's actually reasonable to use generics because they get passed on from the compiled code to runtime. In java if you declare a List<Person> it ends up being a List<Object> anyways. I personally hate the way generics are screwed in Java. I use them mostly out of habbit from other languages and because it seems the right way to do it. - Matthias Hryniszak
I either misunderstand your comment or agree. I'm trying to advocate the use of Generics language features (like List<Person>), so that your collections are strongly typed. I find that strongly typed code is far more communicative of intent. I personally think there's very few excuses to be dealing with the ultimate base class of object (Object in java I guess, but I'm not qualified to speak on java), so if I see collections of objects (Like C#'s ArrayList) I'm compelled to investigate to see if there's a good reason for it, or if the programmer's being lazy with their types - Michael Lang
83
[+6] [2009-01-30 19:43:44] Sridhar Iyer

Global variables

Normally most developers with even a thimble worth of knowledge stay away from them. But somewhere down the line, in some year, somebody inevitably adds one to short circuit some logic or just get a legacy system to work.. further down the line this causes issues in multithreaded systems which are caught much much later and almost too easily escape regression tests.


84
[+6] [2008-10-05 08:45:14] I GIVE CRAP ANSWERS

Lack of abstraction.

  • Description: the code is written such that everything happens on the same level. There is no separation between different concerns and no splitting into parts.

  • Key indicators: you suddenly find presentation code in the business layer, or you find business code in the data access layer. The line count of a feature is much too big for what it does. The code space looks 'flat' and you don't find yourself having to look up and down the chain of abstraction.

  • Fixing: refactoring is key as always. Define your abstraction layers; for instance data access, business logic and presentation. Then slowly begin pushing code into the right layer when you find it. Suddenly other code smells will show up in each abstraction layer (code duplication is common) making it possible to further simplify the code. It is very much possible to refactor such code into elegance.


85
[+6] [2008-09-23 02:48:11] JosephStyons

Code Smell: A giant chain of classes descending from each other, and only the very last one is ever used.

Solution: Usually this says the design is waaay over-engineered. Usually the descendants can be rolled up into a simpler parent-child relationship.


86
[+6] [2008-09-24 06:23:22] timdisney

Comments used to mark out unrelated or loosely related sections of code. Usually means that a file is trying to do too much and should be broken apart into separate files/classes.

//########### Code to do foo ###########
// 500 lines of code...
//########### Code to do bar ###########
// another 500 lines of unrelated code...
//########### Code to do baz ###########
// ...

87
[+5] [2008-09-23 16:14:48] C. Lawrence Wenham

Putting too much meaning into boolean parameters. Eg, the method that starts with:

public void Foo(bool isMonday)
{
   int hoursToCheck = 24; 
   bool ignoreHeader = false;
   string skipLinesContaining = "";

   if (isMonday)
   {
      hoursToCheck = 12;
      ignoreHeader = true;
      skipLinesContaining = "USD";
   }

   ...
}

The isMonday parameter is loaded with too much meaning, and the three implied parameters should be passed on their own.

The same smell manifests itself in enums and configuration settings as well. Be on the lookout for boolean-like parameters that have vague names that could imply many assumptions.


This smells but the smell can be kept low as long as the code is well commented and anything that is happening inside the entity is made known. - Sandor Davidhazi
(1) I think that code would be much improved by passing the actual day into the function (as an enum) then using a switch. - DisgruntledGoat
+1 to DisgruntledGoat. Too many parameters is also a problem, and in this case there may be no use in disclosing these internals. Passing the day-of-week or the actual date looks like a good compromise. - user192472
@DisgruntledGoat, @user192472, what if the flag was isEasterSunday? - CMR
88
[+5] [2008-09-23 03:27:49] Ferruccio

Voodoo code

Code that repeats a call do something more than necessary, just in case.

Or, my favorite example: putting try/catch blocks around code that can't possibly throw an exception. Again, just in case.


89
[+5] [2008-09-23 02:41:59] JohnMcG

Smell: Database columns with names like value1, value2 ... valueN

Problem: Modleing a many-to-many relationship without a marriage table.

Solution: Create a marraige table to normalize the data model.


90
[+5] [2008-11-19 03:52:45] Darren

Many Helper Classes

Prolific use of helper type classes. I'm defining a helper class as one that contains a bunch of related methods that do some common task. They are typically used when you find yourself repeating the same type of code over and over again.

I believe that this either points to lazy coders not thinking about the best place to put the code in the existing API or a failure of the API itself not providing decent default behavior.


(1) In some languages it's the only way to go. For example in Java the String class is final, sealed, basically closed for bussiness. The only way you can go is to create helpers. Other languages, like C# or Groovy allow for extensibility of all classes and it makes reading code a lot easier. - Matthias Hryniszak
91
[+5] [2008-09-30 17:37:22] Mike Dunlavey

Too many classes, too many objects, too many event-like actions

Not every noun should be a class, and not every verb should be a method. If an object doesn't exist to capture and retain user input, be sure you really really need it.

Examples:

If you are given an array of x,y points, and you want to produce a graph, chances are all you need is a routine to draw it, not build it out of point and line objects.

If you have a tree structure, and there is a property of the tree that depends on properties of its subtrees, chances are all you need are functions that sweep through the tree, not a whole lot of event-handling to propogate change events from children upward.

Register and Unregistering anything smells because those are events whose purpose usually is to incrementally maintain correspondence between things. The rationale is usually to save cycles. Machines are incredibly fast these days. You could possibly just rebuild the structure in less time than you would ever notice, and it would take a whole lot less code and bugs. Another way is to get good at Diff-type algorithms to maintain correspondences.

Back pointers smell. Usually the reason is to save cycles. Then you need unit tests to try to prove that they never get inconsistent. If you design the data structure so there's almost no way you can change it to something inconsistent, then there's almost nothing to unit test.

Everybody loves object-oriented programming, right? But don't let that mean the more objects the better! Let it mean the fewer objects the better. Just because event- or message-based programming is a good way to handle certain problems, don't let that mean it's the ideal paradigm for everything. One may think they're saving cycles, but every cycle saved costs a 100 in managing complexity. Usually this turns into a creaky monstrosity that is never quite right because of messages being forgotten or duplicated or happening at the wrong times.

Comment written on a proposal of mine by my boss, a long time ago: KISS. "What's that mean?" I asked. "Keep It Simple, Stupid!" was the reply. Been living by that ever since.


92
[+5] [2008-09-27 10:59:26] Shane MacLaughlin

Wow, most bad smells gone already. Ok in C++ how about delete this, whereby an object commits ritual hari kari without letting anyone else know. I've seen this used variously for watcher and status monitoring routines, and it often smells pretty bad, notably when there are references to the same object elsewhere in the program that aren't informed of the objects demise.

The way I usually refactor this is to make the object pointer a member of another object with broader scope, e.g. the application object.


93
[+5] [2008-09-22 13:27:09] Pace

Input variables that are then modified within a routine. If you ever need to revert back to what was passed in, it has already been changed. I always set an input variable to some form of working variable. That way, you can always reference the original value and it keeps the code clean.


94
[+5] [2008-09-22 20:50:45] David

Catch blocks that simply do exception.printStackTrace()

Exceptions should be handled properly, not simply printed.

  • If a class can't handle the exception on its own, it should be thrown to the caller
  • At a minimum, exceptions should be logged
  • If nothing else, something user-friendly should happen... (any suggestions?)

This applies to product-level code... I'd be more lax on this rule if it's for an internal tool.


95
[+5] [2008-09-22 13:09:34] Omar Kooheji

Not checking for null arguments on publicly visible methods.

Explanation

Any method which is publicly visible assumes that all its arguments have a value.

Solution

Check for null arguments and either deal with them if it's possible or just throw an ArgumentNull exception.


(3) It's better to make sure that the caller complies to the contract of the method. Bullshit In, bullshit out. - xmjx
(2) "BS in, BS out" might be fine for things like Java where an exception will be raised that the caller will likely handle, but having a C program crash because of this is unacceptable. Public methods must validate input, which includes checking for NULL. - Graeme Perrow
Pray, Mr. Babbage, if you put into the machine wrong figures, will the right answers come out? - Tordek
(2) I agree. ArgumentException, InvalidArgumentException... anything along this line is a must in public APIs. - Matthias Hryniszak
96
[+5] [2008-09-22 12:56:54] Paul Shannon

Pattern Duplication

Not just copy/paste of lines but similarity in the methodology. For example, always setting up a transaction, calling arbitrary methods on objects, returning a list of objects, tidying up, etc. This could be refactored into a base class


97
[+4] [2008-09-22 12:59:43] Richie_W

Catching un-checked exceptions - i.e. NullPointerException


Yeah, I agree. Richie_W, honestly...I love you - Richie_W
98
[+4] [2008-09-22 13:16:53] AviD

In contrast to the aforementioned "megaclasses", the opposite (nullclass?) are also smelly: classes that have absolutely no responsibility and are simply there for enterprisey generica. Same goes for methods that call the next method with the same arguments, which then call the next method with the same arguments, etc.

The solution, as with the megaclass, is to properly define proper responsibilities for each class, and purpose for each method. Try to flatten the class hierarchy as much as possible, adding complexity and abstractions only when absolutely necessary.


(1) Yes, but bear in mind the Law of Demeter, en.wikipedia.org/wiki/Law_of_Demeter, "Only talk to your immediate friends". Calling something like a.b.c.d.e.foo() is inappropriately tight coupling. - George V. Reilly
This is common in J2EE/JEE applications. Eg., EJBs are kept as _pass-through_s to business classes, to ensure business logic is not held inside proprietary code brackets. - CMR
@CMR, then those classes also have a responsibility: exposing a specific interface (i.e. EJB). This is a common pattern, wherein the EJB serves as service interface. That is different from the smell I was referring to, where the class has NO actual responsibility whatsoever. - AviD
agreed. Good point. - CMR
99
[+4] [2008-09-22 21:37:22] community_owned

General error handling

In languages where exception handling is possible, typical bug is to have all exceptions caught. This means the developer is not aware what kind of errors could occur.

For example in PL/SQL 'EXCEPTION WHEN OTHERS' smells.


100
[+4] [2008-09-22 14:04:41] community_owned

Too many dimensions for arrays, like:

double *********array = NULL;

That's definitely a call for better data handling.


101
[+4] [2008-09-22 12:40:52] Gishu

Nicely disguised question. Voting smells up and down :)

Isn't this duplication of the refactoring book? Since all of this is already available in a nice place called http://refactoring.com/catalog/index.html with examples to boot..


(2) @Gishu: Duplication? Yes, but thats not a bad thing is it? Centralisation of programming material, Democratic inspection of underlying premises, and an end to the singular ownership of literature are why we are here, no? - Ande
Right on. Please continue... - Gishu
(1) I think this is a worthwhile question. It's good to see some popularity / utility figures (in the form of voting) on different smells. It's also useful to see the way the practice of refactoring exists "in the wild". - Wedge
(1) Jeff and Joel have said that the point of stackoverflow is to get knowledge out there because people don't read books anymore. - John Ferguson
102
[+4] [2008-09-22 11:59:29] Midhat

Configurable constants included in the code


Read the question: What is the smell? How do you correct it? - Rob Cooper
I daresay the smell is the first lines of the class cluttered with const or static final variables, and you'd better move the configuration to a separate ini/properties/XML file, or the database if that is more your cup of tea. - Manrico Corazzi
(1) But at the same time, there's another smell when you put things into configuration that realistically are never going to get changed. - slim
+1 to @slim. An application I had to maintain had one of these a configuration nightmares... It felt as if the code was read from the database, compiled and executed at run time, every time. - CMR
103
[+4] [2008-09-24 19:22:38] community_owned

Suggest you read Refactoring: Improving the Design of Existing Code by Martin Fowler, Kent Beck, John Brant, and William Opdyke (Hardcover - Jul 8, 1999)


104
[+4] [2008-10-07 21:07:15] Jake
// This should never happen.

Explain how to remove it. - user51568
(1) File->New->Project - mpeterson
(1) solution: delete the comment. It is wrong! - TokenMacGuy
At least when you see that you know they are planning for the unexpected :). - Simon
Haha, always funny. But a code smell? nah not so much. Rather just replace it with throwing an exception - Mahol25
105
[+4] [2008-11-05 02:32:11] jfpoilpret

Catching an exception to just rethrow it, in the same form or another form.

It is very common (unfortunately) to meet this kind of code (Java) with 3 different ways to (not) deal with caught exceptions:

try {
    ...
} catch (ExceptionA e) {
    throw e;
} catch (ExceptionB e) {
    log exception
    throw e;
} catch (ExceptionC e) {
    throw new RuntimeExceptionC(e);
}

This code is plain useless: if you don't know what to do with an exception then you should let it pass through to the caller. Moreover, this bloats the code and hinders readability.

Note that in the third case (throw new RuntimeExceptionC(e);), we can argue in some specific cases where this might be useful (normally rare however).


Wrapping an exception to add additional context information or for other purposes (cross application domain serialization perhaps) can be useful. In general I wholeheartedly agree. - orj
I wouldn't mind seeing this pattern if the rethrower knows why this would be an exception, and can reraise it with instructions about what the caller did wrong. - TokenMacGuy
106
[+4] [2009-05-15 04:43:31] zonkflut

Annother Double negative issue in C#

if (!!IsTrue) return value;

I have seen he double ! cause bugs in the past.

instead remove the !! to avoid confusion


this boggle my mind. Why would anyone use a double negative? - Tim
(3) My guess would be some sick sadistic masochist programmer wanting to assert their dominance over future readers of their code base. - zonkflut
(2) This pattern exists to cause a non-boolean value to become boolean. of course C# provides other, more obvious ways to do this, so it's not likely of much real value. - TokenMacGuy
107
[+4] [2009-06-26 04:21:33] jrista

Encapsulated Dependencies

The Smell

When dependencies are directly instantiated within the dependent class, managing those dependencies rapidly becomes a maintenance nightmare. In addition, when dependency management is fully encapsulated within the dependent class, mocking those dependencies for unit testing is impossible, or at best extremely difficult (and requires things like full-trust reflection in .NET.)

Solution: Dependency Injection

The use of Dependency Injection (DI) [1] can be used to solve most dependency management issues. Rather than directly instantiating dependencies within the dependent class, dependencies are created externally and passed into the dependent class via a constructor, public setter properties, or methods. This greatly improves dependency management, allowing more flexible dependencies to be provides to a single class, improving code reuse. This allows proper unit testing by allowing mocks to be injected.

Better Solution: Inversion of Control Container

A better solution than simply using dependency injection is to make use of an Inversion of Control (IoC) [2] container [3]. Inversion of Control makes use of classes that support DI, in combination with extern dependency configuration, to provide a very flexible approach to wiring up complex object graphs. With an IoC container, a developer is able to create objects with complex hierarchical dependency requirements without needing to repeatedly and manually create all of the dependencies by hand. As IoC containers usually use external configuration to define dependency graphs, alternative dependencies may be provided as part of configuration and deployment, without the need to recompile code.

[1] http://en.wikipedia.org/wiki/Dependency%5Finjection
[2] http://en.wikipedia.org/wiki/Inversion%5Fof%5Fcontrol
[3] http://www.castleproject.org/container/index.html

-1 for not knowing that DI and IoC is the same in this case. - Matthias Hryniszak
(1) @Matthias: I was not comparing DI to IoC. I also was not talking about IoC generally, I was talking about IoC "Containers" specifically. There is a significant difference between the concept of Dependency Injection, and the use of an 'IoC container' to achieve it. I think your downvote is completely bogus. - jrista
108
[+4] [2009-07-26 03:57:30] Kip

Error messages when users perform perfectly valid actions

I'm thinking of this one I saw when trying to change my password somewhere:

NOTE: Using a colon (“:”) in your password can create problems when logging in to Banner Self Service. If your password includes a colon, please change it using the PWManager link below.

Solution: Don't be lazy. Sanitize user input properly.


109
[+4] [2008-09-23 03:07:08] JohnMcG

Smell: query in a program that is either "SELECT *" or an insert without column names.

Problem: if the structure of the table changes, the code will break.

Solution: be explicit about what columns are being selected or inserted.


Technically speaking, the code may break even if you specify the columns you want. Nonetheless, it's better practice to be explicit, as you'll pull less data across the wire. - Mike Hofer
@Mike: And when your code breaks, it will be more obvious what the original version of the query was supposed to do and how it worked. - Brian
110
[+4] [2008-09-23 13:44:15] community_owned

Allowing access to objects you own

Smell: long chains of .GetX().GetY().

Problem:

If you allow users to get access to the objects used by your class, either by making them public or via a get method then your just asking for trouble, someone could come along and do:

A a;
a.GetB().GetC().GetD().SetSize(43);

2 things are wrong with this.

  • Joe Bloggs can come along and suddenly change the size of D, in fact he can do almost anything he wants with D. This won't be a problem if you've taken that into consideration whilst writing A, but how many people check that kind of thing?

  • The users of your class can see and have access to how its implemented. If you want to change something, say implement C so that it uses a Q instead of D, then you'll break everyone's code that uses the class.

Solution: The fix depends on how your class will be used, but in both cases the first step is to remove the GetX().

If a user really does need to be able to call SetSize(43) then you should write a wrapper function in each of the classes that passes the new value down. Then if you choose to implement C so that it uses a Q instead of D then no one apart from C will have to know about it.

A a;
a->SetSize(43);

class A
{
    SetSize(int size){b.SetSize(size);}
};

etc.

If the user of the class shouldn't need to call SetSize then just don't implement a wrapper for it.

If you find that most of D's functions need to be pulled up to A then this may indicate that your design is starting to smell, see if there is a way to rewrite C and B so they don't rely directly on D.


You make a decent point here, but the vague representations hides it at first glance. It would be easier to understand if you included a more concrete example, rather than relying on ABC. - Guvante
I can think of one case where this cannot be avoided. Accessing an element nested in a deep schema, when bound to java classes, results in code that looks like the one you specified. - CMR
111
[+4] [2010-10-30 09:39:33] Arne Burmeister

Naming by Type

It is totally useless to name a variable, member or parameter by type. In former times this may had sense, but today a simple click on or hover over the name in the IDE shows the declaration.

Better to use the meaning of the value for the context.

Unfortunately some IDEs like Idea have the bad "functionality" of completing a name by writing the type. Never use this!


112
[+4] [2010-01-30 08:50:24] T.R.

Smell:
File1:

def myvariable;

File2:

def my_variable;

File3:

def myVariable;

Problem: Spaghetti inclusions (if it's not just poor variable naming). Someone is sidestepping redefinition compiler errors (in a statically typed language) or data loss (in a dynamically typed language) by using different variables for the same purpose in each file.

Solution: clean up the inclusion mess, narrow the scope of each instance of the variable (preferably through some form of encapsulation) or both.


113
[+4] [2010-03-12 22:22:07] Matthew Sposato

A ridiculous number of compiler warnings.

Even compilers dislike smelly code. I use Visual Studio with Resharper. Many of items posted in this thread would be warnings. i.e. unused variables, variables assigned a value that is never used, unused private methods, hiding inherited members etc.


114
[+4] [2010-08-14 07:17:52] fastcodejava

Too many big blocks of code.


115
[+4] [2011-03-10 19:07:27] Hannesh

Variable naming inside classes

Variable names are good enough when understood within the scope of the class.

I shouldn't have to write Shader.ShaderTextureSamplers[i].SampleTexture.TextureWidth, when Shader.Samplers[i].Texture.Width is just as readable.


It's kinda a matter of style. - the_drow
116
[+3] [2011-03-20 07:20:05] lzprgmr

Unnecessary if .. else block

Code like this should be better avoided:

if(map.find(key) != map.end())
    return true;
else
    return false;

Replace it with just one statement:

return map.find(key) != map.end();

117
[+3] [2010-11-17 17:31:21] Khainestar

Got a couple lately that defy logic and boy do they smell.

php mysql calls that use $_POST and $_GET directly in the sql.

function calls in PHP function($value=false); then there is comments in the function saying that this never worked right and they didn't know why.

mysql calls that have "or die(mysql_error());" on the end.

No abstraction layer, each database call goes through the motions of creating the database connection.

I could just go on and on about this code, but I have a rm -rf to run.


118
[+3] [2009-09-25 02:11:17] Jai

Multi function functions

If a function is doing more than one thing. This has been mentioned implicitly in other answers but it should get a mention on its own.

Example:

// if function altList = null function returns a list of X ids
// if altList is set the function returns a list of objects of type Y

Function getData(searchStr, altList=null)
{
    // common code
    some code;
    if(altList == null)
    {
         other code;
         return array(int);
    }
    else
    {
         more other code;
         return array(object y);
    }
}

This really should be three functions:

  • Public function that gets a list of ids
  • Public function that gets a list of object of type y
  • Private shared function contains any common code.

(1) realloc is a classic case of malloc overload...if the pointer is non-NULL and size is zero, realloc behaves as if free is called, likewise, when you pass in a NULL pointer and size is greater than zero, realloc behaves as if you called malloc... - t0mm13b
119
[+3] [2008-09-23 03:34:20] community_owned

Give your code a good bath using quality soap, then air out for a week, the smell will be gone.


120
[+3] [2008-09-24 19:08:21] Steve B.

loops with lots of exit points:

for (...)
{  
   if (... ) {
    continue;
   }
   else if ( .. )
   {
     if (... ){
       break;
      }
   }  
}

121
[+3] [2008-09-23 02:44:22] JohnMcG

Smell: In C++, an object is dynaimically created and assigned to a bare pointer.

Problem: These must be explicitly deleted in all paths out of the program, including exceptions.

Solution: Manage the object in a smart pointer object, including std::auto_ptr, or one from the Boost libraries.


122
[+3] [2008-10-07 13:16:09] rjarmstrong

Ignoring all the fundementals or any particular construct; if it feels wrong then you've just got a whiff of a smell. Why - well because you will need to use the code you've just made and that 'wrong' feeling will give you little confidence when using it. A second opinion may be required if you're having trouble refactoring.


123
[+3] [2008-09-22 21:47:06] community_owned

Too short code fragments or structured to death

If I see a lot of short source files, object definitions and methods with only one row real content, I feel this project was structured to death. More brackets then code lines is the first sign.

Dare to write complete (but small !) code snippets/methods/etc. without feeling the pressure to stamp out into unreadable particles.


124
[+3] [2008-09-22 13:20:07] Nazgul

Defensive coding

Code where there are a lot of null checks is a smell. If this is the case then there is some issue with your design.


I disagree completely. If you're writing a component that others will be using, there's nothing wrong with extra checks at every single point of execution. - Alex Weinstein
I agree with Alex, I do this myself on regular occasion when building components. Always assume the worst, especially if something really bad will happen if it gets through. - Rob Cooper
I think this can go both ways, and is still a good code smell. Defensive programming can be a virus. A good example of this is widespread null checking with lots of error handling code which could be better handled by a null object / null sub-class. - Wedge
It's fine checking parameters on input. But when you have a lot of internal privaqte functions doing the same checking, there may be a problem. - Ferruccio
You should check for null only if your method depends on the parameter being not null. Example: if i have a parameter P and I call P.Something() in the method, I should check that P is not null. However, if I only pass P to another method, then that method should be doing the checking. - Sklivvz
If you call P.Something() then the JVM will check for null and throw NPE. You only need to check there if an NPE doesn't suit you. - David Pierre
Java's NPEs don't suit me because they don't communicate what was null. When configuring a component, I check for null and throw my own NPE that identifies the argument that was null. After configuration I don't check for null at all. - user19113
I check for null at interface boundaries. It's the right place to check and you can't be assured that clients get everything right. For internal functions where parameters should not be null, I assert. - George V. Reilly
125
[+3] [2008-09-22 13:24:12] Vilmantas Baranauskas

Use of specific class instead of interface when declaring variable. Smells especially bad in combination with Java's collection framework.

Instead of

ArrayList list = new ArrayList();

use

List list = new ArrayList();

This is just a matter of style... I don't see wrong with this. - Alex Weinstein
ParaSoft's JTest recommends this too and I disagree. If you are the class instantiating the class, you are still coupled to the concrete class. Declaring the type as the interface type is only useful if the implementation is injected externally. - moffdub
Given that your example may have been misunderstood, the general principle you are alluding to is very relevant. It is not simply a matter of style. +1 - Luther Baker
See "Program to an interface, not an implementation", GOF. Even when the concrete class is instantiated in situ (not injected), you know that the rest of code will use only the methods that belong to the interface (in static languages, of course). This makes a possible future implementation switch having significantly less impact. - Dan
126
[+2] [2008-09-22 13:25:54] Vilmantas Baranauskas

When Map is used to hold unique elements. E.g.

Map map = new HashMap();
map.put(name, name);

Use Set instead:

Set set = new HashSet();
set.add(name);

Funny. HashSet internally uses a HashMap ;) - shyam
(1) I wonder why so many people downvote this one, please explain. Implementation defaults such as the first comment here are a bad reason for downvoting - i'd rather downvote that comment for supporting implemenation-dependent programming (which is a big no-no) - Thomas Tempelmann
Yes! A map is for mapping. When someone uses a map as a set, often they or someone else later decides to use the map as an alternative to keeping data encapsulated with the elements of the set. Later, someone else, not realizing this information is in the map, adds it to the encapsulation for the elements of the set. - T.R.
127
[+2] [2008-09-22 13:21:40] Vilmantas Baranauskas

Use of Vector and Hashtable in Java. This usually means that programmer knows nothing about collection framework and newer versions. Use List and Map interfaces instead and any implementation you like.


(2) or that the code is older than you think it is - Steven A. Lowe
128
[+2] [2008-09-22 17:05:44] JB King

"Orphaned" functions that are all put into a file without any organization/order. For example, an 8000 line ASP file of 100 functions that look like spaghetti code or the beginnings of it. There may be more than one smell to this, but when I come across it, there is some pain in having to maintain legacy applications that have this "feature" ;)

The fix for this is to create some classes that group the functions and determine which are useful functions that go into a class and which should be refactored into something else.


129
[+2] [2008-09-22 12:20:54] aku

switch statement with number of cases <= 2

Usually happens when using boolean-like enums ( enum { Off, On } ).

Consider using if-else statements


I would NOT suggest you would do this at all. An enum should be switched, and defaulted to error messages. This is because if the API which provides the enum changes (adds) another enumeration value, like an error enum (bad example, exceptions are usef for this but please bear with me)...<continues> - Statement
<continued>... then the program might misbehave. Example: enum Threat {High, Low, None}; switch(theEnum) { /*no default, or default accepts none (ugly hack)*/}. Then if the enum changes to enum Threat {Dangerous, High, Low, None};, you're in for a nasty surprise. Handle unexpected enums!!! - Statement
Statement, I'm talking about boolean-like enums solely. - aku
130
[+2] [2008-09-22 12:38:01] aku

.NET specific: catching System.Exception exception

Usually means that code author deserves to be beaten hard with baseball bat.

Don't even try to do it unless you have a very good reason. Did I mention baseball bat, no?


Perhaps clarify why authors should not catch them. - Statement
if you are expecting a specific type of exception, explicity catch and handle it. if you catch the Base Exception and handle it (or horrors eat it up), unexpected but real exceptions would be silently killed. - Gishu
Catching a specific exception type I agree with - but what about a bucket to gracefully handle any unhandled errors? - Raithlin
The very good reason is a top level handler. However you should if using one of these quit in the case of an unexpected exception after showing the usual "omg something is wrong!" to the user. - Quibblesome
I think Aku is experienced enough to know there are reasons for catchin Exception, (such as reporting, catching to return default values etc). I think he was more suggesting that some people just catch it to try and do something with it, when there is very little you can do with the base. - Rob Cooper
Surely you should catch System.Exception in every event handler or entry point to your application. Otherwise any exception scenarios you've not considered will crash your application. - Richard Ev
(1) Catching System.Exception at application entry point level is in my opinion the way to go and nowhere else - Matthias Hryniszak
131
[+2] [2008-10-01 10:35:48] tpower

Protected Member Variables

The only place I have ever come across these is when I find the source of bug.

When these are used in code there is normally no distinction between a private member and a protected member. A developer could easily change its value thinking they can see all uses of it in the current class so it wouldn't cause a problem.

You can replace protected with private and add some protected accessors functions. Developers are used to calling base class functions so it would be easier to understand what is happening.


protected is ok when the member is final for me - Arne Burmeister
132
[+2] [2009-03-04 06:51:08] thomasrutter

A comment containing the word TODO

{
  //TODO clean this up a bit
  if (klwgh || jkhdfgdf || ksdfjghdk || bit << 8 == askfhsdkl)
    if (klusdhfg)
      return 1 + blah;
}

(5) Wouldn't these be useful with an IDE (i.e. NetBeans) that turns all of these comments into a todo list? - Soldier.moth
(1) +1 to soldier.moth - these are great for IDEs that put them in a todo list - obelix
They are definitely useful in IDEs. - dstibbe
I like to use TODOs and like you three I also find it helpful when IDEs treat them as special, but they are still code smells. A code smell alerts you to the possibility of bad code nearby, and TODOs often accompany code that's not quite working yet, or where the programmer hastily threw something together with the intention of returning to it later. - thomasrutter
XCode does show TODO comments in a drop down list along with the message list. (Messages are Objective-C functions/methods.) I assume @thomasrutter is under the impression that comments are solely for the purpose of explaining how code works to potential readers. Sorry, I'm going to disagree on this. The "Not working code" may be a codes smell, but comments are comments - I can write what I want there. Wordpress writes poetry in comments at the end of their files. - Moshe
133
[+2] [2008-12-07 09:19:21] Norman Ramsey

In OO languages, private members exposed through getter and setter methods. I'm frightened how many times I've seen students write code with members that are supposedly private but can be read or written through public getter and setter methods. Such classes 'expose the rep' just as thoroughly as public members, but with a lot more cognitive overhead.

Remove the smell by figuring out what secret the module or class is supposed to hide and reducing the number of methods accordingly.


(1) The whole point of using setter methods for private instance variables is so that you can apply constraints. If you had an int that was public any valid integer would be allowed. If you made it private and used a setter method then you can ensure that only integers in the range that you want are allowed to be stored. You'll probably need to get the stored integer as well so a getter method also makes sense. - Simon
@Simon Agreed. But I've seen way too much code that misses the whole point. It just leaves those supposedly "private" variables hanging out naked and exposed! - Norman Ramsey
Classes should almost never have public member variables, only private ones. Even protected members are better off as private member variables and protected get/set properties around them. When member variables are public and directly accessible, they'll be all over the code; when you need to suddenly make a change to them, you'll have to update the code everywhere. - xxbbcc
134
[+2] [2008-09-23 03:08:10] Bazman

Huge methods/functions. This is always a sure sign of impending failure.

Huge methods should be refactored into smaller methods and functions, with more generic uses.

Potential Solution:

I've found that a really great way to avoid this is to make sure you are thinking about and writing unit tests as you go.

You find whenever a method/function gets too large, it becomes very difficult to work out how to unit test it well, and very clear on when and how to break it up.

This is also a good approach to refactoring an existing huge function.


Ive been a professional programmer for 14 years. Long methods is - BY FAR - the most common smell I've seen for code that sucks. It is very, very consistent. How can this not be at the top, I have no idea - Mahol25
135
[+2] [2008-09-23 03:32:45] Ferruccio

Weird constructs designed to get around best practice

e.g.

do
{
    ...
    if (...)
        break;
    ...
} while (false);

That's still a goto, even in disguise.


-1 since you're ignoring the original question's rules, i.e. explain how to fix that. I, e.g., wonder you'd make your example into something better. - Thomas Tempelmann
136
[+2] [2008-09-24 16:04:30] James A. Rosen

The reek [1] project will help automatically check for code smells in Ruby.

[1] http://github.com/kevinrutherford/reek/

137
[+2] [2008-09-23 06:43:14] deadbug

Dynamically generated SQL smells. It has its limited uses, but in general makes me nauseous.

A very simple example below ...

Don't:

DECLARE @sql VARCHAR(MAX), @somefilter VARCHAR(100)

SET @sql = 'SELECT * FROM Table WHERE Column = ''' + @somefilter + ''''

EXEC(@sql)

Do:

SELECT * FROM Table WHERE Column = @somefilter

There are situations where dynamically generated SQL is helpful, for instance when there are a large number of filters, some of which may not be included (Say a search page), by not emitting those filters to the final SQL, you ease the burden on the server, improving performance. - Guvante
I come across this too often ... - Nippysaurus
138
[+2] [2010-01-10 01:22:03] Mawg

Lack of ASSERT()

New guys never seem to use them. I prefer to break my code as early as possible. At the very least, ASSERT() all input parameters (and some return values from functions which you call).

MY own assert is a macro which calls the standard #ASSERT() if I am testing (#ifdef TESTING) otherwise, in a production system it writes something to the system log file.


139
[+2] [2010-01-10 01:28:18] Mawg

Switch cases that fall through

switch (value)
{
  case 1:
  case 2:
  case 3: // some - usually very long - block of code
          break;
  case 4: ...etc
}

These are always worth look at closely, as they invite bugs. And they can generally be worked around by putting the common code into a function. Look for a break after every case;


This construct does have some good uses, like improving readability: the values 1, 2, and 3 are handled just the same here. At a different place, they are handled differently. - Still, checking this is good, because an unintentional fallthrough marks a bug. It's neither good nor bad by itself, but it's something you should pay attention on. - foo
140
[+2] [2009-08-28 17:59:32] obelix

Functions in C++ that take pointers as arguments and then check for NULL on the first line. Pass by reference or const-reference. Makes the contract so much clearer.


(1) Would you prefer an access violation? - Craig Young
141
[+1] [2008-09-23 12:04:48] user17224

My list - http://computinglife.wordpress.com/2008/06/03/what-really-is-bad-code-levels-of-bad-ness/

Excerpts -

  1. Does not catch errors / ignore return values
  2. Memory leaks / Exceptions
  3. No validations (on inputs / parameters / strings)
  4. Too big a function / class
  5. Globals
  6. Pointy code (http://www.codinghorror.com/blog/archives/000486.html)
  7. Too many variables
  8. No indentation
  9. Weak naming
  10. Extremely big individual lines

Care to excerpt/give a summary? - Rob Cooper
142
[+1] [2008-09-22 22:31:00] HLGEM

From a database perspective (SQL Server to be specific) More than two layers of nested ifs - very easy to have a logic error Dynamic SQl in a stored proc - make sure that you aren't open to sql-injection attacks Cursors - is there a set-based solution? More than 5 joins especially when no columns from some of the joined tables are in the result set (do we perhaps need to denormalize for performance?) Use of subqueries instead of derived tables Over use of user-defined functions use of Select *


143
[+1] [2008-09-22 23:11:56] Karthik Hariharan

everyone is pointing out specific things that bother them and that they consider a "code smell".

I think after a while, once you get the hang of it, you sort of develop a "sixth sense" about what is wrong with a piece of code. You may not be able to immediately pinpoint how to refactor it or make it cleaner, but you know something is wrong. This will drive you to find a better pattern/solution for it.

Understanding all the examples others have posted is a good start for developing this sense.


144
[+1] [2008-09-25 08:54:29] Marcin

Any code that is repeated, or any instance when a variable is assigned more than once.

Both are appropriate in certain circumstances, and given the constraints of the environment, but still.


145
[+1] [2008-09-26 13:34:05] Pablo

Rediculous comments:

catch
{
/*YES- THIS IS MEANT TO BE HERE!  THINK ABOUT IT*/
}

I would agree. Comments shouldn't say 'think about it' they should say why it should be there. - TokenMacGuy
146
[+1] [2008-10-16 13:33:44] James McMahon

Apologies if this was already mentioned, but I just looked through the answers and didn't see this mentioned.

Code Coupling can make maintenance a nightmare. If you have display code directly tied into with other logic, making anything but routine maintenance will be all but impossible.

When you are designing your display code, think long and hard about your design and try to keep the design part separate from the rest of your app.


147
[+1] [2008-10-17 15:03:26] onedaywhen

Writing procedural code against a DBMS

Smell: looping through an ordered resultset using a cursor in SQL (or walking an ordered recordset in middleware, etc) aggregating values, setting values based on other values, etc.

Possible problem: the coder hasn't looked for a set based solution. Put another way, they haven't yet had the epiphany that SQL is a declarative language and a SQL DML statement is more like a spec than a piece of code e.g. when I write...

UPDATE MyTable 
   SET my_column = 100
  WHERE my_column > 100;

...I'm telling the DBMS what to do rather than how to do it.


Actually, this swings both ways: working with a resultset as an opaque array that you can hand around also results in smelly code. Treat a resultset as a generator. - staticsan
148
[+1] [2008-10-22 20:16:22] Kon

A large number of Ifs or Cases usually begs for creation of new classes that extend some base class.


-1 for duplicated example - Matthias Hryniszak
149
[+1] [2008-09-22 14:48:37] Sara Chipps

When you are doing the same thing (manipulating an object, doing similar SQL calls, processing data, changing a control) more than once in more than one place it's time to refactor. That gives way to smelly redundant code.


150
[0] [2008-11-05 03:05:51] Peter

Second-guessing assertions.

Ran across this recently:

assert(vector.size() == 1);
for(int i = 0; i < vector.size(); ++i) {
    do_something(vector[i]);
}

If you're asserting that there's only one item in the vector, you don't need the loop:

assert(vector.size() == 1);
do_something(vector.front());

I don't want to go into lots of boring detail; there was a good reason for having the vector for other cases, but in this branch of the code it should have always had size 1.

Obviously it's not a hard and fast rule, but to me it increases the complexity of the code (introducing a loop, another level of indentation) when you're saying that you don't ever expect to need it.


My own assertion "framework" lets me choose to ignore the assertion and let the code continue after the assertion, even in released products. This sometimes allows a user to work around a wrongful assertion, and still perform some action, i.e. save his work. Above "bad" code would be good then. - Thomas Tempelmann
It's not really about the assertion; it's about the intent of the method. The assertion clearly indicates that the intent of the method is that it will operate on a single element vector. Therefore any code that caters for the possibility of more or less elements is a redundant waste that reduces maintainability. - Craig Young
151
[0] [2008-11-16 14:51:54] Pride Fallon

A quick search suggests that this post is the first identification of the code smell "Intelliscents":

Extravagantly roundabout code bespeaking the typist's lack of familiarity with the classes and established idioms of some .net namespace, and his/her reliance on Intellisense to solve a problem at hand.

And my code is redolent with (of?) it.


152
[0] [2008-12-07 09:16:51] Norman Ramsey

Structure or class with a dozen or more member fields. There is probably not a coherent abstraction here. To remove the smell, break the structure or class into pieces. A good source of ideas is Raymie Stata's dissertation. [1].

[1] http://publications.csail.mit.edu/lcs/pubs/pdf/MIT-LCS-TR-711.pdf

153
[0] [2011-03-29 10:17:34] cHao

eval('some PHP/JS/Perl code')

Common to most scripting languages, and useful occasionally. But nearly every case i've seen in the real world was an ugly, bug-ridden example of arbitrary code injection just waiting to happen. If you see code that's using it, it's almost certainly doing something it shouldn't need to be doing. (The sole exception that springs to mind is dynamically generated classes -- and even then, i'd at least look at other options first.)

The fix:

Don't execute strings.

The code inside your evaled string can almost always be converted into real, executable code. If it can't, then question your intentions -- executing code from a database (for example) is almost always a horrible idea.


154
[-1] [2008-11-05 18:01:13] shyam

Lifecycle methods in classes

class Life {
  private boolean initialized = false;
  public void init() {
    try {
    // ...
      initialized = true;
    } catch (XXXException e) {
      // ...
    }
  }
  public void doSomething() {
    if ( !initialized ) {
      throw XXLException(...);
      // instead call init() and continue
    }
    // ...
  }
}

-1 for not following the question's rules - you're supposed to offer a solution (i.e. a fix) to the problem. - Thomas Tempelmann
Looks like a Singleton in disguise - Sam Miller
@Thomas: Looks to me as if the solution "instead call init() and continue" was provided... it's just not that obvious. - Craig Young
Yeah, well observed (better than me, I admit). Still, badly explained, I think. - Thomas Tempelmann
155
[-1] [2008-09-27 11:14:57] Shane MacLaughlin

smell: Unnecessary recursion

why: You've guessed it --> risk of stack overflow (had to get that one in)

solutions:

1) If you can, rewrite the recursive routine as an iterative routine, for example using divide and conquer techniques.

2) If not, examine the stack frame usage and try to minimise, for example by changing from breadth first to depth first analysis on a tree.


tail recusrion will take care of this. Some languages/compilers will make the stack issues o away. - Tim
(3) The joke is good. Removing recursion is not necessarily good. - user51568
156
[-1] [2008-09-22 17:15:54] Mariano

Cyclomatic complexity [1] > 25

[1] http://en.wikipedia.org/wiki/Cyclomatic_complexity

-1 for ignoring the rules of the initial question writer ;) - Thomas Tempelmann
-1 for the same reason as comment before - Matthias Hryniszak
-1 for being extremely lazy in the answer. - Craig Young
+1 for bringing up the quite important point. :D - Rekin
157
[-3] [2008-09-22 21:19:46] AlanR

Here's a somewhat obvious one, but I'd had to put up w/ it a few times...

You check out code from source control.... and it doesn't compile. Drives me crazy.


158
[-3] [2008-09-22 12:23:03] aku

enums behaving like booleans

Sample:

enum SwitchState { On, Off }
if (x == SwitchSteate.On) ...

Consider using boolean variables instead of enums in such cases.

bool isTurnedOn = ...
if (isTurnedOn) ...

This is not a smell. sometimes this is actually good practice when you expect that the variable you're using now may have more than two values in the future. obviously not in the case of On,Off though. - shoosh
(1) There is another code smell showing the opposite case, specifically when the boolean is a parameter to another method call, especially if there are multiple values. - Guvante
(4) @shoosh I agree with all but the last part of your comment. Start with on and off, and later you might add undefined or unknown. Gender is an interesting example of something similar. At one company we originally assumed gender was only male xor female but we live in a more complex world than that... - Ryan
Actually, I think the inverse is a smell. The point is, you want your code to be easy to read. Sometimes you see True and ask yourself "True what?!??". True and False are far too generic to be easily and immediately understood in all contexts. As such, you can greatly improve readability by sometimes replacing a standard boolean with an enum... even if for only 2 values!! - Craig Young
(1) An enum is much more descriptive than true or false. If used as a method parameter, enums are much easier to read than true/false! - Arne Burmeister
159
[-3] [2009-08-08 20:25:15] Matthias Hryniszak

Methods returning constants


  • The Smell

You see a function that always returns the same thing.

  • Additional observations

There's lots of classes implementing those methods just for other parts of the code to get the right value. Sometimes even constructors are being defined just to call the base/super/inherited constructor to pass on those constant parameters.

  • The solution

Change the method to return a value from a private field initialized in constructor and use the Factory pattern to construct the objects (for example using DI as a mega Factory pattern implementation in this case). This makes the inheritance structure in most cases obsolete.


(3) That is not a guaranteed code smell. - Paul Nathan
160
[-5] [2008-09-24 20:12:16] Remi Meier

Break and Continue are the same as GoTo

One should be able to look at the head or tail of a loop to immediately be able to tell under what conditions it terminates.

What to do: Use descriptive (boolean) variables instead of a direct break/continue and test them in the appropriate place (head/tail) of the loop.


While not downvoting, I do disagree. Break and Continue are definitely not the same as GoTo, no way. It is (IMHO) a perfectly valid way to control code flow, as its scope (in contrast with GoTo) is very clearly restricted and does not (if used propertly) affect readibility and maintainability. - petr k.
agree with petr k: goto almost always means either break or continue. In the few cases it's not, it's either a carefully designed state machine or actually, legitimately bad code. - TokenMacGuy
(2) @petr k: Actually, I disagree slightly. Break and Continue can affect readability and maintainability...... It can greatly improve it! :D - Craig Young
(2) break and continue only allow you to jump to specific locations (either to the end of the loop or beginning of the loop) whereas goto allows you to jump anywhere. They are clearly not the same. - gablin
161
[-7] [2008-09-22 15:16:33] Eric Burnett

If statement without corresponding else

Not all if statements need an else, but when one isn't present you should check if it really should be there. If one program state needs special handling, often the opposite state does too.

Example where an else may be necessary:

if (teacher != null) {
    addStudents(teacher, period, students);
}

// Else?
else {
    // Why is `teacher` null? Is this an error state?
    // Will the students be 'lost' for this period?
}

Solutions:

  • Add an else block with the correct logic
  • Change the if check to an assert
  • If no else is necessary, but this isn't immediately apparent, add an empty else block:

    } else {
        // Nothing to do here
    }
    

(2) I wouldn't view this as a smell, I often use exactly this for Guard Conditions en.wikipedia.org/wiki/Guard_(computing) - Rob Cooper
(1) Totally agree with Rob Cooper. Guard conditions are great for keeping things neat before the 'meat of the code' comes. - Camilo Díaz
(1) I didn't mean to imply they can't be used that way. Code smells are indicative of something that may be wrong, not necessarily that something is wrong. There are plenty of uses for if-no-else statements, but there are also situations where the lack of an else is often a bug. - Eric Burnett
I agree with you Eric. It doesn't have to be wrong, but it could be. A missing else could mean that the else block should throw an exception. For example, an if block in a parser that checks all the operators. Here an else block can check for unknown operators. - Peter Stuifzand
Perhaps half of all if statements in well-written code don't have elses. - Jim Ferrans
I've seen one hell of a lot of bugs purely as a result of the else condition having been ignored (same applies to case statements). However, that said; when considering how often if without else is perfectly valid, I'm kind of in two-minds as to the feasibility of using it as a code smell. Either way, Guard Conditions should be excluded from this evaluation. - Craig Young
162