Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I attended a software craftsmanship event a couple of weeks ago and one of the comments made was "I'm sure we all recognize bad code when we see it" and everyone nodded sagely without further discussion.

This sort of thing always worries me as there's that truism that everyone thinks they're an above average driver. Although I think I can recognize bad code I'd love to learn more about what other people consider to be code smells as it's rarely discussed in detail on people's blogs and only in a handful of books. In particular I think it'd be interesting to hear about anything that's a code smell in one language but not another.

I'll start off with an easy one:

Code in source control that has a high proportion of commented out code - why is it there? was it meant to be deleted? is it a half finished piece of work? maybe it shouldn't have been commented out and was only done when someone was testing something out? Personally I find this sort of thing really annoying even if it's just the odd line here and there, but when you see large blocks interspersed with the rest of the code it's totally unacceptable. It's also usually an indication that the rest of the code is likely to be of dubious quality as well.

share

locked by Thomas Owens Feb 24 at 19:15

This post has been locked due to the high amount of off-topic comments generated. For extended discussions, please use chat.

closed as not constructive by Mark Trapp Sep 26 '11 at 8:19

As it currently stands, this question is not a good fit for our Q&A format. We expect answers to be supported by facts, references, or expertise, but this question will likely solicit debate, arguments, polling, or extended discussion. If you feel that this question can be improved and possibly reopened, visit the help center for guidance.If this question can be reworded to fit the rules in the help center, please edit the question.

61  
I sometimes encounter people who comment out code, checkin and say "I might need it again in the future - if I remove it now I'll lose it". I have to counter with "Er, ...but that's what source control is for". –  talonx Oct 25 '10 at 10:31
6  
Sometimes, particularly when optimizing, it's handy to leave the old code as a comment, so you know what the obscure optimized code is replacing. Think of leaving a 3 line swap with temp in place when replacing it with a one line bit twiddling swap. (Although, I see no need to use a one line swap -- EVER, unless programme size is of critical importance.) –  Chris Cudmore Oct 25 '10 at 17:02
4  
I'm maintaining/cleaning up code written by one of our engineers, who coded some useful things but admits he isn't a programmers. As I consolidate stuff I'll comment out his old code and then later we go over the changes and I show him how I replaced his with something smaller/more efficient/easier to understand. Afterwards I strip out those blocks then I check it in. Having the old code there has benefits because he sees how things can be done more simply and I can remember why I changed things when we're talking. –  the Tin Man Nov 4 '10 at 7:19
8  
I leave stuff that "might be used" in for 1 commit, then if things don't break down or a need is not found, it gets removed on the next commit. –  Paul Nathan Nov 16 '10 at 22:35
24  
Hmm. printf("%c", 7) typically rings alarm bells for me. ;) –  fennec Dec 13 '10 at 15:03

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.

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

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.

share
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 '10 at 4:32
4  
@KFro, that's loop unrolling. Thats what compilers do all the time :) VERY efficient! –  user1249 Oct 25 '10 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 '10 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. –  LennyProgrammers Dec 13 '10 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. –  Steve314 Dec 23 '10 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;
share
11  
Wow, that's sooo much more readable than swap(x, y); –  JBRWilkinson Nov 22 '10 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. –  TokenMacGuy Dec 13 '10 at 14:56
12  
By the way, this code has undefined in C and C++ because of multiple changes without an intervening sequence point. –  FredOverflow Dec 20 '10 at 16:18
6  
Looking at that, the only thing that came to mind were emoticons: ^_^ –  Darien Aug 4 '11 at 18:05
  • 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.

share
7  
I tend to limit functions to 1 screen whenever possible. –  Matt DiTrolio Oct 25 '10 at 16:08
17  
1 screen is even a stretch. I start feeling dirty after 10 or so lines. –  Bryan Rowe Oct 25 '10 at 20:10
45  
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 '10 at 13:11
6  
@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'. –  DominicMcDonnell Dec 23 '10 at 21:37
5  
@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 '11 at 13:08
{ Let it Leak, people have good enough computers to cope these days }

Whats worse is that's from a commercial library!

share
27  
This doesn't ring alarm bells. It practically kicks you between the legs. –  Steve Evers Oct 27 '10 at 0:17
13  
Daughter is a meth addict. Who cares, at least she won't become obese. ::sigh:: –  Evan Plaice Nov 19 '10 at 21:52
14  
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 '10 at 14:57
11  
"As the deadline fast approaches, LEAKS are all that I can see, somewhere someone whispers, 'Write in C...... eeeeee!!!'" –  chiurox Dec 18 '10 at 14:32
7  
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. –  Martin Beckett Dec 18 '10 at 21:52

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;
share
13  
There is, it's called COBOL :-) –  Gaius Oct 25 '10 at 14:16
22  
The second case isn't the worst. The worst is: /* Set the age of the user to 13 */ a = 18; –  PhiLho Dec 20 '10 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. –  Steve314 Dec 23 '10 at 10:18
2  
Worse again, a for user_age? Really? –  glasnt Mar 24 '11 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 –  Michael Itzoe Nov 15 '12 at 14:25

Code that produces warnings when compiled.

share
1  
There's a candidate for adding 'All warnings as Errors' compiler option to the makefile/project. –  JBRWilkinson Nov 22 '10 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. –  Rei Miyasaka Dec 10 '10 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 '10 at 3:24
11  
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. –  Rei Miyasaka Dec 19 '10 at 18:56

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.

share
2  
+1 - Enter mshtml - it breaks my eyes :( –  Kyle Rozendo Dec 27 '10 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. –  Evan Plaice Jan 11 '11 at 18:22
1  
+1 - I've even seen this used as pseudo-version control method. –  E.Z. Hart Mar 4 '11 at 19:05

Magic Numbers or Magic Strings.

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

   salesPrice *= 0.9;   //apply discount    

   if (invoiceStatus=="Overdue") { reportToCreditAgency(); }
share
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... –  Slokun Oct 25 '10 at 14:39
8  
@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 '10 at 14:43
  • Maybe not the worst but clearly shows the implementers level:

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

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

Moderator's Note

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

share

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.

share
4  
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 '11 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.

share
17  
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 '10 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. –  DisgruntledGoat Oct 27 '10 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? –  Dean Harding Oct 28 '10 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. –  Barry Brown Nov 20 '10 at 23:00

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

share

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.

share

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!

share
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 '10 at 19:11
1  
@Evan Plaice: Wow, you mean the VS IDE recognises when you've implemented something and removes the //TODO comment? Awesome! –  JBRWilkinson Nov 22 '10 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 –  Evan Plaice Nov 25 '10 at 2:37
1  
@Evan +1 "puts their name". –  Morgan Herlocker Nov 29 '10 at 20:23
3  
Better plan than // TODO , use your bug tracker, that's what its for! –  TokenMacGuy Dec 13 '10 at 14:58

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

share
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. –  Billy ONeal Oct 24 '10 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. –  Anonymous Type Oct 25 '10 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 '10 at 8:18
16  
I am more annoyed at scrolling right than at scrolling down. Whitespace is "free." –  Wonko the Sane Oct 25 '10 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 '10 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.

share
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 '10 at 15:23
2  
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. –  Matt Grande Feb 27 '12 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.

share
6  
While your point is excellent, your tone is unnecessary. –  JBRWilkinson Nov 22 '10 at 15:10

Language agnostic:

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

Language specific (C++):

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

Microsoft C++ specific:

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

C++/OOP specific:

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

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

if (itemType == "Student") { ... } // Not Good

-------------------

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

-------------------

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

Weakly typed parameters or return values on methods.

public DataTable GetEmployees() {...}

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

public void FireEmployee(Object EmployeeObjectOrEmployeeID) {...}
share
1  
+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! –  Donal Fellows Aug 4 '11 at 10:30

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.

share
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 '10 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. –  Rei Miyasaka Dec 14 '10 at 1:29

Code smell: not following best practices

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

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

Which things instantly ring alarm bells when looking at code?

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

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

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

if (something) {
    // whatever
}

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

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

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

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.

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

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

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.

share
1  
+1 delete Considered Harmful. –  György Andrasek Oct 26 '10 at 0:35

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

share
1  
XML schemas count? –  Nick T Oct 25 '10 at 16:45
3  
/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? –  Tom Anderson Oct 26 '10 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. –  Tom Anderson Oct 26 '10 at 13:43
2  
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) –  Sean Patrick Floyd Nov 22 '10 at 14:15

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.

share

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.

share

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