Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share

locked by Thomas Owens Feb 24 at 19:15

This post has been locked due to the high amount of off-topic comments generated. For extended discussions, please use chat.

closed as not constructive by Mark Trapp Sep 26 '11 at 8:19

As it currently stands, this question is not a good fit for our Q&A format. We expect answers to be supported by facts, references, or expertise, but this question will likely solicit debate, arguments, polling, or extended discussion. If you feel that this question can be improved and possibly reopened, visit the help center for guidance.If this question can be reworded to fit the rules in the help center, please edit the question.

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 Oct 25 '10 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.) –  Chris Cudmore Oct 25 '10 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. –  the Tin Man Nov 4 '10 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. –  Paul Nathan Nov 16 '10 at 22:35
24  
Hmm. printf("%c", 7) typically rings alarm bells for me. ;) –  fennec Dec 13 '10 at 15:03
show 16 more comments

60 Answers 60

Functions that reimplement basic functionality of the language. For example, if you ever see a "getStringLength()" method in JavaScript instead of a call to the ".length" property of a string, you know you're in trouble.

share
add comment

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()).

share
add comment
ON ERROR RESUME NEXT

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.

share
1  
That brings back horrible, horrible memories. –  E.Z. Hart Mar 4 '11 at 19:10
show 2 more comments

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
}
else
{
    // **This won't ever happen**
}

or

if (messages.Count > 0)
{
    // Do stuff with messages
}
else if (messages.Count == 1)
{
    // **I hope this code isn't important...**
}
share
add comment

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).
share
add comment

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.

share
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 Oct 24 '10 at 21:54
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 Oct 24 '10 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. –  Evan Plaice Nov 19 '10 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. –  Evan Plaice Nov 19 '10 at 22:22
show 2 more comments

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.

share
add comment

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...

share
add comment
  • 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
share
1  
@Gertjan: "Some languages force you to put the local variables on the top of the [function]." Like, you know, ANSI Standard C. –  Stuart P. Bentley Jun 7 '11 at 1:58
show 1 more comment

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 == '192.168.1.5'){
   printer = '192.168.1.123';
} 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 192.168.1.5 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!

share
add comment

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.

share
show 1 more comment
try
{
//do something
}
catch{}
share
add comment

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.

share
3  
Or if your langauge is powerful enough, define a type that does not allow negative values? –  flamingpenguin Oct 25 '10 at 13:35
add comment

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.

share
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 –  Anonymous Type Oct 28 '10 at 22:37
show 1 more comment

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.

share
add comment

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 :-) )

share
add comment

Disgruntled comments demonstrating lack of restraint:

//I CAN'T GET THIS F^#&^ING PIECE OF S$^ TO COMPILE ON M$VC

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.

share
add comment

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... :/

share
add comment

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.

share
show 1 more comment

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

for (var i = 0; i < num; i++) {
    //alert(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.

share
add comment

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:

try:
    do_something()
except:
    pass

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 {
    doSomething();
} 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.

share
add comment

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.

share
add comment

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.

share
add comment

Something like this

x.y.z.a[b].c

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.

share
show 1 more comment

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

share
show 1 more comment

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 ...

share
add comment
  • $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.
share
show 2 more comments

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.
share
add comment

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");
            return;
        }

This is from a real live production codebase!

share
add comment

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.

share
3  
Which language is this? –  FinnNk Aug 4 '11 at 8:04
add comment

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