Which things instantly ring alarm bells when looking at code?

Question

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.

Answer 1

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

Answer 2

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.

Answer 3

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

x ^= y ^= x ^= y;

Answer 4

{ Let it Leak, people have good enough computers to cope these days }

Whats worse is that's from a commercial library!

Answer 5

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

Answer 6

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;

Answer 7

Code that produces warnings when compiled.

Answer 8

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.

Answer 9

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

Answer 10

Magic Numbers or Magic Strings.

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

   salesPrice *= 0.9;   //apply discount    

   if (invoiceStatus=="Overdue") { reportToCreditAgency(); }

Answer 11

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.

Answer 12

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.

Answer 13

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.

Answer 14

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

Answer 15

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!

Answer 16

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

Answer 17

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.

Answer 18

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.

Answer 19

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.

Answer 20

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

Not good:

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

Better:

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

Best:

private itemTypeEnum itemType;
...
if (itemType == itemTypeEnum.Student) { ... }

Answer 21

Weakly typed parameters or return values on methods.

public DataTable GetEmployees() {...}

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

public void FireEmployee(Object EmployeeObjectOrEmployeeID) {...}

Answer 22

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.

Answer 23

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.

Answer 24

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

Answer 25

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.

Answer 26

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

Answer 27

Catching general exceptions:

try {

 ...

} catch {
}

or

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.

Answer 28

#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

Answer 29

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.

Answer 30

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.

Answer 31

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.

Answer 32

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

Answer 33

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.

Answer 34

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

Answer 35

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

Answer 36

• 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

Answer 37

try
{
//do something
}
catch{}

Answer 38

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.

Answer 39

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.

Answer 40

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

Answer 41

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.

Answer 42

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

Answer 43

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.

Answer 44

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

Answer 45

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!

Answer 46

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.

Answer 47

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!

Answer 48

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.

Answer 49

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.

Answer 50

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.

Answer 51

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.

Answer 52

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

Answer 53

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.

Answer 54

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

Answer 55

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

Answer 56

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

Answer 57

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.

Answer 58

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.

Answer 59

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.

Answer 60

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.