I attended a software craftsmanship event a couple of weeks ago and one of the comments made was "I'm sure we all recognize bad code when we see it" and everyone nodded sagely without further discussion.

This sort of thing always worries me as there's that truism that everyone thinks they're an above average driver. Although I think I can recognize bad code I'd love to learn more about what other people consider to be code smells as it's rarely discussed in detail on people's blogs and only in a handful of books. In particular I think it'd be interesting to hear about anything that's a code smell in one language but not another.

I'll start off with an easy one:

Code in source control that has a high proportion of commented out code - why is it there? was it meant to be deleted? is it a half finished piece of work? maybe it shouldn't have been commented out and was only done when someone was testing something out? Personally I find this sort of thing really annoying even if it's just the odd line here and there, but when you see large blocks interspersed with the rest of the code it's totally unacceptable. It's also usually an indication that the rest of the code is likely to be of dubious quality as well.

  • 61
    I sometimes encounter people who comment out code, checkin and say "I might need it again in the future - if I remove it now I'll lose it". I have to counter with "Er, ...but that's what source control is for".
    – talonx
    Commented Oct 25, 2010 at 10:31
  • 6
    Sometimes, particularly when optimizing, it's handy to leave the old code as a comment, so you know what the obscure optimized code is replacing. Think of leaving a 3 line swap with temp in place when replacing it with a one line bit twiddling swap. (Although, I see no need to use a one line swap -- EVER, unless programme size is of critical importance.) Commented Oct 25, 2010 at 17:02
  • 4
    I'm maintaining/cleaning up code written by one of our engineers, who coded some useful things but admits he isn't a programmers. As I consolidate stuff I'll comment out his old code and then later we go over the changes and I show him how I replaced his with something smaller/more efficient/easier to understand. Afterwards I strip out those blocks then I check it in. Having the old code there has benefits because he sees how things can be done more simply and I can remember why I changed things when we're talking. Commented Nov 4, 2010 at 7:19
  • 8
    I leave stuff that "might be used" in for 1 commit, then if things don't break down or a need is not found, it gets removed on the next commit. Commented Nov 16, 2010 at 22:35
  • 24
    Hmm. printf("%c", 7) typically rings alarm bells for me. ;)
    – user2348
    Commented Dec 13, 2010 at 15:03

60 Answers 60


C++ code with explicit delete statements (unless I'm looking at the innards of a smart pointer implementation). 'delete' is the 'goto' of memory management IMHO.

Closely related to this, code like:

// Caller must take ownership
Thing* Foo::Bar()

(And count yourself lucky if there's the comment!). It's not like smart pointers are rocket science. std::auto_ptr is made for this sort of thing (documenting and enforcing the intent expressed in the comment) and been part of the standard for ages now.

Together these scream unloved legacy code, or maintainers with a mindset stuck somewhere in the early 90s.

  • Unless it's an embedded system...
    – Ant
    Commented Dec 18, 2010 at 10:27
  • Would the embedded guys go anywhere near C++ at all though ? malloc/free in C are a different matter.
    – timday
    Commented Dec 18, 2010 at 10:43
  • Huh, I honestly never knew about std::auto_ptr. C++ used to be my language of choice, but for the past decade or so I've only been using it for homework (yes I've been in school for that long). Learned something new. Commented Dec 21, 2010 at 7:59
  • @Rei: If you like auto_ptr you'll love boost.org/doc/libs/1_45_0/libs/smart_ptr/smart_ptr.htm and maybe boost.org/doc/libs/1_45_0/libs/ptr_container/doc/… too.
    – timday
    Commented Dec 21, 2010 at 23:37
  • 1
    Also, in C++11 auto_ptr becomes deprecated.
    – Lstor
    Commented Aug 4, 2011 at 7:57

When there are no comments or documentation whatsoever of what the code does or is supposed to do (i.e. "the code is the documentation").

Methods or variables with a number as suffix (e.g. Login2()).


Having to maintain Classic ASP applications is sadly a necessesity for most ASP.NET developers, but opening up a common include file and seeing that on the first line is soul-destroying.

  • 1
    That brings back horrible, horrible memories.
    – E.Z. Hart
    Commented Mar 4, 2011 at 19:10
  • Oh god. I once had to maintain (by myself) a HUGE Classic ASP app that relied on On Error Resume Next to actually run properly because the "programmer" just tried the brute force code approach. I mean variables used before they are instantiated, calls to functions that don't exist, etc. I had to leave it in because it would have taken months (no joke!) to fix all the pages that broke when I removed it. I still have nightmares =( Commented Apr 15, 2011 at 20:09
  • +1, although this is compounded by bad language design. Why is canceling a print dialog box an error?!
    – asthasr
    Commented Sep 18, 2011 at 3:41

From my Java-centric perspective:

  • Non-standard coding style.
  • Non-private variables.
  • Missing final on fields.
  • Pointless or overuse of inheritance.
  • Huge classes or blocks of code.
  • Too many comments (they're probably just wishful thinking anyway).
  • Unstructured logging.
  • Getters and setters (interfaces should be about behaviour).
  • Duplicated data.
  • Strange dependencies.
  • Statics, including thread-globals.
  • For multi-threaded code, parts of the same class that are expected to be run in different threads (notably GUI code).
  • Dead code.
  • String manipulation mixed in with other code.
  • Generally mixing layers (higher level stuff, combined with iterating over an array of primitives or thread-handling, say).
  • Any use of reflection.
  • catch blocks without useful code (bad: comments, printf, logging or just empty).
  • Putting every local variable in the first feew lines of the method block. Especially in conjunction with long methods.
  • Using boolean variables to break out of loops / skip iterations instead of just using break / continue
  • Some languages force you to put the local variables on the top of the fnuction. For example X++ (Dynamics AX) requires you to put the in top and put a ; as block-closer. And for the second one there are also situations which require such way of working. For example when working with nested loops, if you want to jump out of 2 or 3 loops at a time you might need to have some flag to indicate whether the loop was terminated in a normal fashion or was terminated by a break statement and wish to take action based upon the type of loop quiting in the previous loop.
    – Gertjan
    Commented Mar 24, 2011 at 11:10
  • 1
    @Gertjan: "Some languages force you to put the local variables on the top of the [function]." Like, you know, ANSI Standard C. Commented Jun 7, 2011 at 1:58

Code that cannot ever, logically enter the execution path.

var product = repository.FindProduct(id);

log.Info("Found product " + product.Name);

if (product != null)
    // This will always happen
    // **This won't ever happen**


if (messages.Count > 0)
    // Do stuff with messages
else if (messages.Count == 1)
    // **I hope this code isn't important...**
//do something

Our legacy VB6 code, you can open any Module or form code page and find a screen full of Public or Global #&@! variables declared at the top, being referenced from all over the @&!!(*!# program. ARGH!!!!

(Whew, I had to get that out :-) )


As for magic numbers: they are bad if they are used in different places and changing it requires you to syncrhonise it in several places. But one number in one place isn't any worse than having one constant to denote one number that is still used in one place.

Furthermore, constants might not have much place in your application. In many database apps, those things should be stored in the database as per app or per user settings. And to complete their implementation they involve this setting and a place in the ui and a note in the end user documentation... all of which is kind-a overengineering and a waste of resources if everyone is perfectly happy when the number is 5 (and 5 it is.)

I think you can leave numbers and strings in place until there is a need to use this number outside that place. Then it is time to refactor things to a more flexible design.

As for strings... I know the arguments, but this is one more place there is no point in doing a one-string-to-one-constant conversion. Especially if the strings in place are coming from a constant implementation anyway (for example, imports that are generated from an outside application, and have a status-string that is short and recognisable, just like 'Overdue'.) There just isn't much point in converting the 'Overdue' to STATUS_OVERDUE when it is used in only one place anyway.

I am very much for not adding complexity if it doens't actually create needed benefits on flexibility, reuse or error checking. When you need the flexibility, code it right (the refactor thingy). But if it isn't needed...


Something like this


This smells like bio-hazard. This much member referencing is never ever a good sign.And yes this is a typical expression in the code I am working with.


Using a hidden object in the user interface (eg, a textbox) to store a value rather than define a properly-scoped and -typed variable.

  • This is what put me off HTML forms for so many years. Commented Jun 7, 2011 at 2:03

Code that shows that the programmer never adapted, years ago, to Java 5:

  • Use of Iterators instead of “for each”
  • Not using generics in collections, and casting retrieved objects to the expected type
  • Using ancient classes like Vector and Hashtable

Not knowing the modern multithreaded ways.

  • And APIs that force you to do it that way… Commented Aug 4, 2011 at 10:35

Anytime I read the following:

//Don't put in negative numbers or it will crash the program.

Or something similar. If that's the case then put in an assert! Let the debugger know that during run-time you don't want those values and make sure the code spells out the contract with the caller.

  • 4
    Or if your langauge is powerful enough, define a type that does not allow negative values? Commented Oct 25, 2010 at 13:35

anything with something like this

// TODO: anything could be here!

Edit: I should qualify that I meant this in production code. But even in code committed to source control this still isn't good. But, that could be a personal thing in that I like to finish the day off having tied off all my loose ends :)

Edit 2: I should further qualify that I meant when I see this in established code. Like something that's a number of years old and I am fixing a bug it. I see a TODO and that's when the alarm bells start ringing. TODO's (to me) imply that the code was never finished for some reason.

  • 4
    I would only agree in context, in production code yes but during development cycle I will often toss "todos" in places where I have an idea but it is to elaborate or involved to implement based on what I am currently working on.
    – Chris
    Commented Oct 24, 2010 at 21:54
  • I have to admit I'm guilty of TODOitis during development too.
    – FinnNk
    Commented Oct 24, 2010 at 22:00
  • 9
    You should use TODO comments for one thing and one thing only: leaving a note for yourself while you work on something related elsewhere. Only when grep says that there are no more TODOs are you allowed to commit.
    – Jon Purdy
    Commented Oct 24, 2010 at 22:07
  • 1
    @Jon Purdy I strongly disagree. When you're designing a project from scratch, it's easier to put down your ideas on a new project under development it may take 5 separate individual components to get a method to work. It's much easier to drop the TODOs in place and mark them off one-by-one. Some IDEs like VisualStudio fetch TODOs in the code and create a list similar to the debugging error list. Commented Nov 19, 2010 at 22:17
  • 1
    @Jon it's much better to commit early and commit often because it'll be easier to backtrack if something is broken. I typically commit after every new feature/module is completed and the project builds. A TODO could be a more time-consuming note like, "revise the comments for this module", in which case I'd come back to it when I switch mental context to commenting for documentation. IMHO, a rich revision history is much more important than clean code. It will save you if something breaks. Commented Nov 19, 2010 at 22:22

For SQL:

The first big indicator is the use of implied joins.

Next is the use of a left join on tableB combined with a WHERE clause like:

WHERE TableB.myField = 'test'

If you don't know that will give incorrect results then I can't trust that any query you write will give correct results.


Tightly coupled code. Especially when you see lots of things hardcoded (names of network printers, ip addresses, etc.) in the middle of code. These should be in a configuration file or even constants, but the following is just going to cause problems eventually:

if (host_ip == ''){
   printer = '';
} else
  printer = 'prntrBob';

Some day, Bob is going to quit and his printer will be renamed. Some day the printer will get a new IP address. Some day will want to print on Bob's printer.

my favorite mantra: always write code like a homicidal psychopath who knows where you live will have to maintain it!


This type of code:

        if (pflayerdef.DefinitionExpression == "NM_FIELD = '  '" || One.Two.nmEA == "" || 
            One.Two.nmEA == " " || One.Two.nmEA == null ||
            One.Two.nmEA == "  ")
            MessageBox.Show("BAD CODE");

This is from a real live production codebase!


When the code looks like a mess: Comments with "todo" and "note to self" and lame jokes. Code that was obviously used at some point solely for debugging purposes, but was then not removed. Variables or functions with names that suggest that the writer did not consider that anyone would read it later. Often these names will be very long and unwieldy.

Also: If the code lacks rhythm. Functions of wildly divergent length. Functions that do not adhere to the same naming schemes.

Slightly related: It always makes me nervous if a programmer has slobby handwriting. Or if they are bad writers or bad communicators.

  • 1
    I was going to give you +1 but some of your comments are a bit of track. Best coder I ever met was also the most introverted person I ever met, took me 2 years just to get him speaking up for himself. And who cares about handwriting if you can type at 100+ wpm?? :D Commented Oct 28, 2010 at 22:37
  • Re. handwriting: I don't care if someone's handwriting isn't beautiful, but I do care if it's slobby and looks as if they couldn't even read it themselves. I'm really just talking from my own limited experiences, and yours may be different. Commented Dec 15, 2010 at 9:55

Anything that violates principles that are important. For instance, a few anti-patterns have been suggested (magic number -- see http://en.wikipedia.org/wiki/Anti-pattern). Anti-patterns are called that because they cause problems (also already mentioned - fragility, maintenance nightmares, etc). Also, watch out for violation of SOLID principles - see http://en.wikipedia.org/wiki/Solid_(object-oriented_design) Also, Code that doesn't consider separation of tiers (data access things inside the UI, etc). Having coding standards and code reviews help to combat this.


I once worked on a project where the contractor had tyepdef'ed every standard datatype from int to string including pointers to obscure names. His approach made understanding the code really difficult. Another style that warns me is premature flexibility; a product I once worked upon had the complete code in DLLs that were loaded in no predictable order. All this to accommodate future evolution. A few DLLs used thread wrappers for portability. It was a a nightmare to debug the program or predict the flow by reading the source code. Definitions were scattered across header files. It was no surprise that the code did not survive beyond second version.


Sometimes I see parts of a method that given all possible inputs, it would still NEVER run, so it shouldn't be there confusing people in the first place. An example would be...
If the method can only be called inside the context of an Admin superuser and I see something checking if the request user is not an Admin superuser... :/


Peephole optimizations on code that could be optimized with better structure, e.g. linear searches implemented in inline assembly when a binary search in plain C/C++/Java/C# would be appropriate (and faster).

Either the person is missing some fundamental concepts, or has no sense of priorities. The latter is much more worrying.


The use of the synchronized keyword in Java.

Not that there is anything wrong with using synchronized when it is used right. But in the code I work with, it seems that every time someone tries to write threadsafe code, they get it wrong. So I know that if I see this keyword, I have to be extra carefull about the reste of the code ...

  • $data - It's like advertising "Physical objects, now at a ridiculously low 100 per 5!"
  • TODO items in code - Use a bug/ticket/issue tracker so people know what's needed on a product level rather than a file level.
  • Work log in code - That's what version control is for.
  • I think $data is a fine variable name in cases where you're writing a generic data processing/statistical function and one parameter is the dataset and the rest are configuration, etc.
    – dsimcha
    Commented Mar 7, 2011 at 0:42
  • @dsimcha: In such cases you can easily replace $data with something more usable, like $samples or $magnitudes.
    – l0b0
    Commented Mar 7, 2011 at 10:12

Most of these are from Java experience:

  • String typing. Just... no.
  • Typecasting will often point to a code smell in modern Java.
  • The Pokemon Exception anti-pattern (when you gotta catch 'em all).
  • Cargo-cult attempts at functional programming where it isn't appropriate.
  • Not using a functional-like construct (Callable, Function etc) where it would be appropriate.
  • Failures to take advantage of polymorphism.

Disgruntled comments demonstrating lack of restraint:


Either they're ill-tempered or they're not experienced enough to have learned that setbacks are inevitable in programming.

I don't want to work with people like that.


@FinnNk, I agree with you about commented out code. What really bugs me is stuff like this:

for (var i = 0; i < num; i++) {

or anything that was there for testing or debugging, and was commented out and then committed. If it is only occasional, it is not a big deal, but if it is everywhere, it clutters up the code and makes it hard to see what is going on.


This is a somewhat minor symptom compared to things others have listed, but I'm a Python programmer and I often see this in our codebase:


Which usually signals to me that the programmer didn't really know (or care) why do_something might fail (or what it does) -- he just just kept "fiddling" until the code didn't crash anymore.

For those coming from a more Java-esque background, that block is the Python equivalent of

try {
} catch (Exception e) {
    // Don't do anything. Don't even log the error.

The thing is, Python uses exceptions for "non-exceptional" code, such as keyboard interrupts or breaking out of a for loop.


getters all over the place make me freak out.

and a special thing: getters delegating to other getters.

this is bad because it means the person who wrote that doesn't understand the basic of object-oriented, which is encapsulation, which means where the data is, the methods that act on that data should be.

delegation is for one method not all the getters. the principle is "tell, dont's ask"; tell one thing to an object to do, don't ask it for one thousand things and then do it yourself.

getters freak me out, cause it means other oop principles are going to be violated hard core.


Missing type information.

Have a look at these method signatures:

  1. public List<Invoice> GetInvoices(Customer c, Date d1, Date d2)

  2. public GetInvoices(c, d1, d2)

In (1) there is clarity. You know exactly what parameters you need to call the function with and it is clear what the function returns.

In (2) there is only uncertainty. You have no idea what parameters to use and you don't know what the function returns if something at all. You are effectively forced to use an inefficient trial and error approach to programming.

  • 3
    Which language is this?
    – FinnNk
    Commented Aug 4, 2011 at 8:04

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