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

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.

60 Answers

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

Language agnostic:

  • TODO: not implemented
  • int function(...) { return -1; } (the same as "not implemented")
  • Throwing an exception for a non-exceptional reason.
  • Misuse or inconsistent use of 0, -1 or null as exceptional return values.
  • Assertions without a convincing comment saying why it should never fail.

Language specific (C++):

  • C++ Macros in lowercase.
  • Static or Global C++ variables.
  • Uninitialized or unused variables.
  • Any array new that is apparently not RAII-safe.
  • Any use of array or pointer that is apparently not bounds-safe. This includes printf.
  • Any use of the un-initialized portion of an array.

Microsoft C++ specific:

  • Any identifier names that collide with a macro already defined by any of the Microsoft SDK header files.
  • In general, any use of the Win32 API is a big source of alarm bells. Always have MSDN open, and look up the arguments/return value definitions whenever in doubt. (Edited)

C++/OOP specific:

  • Implementation (concrete class) inheritance where the parent class have both virtual and non-virtual methods, without a clear obvious conceptual distinction between what should/should not be virtual.
share
17  
//TODO: Comment on this answer –  johnc Oct 25 '10 at 8:55
8  
I guess "language agnostic" means "C/C++/Java" now? –  Inaimathi Oct 25 '10 at 14:51
2  
@Inaimathi -- TODO comments, function stubs, exception abuse, confusion of zero/null/empty semantics and pointless sanity checks are inherent to all imperative/OOP languages and to some extent all programming languages in general. –  Rei Miyasaka Oct 28 '10 at 4:17
show 2 more comments

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
try
{
//do something
}
catch{}
share
add comment

Linking obviously GPL'd source code into a commercial, closed-source program.

Not only does it create an immediate legal problem, but in my experience, it usually indicates either carelessness or unconcern which is reflected elsewhere in the code as well.

share
6  
While your point is excellent, your tone is unnecessary. –  JBRWilkinson Nov 22 '10 at 15:10
show 3 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
/* Fuck this error */

Typically found inside a nonsense try..catch block, it tends to grab my attention. Just about as well as /* Not sure what this does, but removing it breaks the build */.

A couple more things:

  • Multiple nested complex if statements
  • Try-catch blocks that are used to determine a logic flow on a regular basis
  • Functions with generic names process, data, change, rework, modify
  • Six or seven different bracing styles in 100 lines

One I just found:

/* Stupid database */
$conn = null;
while(!$conn) {
    $conn = mysql_connect("localhost", "root", "[pass removed]");
}
/* Finally! */
echo("Connected successfully.");

Right, because having to brute force your MySQL connections is the right way do things. Turns out the database was having issues with the number of connections so they kept timing out. Instead of debugging this, they simply attempted again and again until it worked.

share
17  
If only I could upvote this 6 times! All good examples. I also dislike arrogant/funny comments (especially if they include swearing) - it might be slightly amusing the first time you read them but get very old (and hence distracting) very quickly. –  FinnNk Oct 24 '10 at 22:14
5  
I like your example, although I would say that in a certain contexts, multiple nested if statements are unavoidable. With a lot of business logic, the code can be a bit confusing, but if the business itself is confusing to begin with, to simplify the code would be to less accurately model the process. As Einstein said: "Things should be as simple as possible and not one bit simpler." –  Morgan Herlocker Oct 25 '10 at 1:15
2  
@Prof Plum -- What example can you give? Usually the alternative to multiple nested if's is to break it out into (many) methods. Junior developers tend to avoid this as though it's less desirable than the if's; but usually when pressed they say "if's do it in fewer lines". It takes someone confident in OOP to step in and remind them that fewer lines != better code. –  STW Oct 25 '10 at 1:29
2  
@STW That is a good point, however, I would say that it depends how deep the nesting is. I would certainly agree that anything more then three nests deep is often in need of refactoring, because its going to get pretty hairy. Insurance quoting, however is a good example where multi-nesting can actually model the real world quite well. With exceptions to certain rates/premiums, the manual will literally read something like "if this is a property and if it has a minimum rate below 5.6 and if it is in NC and if it has a boat on the premises, then do such and such." with many other options. –  Morgan Herlocker Oct 25 '10 at 1:44
4  
@josh, if "they" are colleagues then I would ponder on why you did not say "we"... –  user1249 Oct 25 '10 at 2:28
show 14 more comments

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

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

@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

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
  • $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
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
  • Maybe not the worst but clearly shows the implementers level:

    if(something == true) 
    
  • If a language has a for loop or iterator construct, then using a while loop also demonstrates implementers level of understanding of the language:

    count = 0; 
    while(count < items.size()){
       do stuff
       count ++; 
    }
    
    for(i = 0; i < items.size(); i++){
      do stuff 
    }
    //Sure this is not terrible but use the language the way it was meant to be used.
    
  • Poor spelling/grammar in documentation/comments eats at my almost as much as code itself. The reason for this is because code was meant for humans to read and machines to run. This is why we use high level languages, if your documentation is hard to get through it makes me preemptively form a negative opinion of the codebase without looking at it.

Moderator's Note

If you want to discuss this answer further take it to chat.

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

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

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

Code smell: not following best practices

This sort of thing always worries me as there's that truism that everyone thinks they're an above average driver.

Here's a news flash for ya: 50% of the world's population is below average intelligence. Ok, so some people would have exactly average intelligence, but let's not get picky. Also, one of the side affects of stupidity is you can't recognize your own stupidity! Things don't look so good if you combine these statements.

Which things instantly ring alarm bells when looking at code?

Many good things have been mentioned, and in general it seems that not following best practices is a code smell.

Best practices are usually not invented randomly, and are often there for a reason. Many times it can be subjective, but in my experience they are mostly justified. Following best practices should not be a problem, and if you're wondering why they are as they are, research it rather than ignoring and/or complaining about it - maybe it's justified, maybe it's not.

One example of a best practice might be using curlies with every if-block, even if it only contains one line:

if (something) {
    // whatever
}

You might not think it's necessary, but I recently read that it is a major source of bugs. Always using brackets have also been discussed on Stack Overflow, and checking that if-statements have brackets is also a rule in PMD, a static code analyzer for Java.

Remember: "Because it's best practice" is never an acceptable answer to the question "why are you doing this?" If you can't articulate why something is a best practice, then it's not a best practice, it's a superstition.

share
2  
This may be pedantic, but I think it matters what average you pick. As I understand it, 50% of the world's population is below the median intelligence (by definition); but other averages do not work the same way. For example take the population (1, 1, 1, 5) which has an arithmetic mean of 2. –  flamingpenguin Oct 25 '10 at 13:33
3  
The flipside of this is people who slavishly follow "best practices" without any critical thought as to why something is a "best practice". This is why I strongly dislike the term "best practice", because for some people it's an excuse to stop thinking and follow the herd. "Because it's best practice" is never an acceptable answer to the question "why are you doing this?" If you can't articulate why something is a best practice, then it's not a best practice, it's a superstition. –  Dan Dyer Dec 18 '10 at 10:43
show 4 more comments

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

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

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

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

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

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

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

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

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

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

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

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