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

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

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

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

Whats worse is that's from a commercial library!

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

Magic Numbers or Magic Strings.

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

   salesPrice *= 0.9;   //apply discount    

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

Answer 10

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

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 22

Weakly typed parameters or return values on methods.

public DataTable GetEmployees() {...}

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

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

Answer 23

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

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

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 29

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 30

#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