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
    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.) 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. 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. Nov 16, 2010 at 22:35
  • 24
    Hmm. printf("%c", 7) typically rings alarm bells for me. ;)
    – user2348
    Dec 13, 2010 at 15:03

60 Answers 60

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

  • 19
    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, 2010 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." Oct 25, 2010 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, 2010 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. Oct 25, 2010 at 1:44
  • 4
    @josh, if "they" are colleagues then I would ponder on why you did not say "we"...
    – user1249
    Oct 25, 2010 at 2:28

The major red flag for me is duplicated code blocks, because it shows that the person either doesn't understand programming fundamentals or was too scared to make the proper changes to an existing code base.

I used to also count lack of comments as a red flag, but having recently worked on a lot of very good code with no comments I've eased back on that.

  • 1
    +1: I saw some code from the colleague who touted himself as a linux expert, who wrote a simple looping application as one long function, the whole thing over and over in main(). Yikes.
    – KFro
    Oct 25, 2010 at 4:32
  • 4
    @KFro, that's loop unrolling. Thats what compilers do all the time :) VERY efficient!
    – user1249
    Oct 25, 2010 at 6:23
  • 3
    @Thorbjorn: Sometimes, you have to help the compiler a little; after all, you are the smart human and he's just a dumb computer.
    – yatima2975
    Oct 25, 2010 at 10:16
  • 3
    Another reason i've seen: the consultant was paid to implement the feature as quickly as possible (that's why tests and documentation are missing too, of course). Copy/paste is faster than thinking about how to do things right. Dec 13, 2010 at 17:07
  • 4
    Avoiding code duplication can be an obsession. In a language like C++, it isn't always as easy as it should be to factor out the varying parts yet still have robust and efficient code. Sometimes, a bit of cut-and-paste is the more practical option. Also, the optimisation principle can apply - cut-and-paste can get you a quick and easy solution which you can refactor later if needed. You may be saving a maintenance headache for later, but you know for certain that you're avoiding delays right now.
    – user8709
    Dec 23, 2010 at 10:02

Code that tries to show off how clever the programmer is, despite the fact that it adds no real value:

x ^= y ^= x ^= y;
  • 14
    Wow, that's sooo much more readable than swap(x, y); Nov 22, 2010 at 15:04
  • 8
    if x and y are pointers, and this assignment takes place over a reasonble length of time, it soundly breaks conservative garbage collectors like Boehm GC. Dec 13, 2010 at 14:56
  • 12
    By the way, this code has undefined in C and C++ because of multiple changes without an intervening sequence point. Dec 20, 2010 at 16:18
  • 11
    Looking at that, the only thing that came to mind were emoticons: ^_^
    – Darien
    Aug 4, 2011 at 18:05
{ Let it Leak, people have good enough computers to cope these days }

Whats worse is that's from a commercial library!

  • 33
    This doesn't ring alarm bells. It practically kicks you between the legs. Oct 27, 2010 at 0:17
  • 16
    Daughter is a meth addict. Who cares, at least she won't become obese. ::sigh:: Nov 19, 2010 at 21:52
  • 17
    When I find myself in times of trouble, mother buntu comes to me. Speaking words of wisdom, let it leak. LET IT LEAK .. LET IT LEAK. LET IT LEAK oh LET IT LEAK. If it just leaks one time, it's not a leaaaaaak. (if you read this far, +1). I really need to consider decaf.
    – Tim Post
    Dec 13, 2010 at 14:57
  • 13
    "As the deadline fast approaches, LEAKS are all that I can see, somewhere someone whispers, 'Write in C...... eeeeee!!!'"
    – chiurox
    Dec 18, 2010 at 14:32
  • 9
    Once upon a time there were two OSes. One leaked and crashed if it ran for more than 49days, the other was perfect and would run for ever. One was launched in 1995 to a huge fanfair and was used by millions of people - the other never shipped because they are still checking that it is bug free. There is a difference between philosophy and engineering. Dec 18, 2010 at 21:52
  • 20,000 (exaggeration) line functions. Any function that takes more than a couple of screens needs re-factoring.

  • Along the same lines, class files that seem to go on forever. There are probably a few concepts that could be abstracted into classes which would clear up the purpose and function of the original class, and probably where it is being used, unless they are all internal methods.

  • non-descriptive, non-trivial variables, or too many trivial non-descriptive variables. These make deducing what is actually happening a riddle.

  • 9
    I tend to limit functions to 1 screen whenever possible. Oct 25, 2010 at 16:08
  • 20
    1 screen is even a stretch. I start feeling dirty after 10 or so lines.
    – Bryan Rowe
    Oct 25, 2010 at 20:10
  • 56
    Okay, I'm going to voice what may be an unpopular opinion. I say it's code smell to write functions that are atomic, top-to-bottom processes that are broken into separate functions because the developer is clinging to some "functions should be short" cargo-cultism. Functions should be broken along FUNCTIONAL lines, not just because they should be some mythical "right size". That's why they're called FUNCTIONS.
    – Dan Ray
    Dec 23, 2010 at 13:11
  • 7
    @Dan, Functions shouldn't be short for the sake of being short, but there is only so much info you can hold in your head at one time. Maybe I've got a small brain as for me that limit is a couple of screens :). Breaking functions into multiple functions when they start to test that boundary is necessary to avoid mistakes. On one hand it provides encapsulation so you can think at a higher level and on the other it hides what is happening so makes it harder to work out how the function works. I think that breaking functions up should be done to aid readability, not to fit some 'perfect length'. Dec 23, 2010 at 21:37
  • 6
    @Dominic, @Péter, I think the three of us actually are saying the same thing. When there's a good reason to factor code into a smaller function, I'm for it. What I'm rejecting is shortness for the sake of shortness in function design. You know, a call stack that's three times longer than it needs to be, but at least those functions are short. I'd rather debug a tall function that does one thing well and clearly than chase the execution path through a dozen chained functions that are each only called from the previous one.
    – Dan Ray
    Jan 28, 2011 at 13:08

Comments that are so verbose that if there were an English compiler, it would compile and run perfectly, yet doesn't describe anything that the code doesn't.

//Copy the value of y to temp.
temp = y;
//Copy the value of x to y.
y = x;
//Copy the value of temp to x.
x = temp;

Also, comments on code that could have been done away with had the code adhered to some basic guidelines:

//Set the age of the user to 13.
a = 13;
  • 15
    There is, it's called COBOL :-)
    – Gaius
    Oct 25, 2010 at 14:16
  • 28
    The second case isn't the worst. The worst is: /* Set the age of the user to 13 */ a = 18;
    – PhiLho
    Dec 20, 2010 at 10:13
  • 4
    @PhiLho - no, even worse is when the / from the */ is missing, so all the code to the end of the next */ is ignored. Luckily, syntax highlighting makes that kind of thing rare these days.
    – user8709
    Dec 23, 2010 at 10:18
  • 3
    Worse again, a for user_age? Really?
    – glasnt
    Mar 24, 2011 at 3:05
  • 2
    I used to maintain a code standard doc at a previous employer, one section of which was appropriate commenting. My favorite example was from MSDN: i = i + 1; //increment i Nov 15, 2012 at 14:25

Code that produces warnings when compiled.

  • 1
    There's a candidate for adding 'All warnings as Errors' compiler option to the makefile/project. Nov 22, 2010 at 15:07
  • 3
    I guess that could be useful if you're in a project with multiple people that you simply don't trust -- though if I were to join a project where that option's set, that in itself would have me worrying about how able the other programmers are. Dec 10, 2010 at 15:22
  • 1
    I disagree with this. Some compiler warnings (like comparison between signed and unsigned, when you know both values to be unsigned, even if the types are different) are preferable to littering code with needless casts. If I reduce code by using a portable signed integer that a function modifies only if the integer has an unsigned value, I'm going to do that.
    – Tim Post
    Dec 19, 2010 at 3:24
  • 13
    I'd rather clutter my code with an almost-superfluous (unsigned int) than clutter my warning/error lists with benign warnings. I'd hate for the warning list to become a blind spot. It's also much more of a PITA explaining to other people why you're ignoring a warning than it is to explain why you're doing a cast of natural ints to unsigned ints. Dec 19, 2010 at 18:56
  • Occasionally, you have to work with an API that spews errors whatever you do. Classic examples are where the API is defined in terms of things that are broken by design (some old ioctl() constants were like that, and sometimes OS developers insist on using the wrong type in their headers) or where they've deprecated something without leaving a good replacement (thank you, Apple). Aug 4, 2011 at 10:13

Functions with numbers in the name instead of having descriptive names, like:

void doSomething()

void doSomething2()

Please, make the function names mean something! If doSomething and doSomething2 do similar things, use function names that differentiate the differences. If doSomething2 is a break-out of functionality from doSomething, name it for its functionality.

  • Likewise @Parm for SQL
    – Dave
    Nov 22, 2010 at 19:04
  • 2
    +1 - Enter mshtml - it breaks my eyes :( Dec 27, 2010 at 7:59
  • 3
    The exception to this would be GUI code. For instance, if a snail-mail form had two addresses; address1 and address2 is more reasonable than address and alternateAddress. Spurious labels that are static-only are also a reasonable exception too. Jan 11, 2011 at 18:22
  • @Evan - fair enough, although I was more making a distinction in functionality. Jan 11, 2011 at 18:25
  • 1
    +1 - I've even seen this used as pseudo-version control method.
    – E.Z. Hart
    Mar 4, 2011 at 19:05

Magic Numbers or Magic Strings.

   if (accountBalance>200) { sendInvoice(...); }

   salesPrice *= 0.9;   //apply discount    

   if (invoiceStatus=="Overdue") { reportToCreditAgency(); }
  • 4
    Not really that bad with the second two, at least the discount is explained, and the "Overdue" is intuitive. The 200, on the other hand...
    – Tarka
    Oct 25, 2010 at 14:39
  • 10
    @Slokun - Intuitive isn't the point so much as maintainability and fragility. For example, consider what happens when the discount amount changes and 0.9 is hard-coded in six different places. Also, using strings instead of constants/enums is asking for trouble in languages with case sensitive strings.
    – JohnFx
    Oct 25, 2010 at 14:43
  • +1 I just spent way too much time debugging a problem that turned out to be caused by a line 'timeout=15;' buried in a program. Dec 13, 2010 at 21:35
  • I think the last one is sometimes okay, depending on where that data for invoiceStatus came from. If it just came from a public api that returns JSON that was just decoded, checking against a hard-coded String constant is probably okay. Agree though that "200" and "0.9" are just magic constants and should not be hard-coded in this way. Even if they are only used in this one place, maintenance is easier if you define them in a configuration section separately rather than interspersing them in the logic code. And if they are used in multiple places, maintenance becomes much easier.
    – Ben Lee
    Feb 25, 2013 at 21:30
  • 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.


The one I notice immediately is the frequency of deeply nested code blocks (if's, while's, etc). If code is frequently going more than two or three levels deep, thats a sign of a design/logic problem. And if it goes like 8 nests deep, there better be a darn good reason for it not to be broken up.

  • 6
    I know some people learned that every method should have only one return statement, but when it causes 6+ levels of if/then nesting I think its doing far more harm than good.
    – Darien
    Aug 4, 2011 at 18:08

When grading a student's program, I can sometimes tell in a "blink"-style moment. These are the instant clues:

  • Poor or inconsistent formatting
  • More than two blank lines in a row
  • Nonstandard or inconsistent naming conventions
  • Repeated code, the more verbatim the repeats, the stronger the warning
  • What should be a simple piece of code is overly complicated (for example, checking the arguments passed to main in a convoluted way)

Rarely are my first impressions incorrect, and these warning bells are right about 95% of the time. For one exception, a student new to the language was using a style from a different programming language. Digging in and re-reading their style in the idiom of the other language removed the alarm bells for me, and the student then got full credit. But such exceptions are rare.

When considering more advanced code, these are my other warnings:

  • The presence of many Java classes that are only "structs" to hold data. It doesn't matter if the fields are public or private and use getters/setters, it's still not likely part of a well thought-out design.
  • Classes that have poor names, such as just being a namespace and there's no real cohesion in the code
  • Reference to design patterns that aren't even used in the code
  • Empty exception handlers without explanation
  • When I pull up the code in Eclipse, hundreds of yellow "warnings" line the code, mostly due to unused imports or variables

In terms of style, I generally don't like to see:

  • Javadoc comments that only echo the code

These are only clues to bad code. Sometimes what may seem like bad code really isn't, because you don't know the programmer's intentions. For instance, there may be a good reason that something seems overly complex-- there may have been another consideration at play.

  • I fail to see how using the style from one language in another is a grievous error (2, 4, 8 spaces per indent...). If the student has little other style to follow, there's nothing wrong with being self-consistent. As the grader you see a billion programs so you're at the opposite end of that spectrum, but that isn't a reason to totally dismiss a different (but consistent) style.
    – Nick T
    Oct 25, 2010 at 16:37
  • 18
    I see nothing wrong with classes that simply aggregate data - i.e., structs. That's what Data Transfer Objects (DTOs) are and can make code much more readable than say, e.g. passing in 20 parameters to a method.
    – Nemi
    Oct 26, 2010 at 17:14
  • 1
    Your comment about structs is spot on. It's fine when the data is in its rawest form and will not be modified in any way. But 95% of the time you should have some functions in the class to format/operate on the data. I just remembered some of my code that essentially uses that pattern and ought to be improved upon. Oct 27, 2010 at 14:42
  • 2
    +1 for inconsistent style and extra line breaks (I've seen random indentations, no indentation, random and changing naming conventions and more - and this in production code!). If you can't even bother to get that right, then what else can't you be bothered doing? Oct 28, 2010 at 5:19
  • 1
    I look for the declaration line of a method being indented too far relative to the body. This is a sign they copy-and-pasted from someone else's file. Nov 20, 2010 at 23:00

Personal favorite/pet peeve: IDE generated names that get comitted. If TextBox1 is a major and important variable in your system, you've got another thing coming come code review.


Completely unused variables, especially when the variable has a name similar to variable names that are used.


A lot of people have mentioned:

//TODO: [something not implemented]

While I wish that stuff was implemented, at least they made a note. What I think is worse is:

//TODO: [something that is already implemented]

Todo's are worthless and confusing if you never bother to remove them!

  • 1
    I just went through the excercise of having to produce a report of all the TODOs in our release product, plus a plan for dispositoning them. Nearly half turned to out to be obsolete.
    – AShelly
    Oct 26, 2010 at 19:11
  • 1
    -1 TODO comments are used in MS Visual Studio to track parts of the project that still need work. IE, the IDE keeps a list that tracks TODOs so you can easily click on them and be brought directly to the line that the TODO exists on. I would rather see TODOs placed explicitly to see what work still needs to be done. See dotnetperls.com/todo. Nov 19, 2010 at 21:49
  • 3
    @Evan Plaice: Wow, you mean the VS IDE recognises when you've implemented something and removes the //TODO comment? Awesome! Nov 22, 2010 at 15:06
  • 4
    @Prof Plum Why not just create a policy where the person responsible for a TODO puts their name in the comment. That way, if there are some leftovers it Nov 25, 2010 at 2:37
  • 3
    Better plan than // TODO , use your bug tracker, that's what its for! Dec 13, 2010 at 14:58

A method that requires me to scroll down to read it all.

  • 14
    Hmm.. this depends on what's being implemented. It would not be unreasonable for this to occur when implementing some complicated algorithms, as that is how they are defined. Of course, if the majority of methods are that way, that is a red flag. Oct 24, 2010 at 23:51
  • 9
    As a blanket statement then I disagree, wasting time constantly refactoring so that everything fits this kind of self imposed rule actually increases the overall cost of the project. Oct 25, 2010 at 2:48
  • 1
    I disagree that this rule can increase the overall cost of a project but I guess that is subjective because it would be dependant on the developer(s). If you keep to the general principle of "separation of concerns" whilst developing then re-factoring would be less of a task if one chose to do it. Something for consideration is how much would it cost three years down the line when developers who didn't code the original project are trying to bug fix a bunch of methods with 300+ lines of code? How much does that cost?
    – BradB
    Oct 25, 2010 at 8:18
  • 18
    I am more annoyed at scrolling right than at scrolling down. Whitespace is "free." Oct 25, 2010 at 13:37
  • 4
    I'd rather scroll than have to skip all over the file (or multiple files) to figure out what the code is doing.
    – TMN
    Oct 25, 2010 at 17:15

Conjunctions in method names:

public void addEmployeeAndUpdatePayrate(...) 

public int getSalaryOrHourlyPay(int Employee) ....

Clarification: The reason this rings alarm bells is that it indicates the method likely violates the single responsibility principle.

  • Hmmm...if the function name accurately defines what the function does, then I disagree. If the method does two separate things that would be better to separate, then I may agree, depending on the context. Oct 25, 2010 at 15:07
  • 2
    That's the point. The conjunction implies that it is very likely it does more than one thing. Per the question it is just something that raises my awareness that something MAY be wrong.
    – JohnFx
    Oct 25, 2010 at 15:23
  • 3
    Now what if you have to add an employee and update their payrate in several places? If that method contains two method calls (addEmployee(); updatePayrate();), then I don't think that's a problem. Feb 27, 2012 at 16:36

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.

  • 7
    While your point is excellent, your tone is unnecessary. Nov 22, 2010 at 15:10
  • @JBRWilkinson: Thanks, you're correct. My apologies to all.
    – Bob Murphy
    Nov 23, 2010 at 0:03
  • I think your heading needs re-writing. Both statically and dynamically linking to GPL'd source code is a violation of the GPL... Aug 4, 2011 at 8:45
  • Good points. I've rewritten the entire post. Thanks to all.
    – Bob Murphy
    Aug 4, 2011 at 17:02

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.
  • 19
    //TODO: Comment on this answer
    – johnc
    Oct 25, 2010 at 8:55
  • 8
    I guess "language agnostic" means "C/C++/Java" now?
    – Inaimathi
    Oct 25, 2010 at 14:51
  • +1 "Throwing an exception for a non-exceptional reason" couldn't agree more!
    – billy.bob
    Oct 26, 2010 at 8:36
  • 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. Oct 28, 2010 at 4:17
  • I believe that C preprocessor macros in lower case are okay, but only if they evaluate their arguments only once and result in only a single statement.
    – Joe D
    Jan 2, 2011 at 22:49

Using lots of text-blocks rather than enums or globally defined variables.

Not good:

if (itemType == "Student") { ... }


private enum itemTypeEnum {
if (itemType == itemTypeEnum.Student.ToString()) { ... }


private itemTypeEnum itemType;
if (itemType == itemTypeEnum.Student) { ... }
  • Or bestest: Use polymorphism.
    – Lstor
    Aug 4, 2011 at 7:52

Bizarre indentation style.

There's a couple of very popular styles, and people will take that debate to the grave. But sometimes I see someone using a really rare, or even a home-grown indentation style. This is a sign that they have probably not been coding with anyone other than themselves.

  • 2
    or just a sign that they are a highly prized individualistic talent that hasn't become caught up in the web of homogonised coding practises that are in no way related to "best practises". Oct 28, 2010 at 22:35
  • 1
    I hope you're being sarcastic. If someone is coding in such an unusual way, that should set off alarm bells. They can be as artistic as they want, but still... alarm bells.
    – Ken
    Oct 29, 2010 at 2:23
  • 2
    There's a somewhat uncommon style (I believe it's called Utrecht style) that I find is quite useful despite being extremely uncommon outside of Haskell/ML/F#. Scroll down to "module Shapes": learnyouahaskell.com/making-our-own-types-and-typeclasses . What's nice about this style is that you don't have to modify the delimiter on the preceding line to add a new one -- which I often forget to do. Dec 14, 2010 at 1:29
  • @ReiMiyasaka Seven years late, but...Utrecht style really irks me. I believe that it is an error in the Haskell specification not to impose another "layout rule" on vertically-organized lists. That way, the parser could detect a new list entry just by checking indentation, which is how everyone organizes their lists anyway.
    – Ryan Reich
    Jul 29, 2017 at 19:18
  • @RyanReich Weird, seven years later, and I still like it. I do agree, though; for all its syntactical awkwardness and failings, F# also allows items to be delimited just by a newline and indentation (in most cases), which makes for tidy code. Aug 1, 2017 at 23:11

Weakly typed parameters or return values on methods.

public DataTable GetEmployees() {...}

public DateTime getHireDate(DataTable EmployeeInfo) {...}

public void FireEmployee(Object EmployeeObjectOrEmployeeID) {...}
  • 2
    +1: I have to work with some “REST” web services that return everything as pseudo-HTML tables, even where you pass in things that are a clear syntactic error. Not authorized? Input complete junk? Server over capacity? 200 (plus a message in a horrid format in a one column, one row table). AAaaaaaaaargh! Aug 4, 2011 at 10:30
  • Multiple ternary operators strung together, so instead of resembling an if...else block, it becomes an if...else if...[...]...else block
  • Long variable names with no underscores or camelCasing. Example from some code I have pulled up: $lesseeloginaccountservice
  • Hundreds of lines of code in a file, with little to no comments, and the code being very non-obvious
  • Overly complicated if statements. Example from code: if (!($lessee_obj instanceof Lessee && $lessee_obj != NULL)) which I chomped down to if ($lessee_obj == null)

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.

  • 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. Oct 25, 2010 at 13:33
  • ummm, you cited the "what-superstitions-do-programmers-have" post where a user made a bold statement about curly braces with no source. I don't see that as a good example of best practices. Nov 19, 2010 at 22:33
  • @Evan: yes, you're right. I added a bit more about that, hope this helps.
    – Vetle
    Nov 20, 2010 at 10:58
  • 4
    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, 2010 at 10:43
  • Very good comment, Dan! I added the two last lines to the answer.
    – Vetle
    Dec 20, 2010 at 9:38

Comments that say "this is because the design of the froz subsystem is totally borked."

That go on over an entire paragraph.

They explain that the following refactor needs to happen.

But did not do it.

Now, they might have been told they couldn't change it by their boss at the time, because of time or competence issues, but maybe it was because of people being petty.

If a supervisor thinks that j.random. programmer can't do a refactoring, then the supervisor should do it.

Anyway this happens, I know the code was written by a divided team, with possible power politics, and they didn't refactor borked subsystem designs.

True story. It could happen to you.


Can anyone think of an example where code should legitimately refer to a file by absolute path?

  • 1
    XML schemas count?
    – Nick T
    Oct 25, 2010 at 16:45
  • 1
    System configuration files. Typically should be set by ./configure, but even that needs a default value somewhere.
    – eswald
    Oct 25, 2010 at 16:54
  • 4
    /dev/null and friends are okay. But even things like /bin/bash are suspect - what if you're one some kooky system that has /usr/bin/bash? Oct 26, 2010 at 13:40
  • 1
    Web service client code generated by JAX-WS tools (JBossWS and Metro, at least) contains a hardwired absolute path to the WSDL file (twice!). Which is probably something wildly inappropriate like /home/tom/dev/randomhacking/thing.wsdl. It is criminally insane that this is default behaviour. Oct 26, 2010 at 13:43
  • 4
    about /dev/null: I have the habit, when developing on windows to keep apps and libs under c:\dev. Somehow, a folder null is always automatically created inside that folder. I swear I have no idea who does that. (One of my favorite bugs/features) Nov 22, 2010 at 14:15

Catching general exceptions:

try {


} catch {


try {


} catch (Exception ex) {

Region overuse

Typically, using too many regions indicates to me that your classes are too big. It's a warning flag that signals that I should investigate more into that bit of code.

  • Catching general exceptions is only a problem if you rethrow them without doing anything. Really for most things one exception class would suffice. I tend to use runtime_error.
    – CashCow
    Mar 2, 2011 at 13:18
  • +1 for the 'catch and throw away exception' example. If you're not doing anything with the exception, don't catch it. At the very least, log it. At the very, very least put in a comment explaining why it's okay to catch all exceptions and move on at that point in the code.
    – E.Z. Hart
    Mar 4, 2011 at 19:09

Class naming conventions that demonstrate a poor understanding of the abstraction they're attempting to create. Or that don't define an abstraction at all.

An extreme example comes to mind in a VB class I saw once that was titled Data and was 30,000+ lines long...in the first file. It was a partial class split into at least half a dozen other files. Most of the methods were wrappers around stored procs with names like FindXByYWithZ().

Even with less dramatic examples, I've sure we've all just 'dumped' logic into a poorly conceived class, given it a wholly generic title, and regretted it later.


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.

#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...

Of course without any kind of documentation and the occasional nested #defines

  • I've seen exactely this "pattern" yesterday... in production code... and even worse... in C++ production code :-/ Dec 18, 2010 at 10:31

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