Which things instantly ring alarm bells when looking at code? [closed]

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

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 2

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 3

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 4

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 5

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

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 7

try
{
//do something
}
catch{}

Answer 8

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 9

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 10

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 11

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 12

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 13

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 14

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 15

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 16

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 17

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 18

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 19

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 20

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 21

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 22

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 23

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 24

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

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 26

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 27

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

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 29

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 30

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.