For a basic university course in programming, we were supposed to write a simple client program connecting to a server. Here is a part of the server code we were given, written by someone who obviously really, REALLY prefers python's syntax to Java's.

It was only about 50 lines in total, so it's really no big deal, and we didn't have to do anything with it except run it. But the style still bothers me.

import java.net.*                                         ;
import java.io.*                                          ;
import java.util.*                                        ;
public class Server                                       { 
    public static void main( String[] args)               {
        try                                               {
            ServerSocket sock = new ServerSocket(4712,100);
            while(true) new Handler(sock.accept()).start();}
        catch(IOException e) {System.err.println(e);}     ;}} 

class Handler extends Thread                              {
    public void run()                                     {
        Random random=new Random()                        ;
        try                                               {
            //yada yada yada
        catch(Exception e) {System.err.println(e);}       ;}}

Interestingly enough, that's

if (0 == foo()) {}

i.e. putting the constant on the left to avoid an == / = mixup. I know it would be better to do it, and I feel bad for it, but reading it is for me a mental speedbump.

Doesn't say much except that being annoying doesn't mean it's necessarily bad.

Explicitly testing against boolean literals:

if(foo == true)
{
   ...
}

and (Joel Spolsky mentioned this on the podcast) refusing to return boolean expressions:

if(x < 24)
{
    return true;
}
else
{
    return false;
}

that makes me crazy.

Useless comments.

Example:

i++; // Increment of i

Omission of braces just because they are not required for single-statement blocks. Like this:

if (SomeCondition)
    DoSomething();

If you spent enough time staring at code like this

while (SomeCondition)
   DoSomething();
   DoSomeOtherThing();
DoYetAnotherThing();

and wondering "is this a bug or just somebody's sloppy indentation?", then I bet you know what I mean. If you didn't... well, I guess you were just lucky so far. :-)

The use of type prefix in variables name in modern languages like C#,

int iIndex;
string strMessage;

Inconsistent indentation.

I can put up with tabs or spaces, all sorts of brace matching, and various depths of indentation - as long as it is used consistently across a project.

Inconsistent brace usage within the same construct:

if (something)
   this->noteSomething();
else
{
   several();
   statements();
   within();
   braces();
}

Placement of commas like this:

enum Whatever 
{
      SomeValue
    , AnotherValue
    , YetAnotherValue
}

I understand the rationale, but my eyes bleed when I have to read something like that. Very inhumane.

What I don't understand is why language syntax designers do not allow to have redundant delimiter characters at the ends of character-delimited lists. Of course, something like this

enum Whatever 
{
    SomeValue,
    AnotherValue,
    YetAnotherValue,
}

would be a little inhumane too but, in my humble opinion, not nearly as ugly as the first sample...

GNU style ^[1], of course.

Spaceless concatenation like this:

string s = "Bob"+" "+objWhatever.ToString()+obj2.ToString()+"isadork";

Overkill abstraction of object oriented design.

Not naming UI elements. I'm dealing with a codebase now that has a tabcontrol, and the controls in each tab aren't UserControls so I think it's up to Button37 and TextBox25 by now.

Since programmers have to write correct code without syntax errors, I am overly picky when it comes to obvious typographical errors in variable and function names. Especially not since rename-refactoring is just a click away.

A few examples:

m_bHasLoosed
SetDesturctionMode
GetAdminitsrationRights
HasPlayerWinningGame

The last one being the worst of all.

I haven't seen SESE (Single-Entry, Single-Exit) mentioned yet:

int somefunc( somearg arg, someotherarg otherarg ) {
    int ret = 0;
    if ( arg ) {
        if ( otherarg ) {
            // ok, we're good
            ret = ...;
        } else {
            ret = -1;
        }
    } else {
        ret = -1;
    }
    return ret;
}

Makes me puke. Guard clauses are so much better:

int somefunc( somearg arg, someotherarg otherarg ) {

    if ( !arg )
        return -1;

    if ( !otherarg )
        return -1;

    // ok, we're good
    return ...;
}

The coding style that bothers me the most is people who comment out code in revision-controlled code -- and then never delete it! One step even worse is revision-controlled code that has dozens of files that are no longer used or even linked to. Grr..

Declaring all variables at the start of a function, away from the scope in which they're used:

void ugly() {
    int x, y, i, j, count, length, count2;
    bool done, match, nomatch, p, q;
    double f, g;
    char *buff, *buff2;

    // ...
}

I found this the other day in work when trawling through our source control. I think it's one of the most creative uses of the switch syntax I've ever seen;

bool flag;

// snip

switch(flag)
{
    case true:
    {
    	// Do something
    }
    break;

    default:
    {
    	// Do something else
    }
    break;
}

Redundant parentheses:

if (((x) && (isValid())) || ((y < 1) && (y > 100))) {
// ...
}

Hungarian notation in any form

When the variable and method/function names are as unintuitive as they can be:

void function1(int i, int j) {
    for(index, index++, index < 5) 
       for(index1, index1++, index1 < 10)

Code is meant to be as self-explanatory as possible and the most control we as programmers have is on things we can name/define ourselves (comments is really the next level and should be done only if required).

Any code block in a ten year-old source file preceded by:

// Temporary hack - buggy as hell. Will fix later.

Mixing the bracket styles between K & R ^[1] and that other style...

if (condition){
}
else
{
}

Inconsistent mixing of initialization with declaration

int x,y,z=0;

  x = 1;
  y = 1;

Undocumented (uncommented) hacks.

After almost two decades in the field, I can tolerate the lack of comments in general, and I can tolerate justifiable hacks. But uncommented hacks?!...

if (SUCCEEDED(hr))
{
    hr = ...
    if (SUCCEEDED(hr))
    {
        hr = ...
        if (SUCCEEDED(hr))
        {
            hr = ...
            if (SUCCEEDED(hr))
            {
                hr = ...
                if (SUCCEEDED(hr))
                {
                    hr = ...

                    // up to 19 nested if's...
                }
            }
        }   
    }
}

Just because they heard one day that there should be only one return statement per function.

    if(something)
    {
doSomething();
    }
    else
    {
somethingElse();
    }

Makes me want to cry, specially when you are deep in a nest of some sorts

In C, I saw someone get around the "two lines of code need a block" rule like this:

for(/*conditions*/)
    doThis(), doThat();

While I understand wanting to shorten code, that's ridiculous.

I just don't like prefixes in front of class field names: string m_name;

Useless variable names

int temp1 = 10;
int temp2 = 5;
string a = "test";

I watched this video Improving Code Quality with Code Analysis ^[1] from the PDC, and I couldn't believe this example which was provided.

public FldBrwserDlgExForm(): SomeSystem.SomeWindows.SomeForms.SomeForm
{
    this.opnFilDlg = new opnfilDlg();
    this.foldrBrwsrDlg1 = new fldrBrwsrDlg1();
    this.rtb = new rtb();
    this.opnFilDlg.DfltExt = "rtf";
    this.desc = "Select the dir you want to use as default";
    this.fldrBrwsrDlg1.ShowNewFldrBtn = false;
    this.rtb.AcpectsTabs = true;
}

This always makes me angry. When developers rename variables because they believe it saves time and effort. It makes my brain hurt.

Here is what it's supposed to say, you tell me, which one is easier to read?

public FolderBrowserDialogExampleForm(): System.Windows.Forms.Form
{
    this.openFileDialog1 = new openFileDialog();
    this.folderBrowserDialog1 = new FolderBrowserDialog();
    this.richTextBox1 = new RichTextBox();
    this.openFileDialog1.DefaultExt = "rtf";
    this.folderBrowserDialog1.Description = "Select the directory you want to use as default";
    this.folderBrowserDialog1.ShowNewFolderButton = false;
    this.richTextBox1.AcceptsTabs = true;
}

p.s. In the video she says it's a real MSDN sample. Which blew me away! Looks like something a junior would write.

Ohhh, I hate when I find tons of commented old code just because some people didn't have the courage to delete it completely :-p

// Code that was used 100 years ago
// DoSomethingCoolV1();
// DoSomethingVeryCoolV1();
// DoSomethingExtremelyCoolV1();
...
// MagicV1();


// Code that was used 50 years ago
// DoSomethingCoolV2();
// DoSomethingVeryCoolV2();
// DoSomethingExtremelyCoolV2();
...
// MagicV2();


// Code that was used 5 years ago
// DoSomethingCoolV3();
// DoSomethingVeryCoolV3();
// DoSomethingExtremelyCoolV3();
...
// MagicV3();


// Deleted buggy code
// DoUglyBUg();

 DoSomethingCoolV4();
 DoSomethingVeryCoolV4();
 DoSomethingExtremelyCoolV4();
...
 MagicV4();

I hate dead old code commented. It make absolutely no sense. We all use source version control tools!!!

Code with stupid, pointless comments comes to mind:

public void doer() {
    for (int i = 0; i < 10; i++) {
        if (a == true) {
            ...
        } // end if
    } // end for
} // end doer

Yes, even on methods where the // end <something> comes only a handful away from the start.

Hiding the first line of code behind an opening brace:

if (isAdminUser())
{ launchNukes();
 showAdminLinks();
 showDangerousButtons();
}

I've seen code where this was done 100% of the time. Its always hard to read.

I hate it when there is no style--every engineer just does whatever he wants and the reader can tell which parts of a file were written by which engineers.

Space between function name and brackets. "Reversed" spacing around keywords - I'd use if (true) instead of if( true ).

fun1 () {
}

...

fun3() {
    if( fun1 ().fun2 ().fun3() ) {
    }

    while( true ) {
    }
}

This really hurts my eyes.

Leaving the brackets before a longer loop.

while(foo == bar)
    for(i = 0; i < count; i++) 
    {
        /* code */
    }

Or a bit more ugly

while(foo == bar)
    for(i = 0; i < count; i++) {
        /* code */
    }

One more:

while(foo == bar) for(i = 0; i < count; i++) {
    /* code */
}

The worst I saw was on some well known open source network code in C (I can't recall the software name): the author defines a bunch of pre-processor shortcuts (like W for while, and worse) and use it through the whole code...

I also saw an ex-Pascal programmer defining BEGIN and END to { and }...

This is one that continually blows my mind when I see it. I've actually come across this more than once.

if (A) {
    //Do something
}
else if (!A) {
    //Do somehthing else
}

Once I worked with a guy who insisted on right-justifying declarations in C++:

class X
{
    int       asome;
   long  bsomeother;
 string     astring;
};

eek!

Space between every single line makes my eyes bleed:

function doSomething( ... )

{

    foo();

    printf( "whatever" );

    foreach ( ... )

    {

        whatever();

    }

}

Super long comment blocks for super short functions

//**************************************************
// Method: getFooBarThing
//
// Description:
//    Get the Foo Bar thing
//
// Preconditions:
//    None.
//
// Postconditions:
//    None.
//
// Throws:
//    None.
//**************************************************
int Baz::getFooBarThing()
{
  return fooBarThing;
}

No joke. Just a little exaggerated.

PHP:

$out = "
<html>
    <head>
    </head>
    <body>
    <p>Yeah, you guessed it, whole pages of html inside a string, with <a href=\"mypage.php\">links</a>, variables like this: $txtOneContent -  and all without syntax highlighting because it's all in a damn string!</p>
    <p>Make one tiny and innocuous change, and the website explodes.</p>
    </body>
</html>
";
echo $out;

I really hate when people assign the same variable twice, for example:

DataSet ds = new DataSet();
ds = GetSomethingFromDB();

The first line is totally useless! who writes that doesn't realize that it will be overwritten in the next statement, it's wasting memory.

Putting gaps between an instance and method :

object     .     somemethod ("");

I once worked with a guy who prefixed every variable, function, method, module - everything - with a 'z'. Why? To indicate his authorship; 'Z' was the first letter of his first name. Worse, he would use names like zString1 and zString2.

Indian variant

if (myBool.ToString().Length>4){// do something

I could spam a whole page on this one. Pet hates include:

• Splitting declaration and assignment for no reason whatsoever. It really gets me confused when someone declares a variable and uses it 3 pages later. Apparently it's cool to declare all of your stuff in the same place regardless of where it's used... no. Stop that.

• Packing code onto the screen like it's 1980 and we're all using 320x240 screens. This usually involves no spacing when using operators. People who do this usually miss the point. If you want to take in the whole problem on one screen, break it down into nicely named subroutines and it will read nicely and increase comprehension.

• Crap or abbreviated naming (to the point where it reads like a text message).

• Re-using variables for multiple purposes. I think everyone has seen this one somewhere.

• Using comments where self-documenting / literal code would do the job. You just lost half of my screen space repeating yourself. Instead, call a function named after whatever the thing does, remove the comment.

• Magic numbers. Named constants are your friend, as are enums.

• Check multiple input conditions, resulting in the default nesting in a function being 2+ deep. Invert the ifs and return immediately, no nesting needed.

E.g. Potentially really messy:

if(someVal != null)
{
   if(someOtherVal != null)
   { 
       DoSomeStuff();

       ... 10 years later
   }
}

This is generally cleaner

if(someVal == null) 
    return;

if(someOtherVal == null)
    return;

DoSomeStuff();

I hate it when people declare/use undescriptive variable...

e.g.

string zp = string.Empty;
int n1 = -1;
List<object> xList = new List<object>();

Not necessarily a programming style, but more of a by product of being ignorant, or totally unaware of how to use the given IDE... I hate it when I open up a file and find crazy amounts of errant white space to the right of the line. Generally when I find this, I also find a mishmash of tabs and spaces. Example:

public string Test()
{
    code();                                       < 25 spaces or tabs here
    morecode();
}

My Java lecturer was obsessed with tabs, for which I hated him:

public void method() 
{
    for( int i = 0; i < 10; i++ )
        {
             if(someval)
                 {
                      print(whatever);
                 }

        }

}

The following drives me mad

If (evaluates_to_boolean) then return true else return false;

Or the highly contagious horizontal alignment:

const
MY_CONST_ONE    =  1;
MY_CONST_TWO    =  2;
MY_CONST_THREE  =  3;
MY_CONST_FOUR   =  4;
MY_CONST_TEN    = 10;

I'm saying it's contagious because I feel bad for breaking it, so I will tend to keep it up. Let's say, were I to come along and add a MY_CONST_TEN_THOUSAND_AND_SEVEN, I would most probably spend time aligning the rest of the code...

const
MY_CONST_ONE                      =     1;
MY_CONST_TWO                      =     2;
MY_CONST_THREE                    =     3;
MY_CONST_FOUR                     =     4;
MY_CONST_TEN                      =    10;
MY_CONST_TEN_THOUSAND_AND_SEVEN   = 10007;

It's a waste of time to make things align horizontally.

I'd recommend reading the book by Robert C. Martin's - Clean Code. An excellent book! Every programmer's must-read.

I saw this one in VB .NET and wanted to strangle somebody:

Select Case True

    Case SomeMethod()
        ' do some stuff

    Case SomeOtherMethod()
        ' do some other stuff

    Case YetAnotherMethod()
        ' do yet other stuff

End Select

I had to stare at it for a while before I understood the intent. And even then I was baffled at what would make anyone think it was preferable to

If SomeMethod() Then
    ' do some stuff

ElseIf SomeOtherMethod() Then
    ' do some other stuff

ElseIf YetAnotherMethod() Then
    ' do yet other stuff

End If

My guess is the programmer read somewhere once that the Select Case statement runs 3% faster than the If statement in VB .NET. That's usually how abominations of this sort arise.

EDIT: For the unconvinced, consider this:

Select Case True

    Case True
        Debug.Print("Case 1")

    Case False
        Debug.Print("Case 2")

    Case True
        Debug.Print("Case 3")

End Select

What will the output be?

The output will be "Case 1". To the novice this is far from obvious, and even to the experienced eye this is a bizarre way to write code. Select Case usually deals with exclusive cases, but it doesn't have to; it uses the first matching case and doesn't evaluate any others. So in our SomeMethod etc. example, if SomeMethod is True, the other two methods won't even be evaluated. Yes, if you're experienced, you'll know this, but that doesn't make it a good idea to write code in a non-obvious, non-idiomatic way.

The use of different human languages for identifier names gives me the creeps.

Intermixing double and single quotes in JavaScript, in the same piece of code:

var options = {
    foo: "Some text",
    bar: 'Copy paste is fun',
    wtf: "(" + x + ')',
    baz: "It's cool"
};

Yes, it's a convenience when your string literals contain quotes, but I prefer uniformity of style in code. It's not that hard (or relatively more unreadable) to escape quotes using "\".

Not indenting function blocks.

Like this:

void DoLotsOfStuff() {
int x,y,z,is_available,thread_not_ready;
double latitude,
          longitude,
          altitude;
if (0 == is_available) { printf("data not available"); }

// more stuff....
}

A co-worker (a student working on his diploma thesis) wrote code like this. Several functions up to 100 or 200 lines long. I made this example up, but it comes close to his style.

Building in-line SQL code (horrible in itself), or even worse, SQL code that's prone to SQL injection:

sql = "SELECT * FROM users where UName ='" + username + "' AND pw ='" + password + "';

I know a lot of people love this, but using "this." in front of every class variable drives me nuts.

The reason it's supposed to be used is to identify class variables. It's extremely bad at that since a missing "this." does not PROVE that it's a local variable.

Also, your GUI will color them differently. If you're not using a GUI that does, stop typing this. and go get a real editor! This kind of thing is why everyone keeps telling you to upgrade.

The real reason (and I can sympathize) is that programmers like consistency and it drives them nuts that you so often have to have this:

void func(String name) {
    this.name=name;
}

to avoid collisions. Personally I don't have a problem with that inconsistency, but if I did, I'd use this solution instead:

void func(String pName) {
    name=pName;
}

Relying on a manual process (like some programmer deciding to use "this." before every instance variable) is just going to lead to some time where you see a variable without "this." and ASSUME it's local. Use something more deterministic.

if (var = val) 
{
    someaction();
}
else {
    if (var = someOtherVal)
    {
        someotheraction();
    }
    else
    {
        if (var = yetSomeOtherVal) 
        {
        .
        .
        .

Sure it works but I actually saw this once and got and puked a little. In my mouth.

In C, I like omitting braces in single statements following ifs, whiles, fors, etc.

if (condition)
    DoSomething();

But some people take it too far.

i = 0;

if (i)
    if (i > 5)
        Something();
else
    SomethingElse();

In that code, SomethingElse() is never called.

Comments that come after the code they refer to. And boilerplate in comments that adds nothing to meaning. Example (not made up)

int count_;
// effects: The number of objects.

I would be interested to know if (a) other people have seen this style (b) anyone prefers it to the alternative. I think this originates in the C++ standard. In Working Draft, Standard for Progamming Language C++ from 2005-10-19, section 17.3, which lists the conventions used to describe the C++ library. It provides a list of attributes for functions. It includes Requires, Effects, Postconditions, Returns, Throws and Complexity. In the context of a library it makes sense to list a function prototype and then its characteristics. It makes no sense to me to do that in the code itself.

There is a style, unnamed as far as I know, in which all functions return a status code, and all calls to such APIs are placed inside an if-statement. An example is socket API calls in UNIX. For example, if all api calls return -1 on failure you may see something like this for a sequence of API calls:

if(api1() != -1) {
  if(api2() != -1) {
    if(api3() != -1) {
      // all three api calls were ok - do stuff...
    } else {
      // handle api3 error
    }
  } else {
    // handle api2 error
  }
} else {
  // handle api1 error
}

It seems like many people are completely comfortable with this. But I do not like the way it pushes the error handling out of the picture, moving it away from the API call it refers to. When you combine this style with selectvely ignoring some of the errors and omitting the brackets on one or more of the conditions, or where some additional work has to be done before one or more of the API calls, you have a style where a maintainer has to check for correctness every time they deal with it. Though more verbose I prefer

if(api1() == -1) {
  // error - escape somewhere, maybe return -1 to caller.
}

if(api2() == -1) {
  // error - escape
}

if(api3() == -1) {
  // error - escape
}

// All three calls succeeded - do stuff here.

To me the latter seems so transparent that I have no idea why anyone uses the former. Except maybe it makes the programming seem more exciting, as you arrive breathlessly inside all those conditional brackets ready to do the real work.

Amongst the many "programming styles" I've witnessed, the one I hate the most is:

the style that emerges spontaneously from the source code itself when many people 'fix' a bug and don't explain what they did

This can be shown with an example, coming directly from the code I'm debugging now (the person who originally wrote this left a while ago, and those who had to fix it can't remember what they did):

if ((a[i].Y - (((a[i].Y/b))*b)) < 75
{
    skipDiff += diff;
    counter++;
    continue;
}

If you look at it closely, it contains everything one might hate in the shortest amount of lines of code:

• uncommented hacks (the whole algebraic expression is a - a/b * b < 75 which is absurd, unless you realize that they are integers, thus the division is used to "round" something)
• uncommented values (75 WHAT?!? )
• over-use of parenthesis leading one to question his own mental health
• counter is the outer for loop counter; yes, we are incrementing it inside the for loop as well as in the for loop condition. Oh, did I mention that it is used as an array index as well?
• not only meaningless but also wrong variable names (I put 'a' and 'b' to avoid the customer spotting the code here, but the original variable names were something like _pageHeight -- which was not the page height, but the position you were currently at in the current page)

Also add that this code, albeit being on a version control system, has 2 revisions: the original checkin and the 'massive' revision that happened after a handful of people 'fixed' the code, which was on a single PC. And no revision comment was added.

Claustrophobic code:

int x;
for(x=0;x<10;++x){
    if(x==5) continue;
    dosomething(x);
}
while(unrelatedloop()){
    anotherbody();
    foo=bar;
}
int y=0;//comment
if(x==1) something();
if(y==1) something();

I need my breathing space! Furthermore, whitespace can be used to group related statements (vertical) or tokens (horizontal) together.

Not understanding Boolean logic.

// If the foo happens or if the foo doesn't happen but the bar happens instead
if (foo || (!foo && bar))

Equivalent to:

if (foo || bar)

Even worse when there are more than two terms!

For me it's code that's deliberately hard to read, whilst ironically the developer thought they were making it easier to read. I posted this code snippet as an answer to another question, but it's just as relevant here. Here's an exact copy of code from something I work on, all the comments (or lack thereof) as identical to how it appears in the source code:

function getAttributes(&$a_oNode)
{
    $l_aAttributes = $a_oNode->attributes();
    if ($l_aAttributes === NULL)
        return array();

    $l_iCount = count($l_aAttributes);
    $l_i = 0;

    for ($l_i = 0; $l_i < $l_iCount; $l_i++)
    {
        $l_aReturn[$l_aAttributes[$l_i]->name()] = $l_aAttributes[$l_i]->value();
    }

    return $l_aReturn;
}

function getText(&$a_oNode)
{
    $l_aChildren = $a_oNode->child_nodes();
    if ($l_aChildren === NULL)
        return NULL;

    $l_szReturn = "";

    $l_iCount = count($l_aChildren);
    for ($l_i = 0; $l_i < $l_iCount; $l_i++)
    {
        if ($l_aChildren[$l_i]->node_type() == XML_TEXT_NODE)
            $l_szReturn .= $l_aChildren[$l_i]->node_value();
    }

    if (strlen($l_szReturn) > 0)
        return $l_szReturn;

    return NULL;
}

Someone obviously didn't read Joel Spolsky's article on Hungarian notation ^[1]. This code just makes my head hurt.

Code where the original developer used the names of his old girl friends as variable or function names (or, alternately, German numbers). That's the worst I've seen.

MyObject o = new MyObject();
Debug.Assert(o != null);

MyOtherObject o2 = new MyOtherObject();
Debug.Assert(o2 != null);

Come on! I know John Robbins et. al. say use Debug.Assert, but you don't trust the runtime to create your object?

private static final int TWO = 2;

'private' no less ..

Tabbing identifiers

a_namespace::a_class  a
int                   b
char                  c

There is no reason to write code like that... I wonder what happens if you have already defined b and c and then you need a. Do you spend time tabbing all the other variables? Brr...

Putting brackets on the same line as the code to execute inside those brackets. Here's some actual code from a web app I inherited. I will forever hate the guy who wrote this, as well as understand how PHP can get such a bad name (when used improperly).

if($Submit == "Complete Install" || $Submit == "Save Report")
    {
    $dbStatus = "Open";
    if($Submit == "Complete Install")
    	{
    	if(trim($Desc) == "")
    		$Error = "Invalid 'Install Description' provided!";
    	else
    		$dbStatus = "Closed";
    	}

    if(trim($ContactID) == "")
    	$Error = "Invalid 'Site Contact' provided!";

    if(trim($ContactID) == "AddNew" && trim($ContactName) == "")
    	$Error = "Invalid 'Contact Name' provided!";

    if(trim($ContactID) == "Attach" && trim($AttachID) == "")
    	$Error = "Invalid 'Site Contact' provided!";

    if(trim($ProductID) == "")
    	$Error = "Invalid 'Product Serial' provided!";



    if($Error == "")
    	{

                //snip 150 lines

        }
    }

Also, note how register_globals is enabled (-_-). And this exact same logic is copied and pasted in at least 20 different files.

I've seen

#define ONE 1
#define TWO 2
#define THREE 3

etc

Coding that uses a mixture of GNU and K&R i.e.

int function() {
    if (condition) {
        if (another condition) {
            DoStuff()
            }
        else {
            DoSomthingElse() 
            }
        }
    }

Unfortunately the code I maintain with has this bracketing style.

She'd finish a line of code, she'd hit enter, hold space, and when the cursor gets "close enough" she'd start writing the next line of code there...

public func()
{
        int x = 0;
       double y = 0;
       string str = "";
           if(something)
             {
                 doSomething();
              andOtherStuff();
                }
              y = 8;
        x = 14;
        }

I worked with her on projects in school going through pages of her code. First things first, I'd have to make it readable... then I began adding my code to hers. It made it easy to track her changes though.

I "like" this one :

if (foo == bar)
{
  DoSomething();
}
if (foo != bar)
{
  DoSomethingElse();
}

But my favorite is :

bool b = bool.Parse("true");

Space between ( and the rest )

Usually something like this:

               if (true)
               {
                 if (thing = 7)
                 {
                   if (foo != 8)
                     Console.WriteLine("Don't delete me. I know what you did.");
                 }
                 else
                   do
                   {
                       It();
                   }
                   while(foo == 8)
               }
        else
        {
          switch otherThing:
           {
             case "A":
             Console.WriteLine("Your answer is not B!");
             break;
             default:
             Console.WriteLine(@"Your answer is not A!
             You fail at life. Everyone hates you.");
             break;
           }
        }

Stuff like this: Constant switches, loops, ifs and do-whiles, without a SINGLE LINE OF COMMENT. It just makes me want to scream at other coders.

I'm surprised that no one has mentioned this one. It's not as bad as brace misplacement, but I hate it when people put the one-line then branch on the same line as the if:

if(condition) dostuff;

It makes it MUCH more complicated to add debug statements, or set break points, and I never actually end up reading it correctly for some reason...

Screen real estate is NOT that expensive.

I've seen conditions written like these two snippets:

if(foo< bar) {
   ...
}
if(foo<= baz && baz!= 1) {
   ...
}

The developer wouldn't insert a space between left-hand variables and their comparators, saying he wasn't as interested in how the comparison was being done as much as which variables were being compared. This helped him better set apart the variables in the condition.

This was a very minor thing, but it utterly drove me up the wall every time I encountered it.

Multiple statements on the same line, e.g.

if ( condition ) { doThis(); doThat(); }

This takes readability down a couple of notches.

A very strange C line of code:

int x = 0;
    int y = x+++++x;    /* what the hek !*/
    printf("y = %d", y);

Actually, this line doesn't pass the compiler check in VS and devc++ 4.9, but I believe it passed in devc++ 4.

While Style is number 1, and functionality is number 2.

I'd probably been coding for 20 years and working in Perl for a couple years before I saw something like this in someone else's code:

$y = 2 if $x == 5;

I'd never seen this way of writing an if in any other languages, and it took a while before my brain stopped automatically assuming that everything after the assignment was a comment. I don't know of any other languages where this would be allowed. I found it weird.

Worst ever?

# TODO: Document This!

...exceptionally good when it's found at the top of every method declaration and at the top of the file.

I've been dealing with a large chunk of legacy code that's been written in bad Perl by somebody with a Unix shell background.

Because of the Unix background they've adopted the convention of using a zero return value as success. Everbody else in the Perl world this evaluates to false. Because of this you have variants of:

if (not $success ) {
  # happy path
} else {
  # failure
}

everywhere - mixed in with "normal" Perl libraries with the saner convention of false == failure, true == success.

Evil.

Extra empty lines in the source code, especially in the very beginning or end of the method/class:

public bool HasSomething()
{




     int a = this.A + this.dA;
     // and other method's code here

     return false;




}

C#-specific: use of #region directive for a very small code blocks:

public class MyLittleClass
{
    #region Fields

    private bool _isSomethingLoaded;

    #endregion

    // The rest of the class
}

Using ifdef...endif to 'branch' code from the main trunk

This rubbish:

#ifdef BUGFIX_1
   //some rubbish code
#endif
#ifdef BUGFIX_2
   //more garbage
#endif

This is rubbish for so many reasons:

• Lack of readability. You can't easily tell which code path to follow.
• Error prone when getting rid of the guards
• Doesn't work with the toplevel makefiles
• Makes code harder than it should be to manage.
• Easy to create broken builds.

¡uʍopəpᴉƨdn əpoɔ ƃuᴉpɐəᴚ

Having to stand on your head to read code:

sub main() {                                       sub 
                                                   uʍopəpᴉƨdn($);
    for my $input (reverse <>) {
        chomp $input;
        my    $ʇndʇno = uʍopəpᴉƨdn($input);
        say   $ʇndʇno;
    }
}

1) unnecessary javadoc for every instance variable and method. for e.g. see below

/**
 * The logger instance.
 */
private final Logger logger = Logger.getLogger(BasicCollector.class);

/**
 * The collection backing this collector.
 */
protected Collection collection;

/**
 * The ordered list of sort criteria for this collector.
 */
protected List sortCriteria;

2) passing the instance properties of the same class to instance methods...like below

class A
{
 B bRef;

void foo()
{
bar(bRef);
}

void bar(B anotherRef)
{
}
}

Verbose naming conventions:

class Item {
    int idOfTheItem;
}

Mixing different languages:

String UserNomDeFamille;

Not to mention that Java allows UTF-8 in source code. You can see horrors like these:

public void SetDépart(Date départDate) {
    ....

Naming functions 'consistently' by calling them...

db_r_123
db_w_276
db_r_322

The most frustrating style I ever encountered was a colleague's compiler. It was page upon page of code without any indentation whatsoever; every single line started in the left column so there were next to no visual cues. There were several hundred files like this:

IF (condition1
AND (condition2
OR condition3))
THEN
BEGIN
dothis();
END
// some comment here
ELSE
BEGIN
dothat();
END;

Maybe I'm exaggerating the number of files and lines, but not by much. (It was in a language called UFO^[1], so I couldn't just automatically reindent without significant work to make an emacs mode to support that obscure language. The language had a number of other problems, including an implementation of strings based on unbalanced trees that was so boneheaded that it turned a fast compiler into something that ran slower than molasses in Antarctica.)

^{[1] I don't really remember that much about it other than what I've written above. What I do remember is that the code got rewritten in Java 1.0 and suddenly went enormously faster despite being theoretically less elegant.}

Blocks from people who are too much in love with Pascal:

if (condition == true) {
    code...
} //END IF condition == true)

Functions laid out like this:

bool SomeMethod
(
   int arg1,
   int arg2,
)
{ 
   // some code
}

Amazingly, my pet peeve coding style isn't listed here.

And that is - people who still format their code for 80 columns.

Microsoft STL code has horrible indentation. This is from <hash_map>, and the entire STL is written the same way. They also seem to rely on comments in lieu of good names for things (_Mfl? _Kty? _IReft?).

        // TEMPLATE CLASS _Hmap_traits
template<class _Kty,    // key type
    class _Ty,  // mapped type
    class _Tr,  // comparator predicate type
    class _Alloc,   // actual allocator type (should be value allocator)
    bool _Mfl>  // true if multiple equivalent keys are permitted
    class _Hmap_traits
        : public _STD _Container_base
    {   // traits required to make _Hash behave like a map
public:
    typedef _Kty key_type;
    typedef _STD pair<const _Kty, _Ty> value_type;
    typedef _Tr key_compare;
    typedef typename _Alloc::template rebind<value_type>::other
        allocator_type;

    typedef typename allocator_type::pointer _ITptr;
    typedef typename allocator_type::reference _IReft;

    enum
        {   // make multi parameter visible as an enum constant
        _Multi = _Mfl};

    _Hmap_traits()
        : comp()
        {   // construct with default comparator
        }

    _Hmap_traits(const _Tr& _Traits)
        : comp(_Traits)
        {   // construct with specified comparator
        }

    class value_compare
        : public _STD binary_function<value_type, value_type, bool>
        {   // functor for comparing two element values
        friend class _Hmap_traits<_Kty, _Ty, _Tr, _Alloc, _Mfl>;

    public:
        bool operator()(const value_type& _Left,
            const value_type& _Right) const
            {   // test if _Left precedes _Right by comparing just keys
            return (comp(_Left.first, _Right.first));
            }

        value_compare(const key_compare& _Traits)
            : comp(_Traits)
            {   // construct with specified predicate
            }

    protected:
        key_compare comp;   // the comparator predicate for keys
        };

    static const _Kty& _Kfn(const value_type& _Val)
        {   // extract key from element value
        return (_Val.first);
        }

    _Tr comp;   // the comparator predicate for keys
    };

Using spaces instead of tabs for indentation. Yeah, I know it's contentious, but if we all use tabs, then everyone can see the code indented at their preferred width. If we use spaces, then it's up to whoever wrote the code originally.

Everyone writes a summary of their changes at the top of the file for every commit. It's especially funny because we use ClearCase and all of the bureaucracy built into it is completely ignored. That is, every checkout/checkin/commit/deliver message is empty.

Staggered Indentation

Indentation for showing off public-vs-private in Java:

        private final static String
    boundary_after_not_word      = "(?="  + identifier_charclass + ")";

        private final static String
    not_boundary_after_word      = boundary_after_not_word;

public final static String
    precedes_word                = boundary_after_not_word;

        private final static String
    boundary_after_word          = "(?!"  + identifier_charclass + ")";

        private final static String
    not_boundary_after_not_word  = boundary_after_word;

public final static String
    not_precedes_word            = boundary_after_word;

        private final static String
    boundary_before_not_word     = "(?<=" + identifier_charclass + ")";

        private final static String
    not_boundary_before_word     = boundary_before_not_word;

public final static String
    follows_word                 = boundary_before_not_word;

        private final static String
    boundary_before_word         = "(?<!" + identifier_charclass + ")";

        private final static String
    not_boundary_before_not_word = boundary_before_word;

public final static String
    not_follows_word             = boundary_before_word;

OMG ^[1], I just saw this real example for declaring a variable:

gv_240407           TYPE c,                       "RM 24.04.07

gv_230507           TYPE c,                       "RM 23.05.07

gv_180208           TYPE c,                       "RM 18.02.08

gv_040308           TYPE c,                       "RM 04.03.08

gv_170408           TYPE c,                       "RM 17.04.08

So smart, isn't it ? Grrr

Some more vertical alignment:

void    print_array     (   int const     *arr            ,
                            int const     size            );
void    unite_2_arrays  (   int const     *arr1           ,
                            int const     size1           ,
                            int const     *arr2           ,
                            int const     size2           ,
                            int           *arr_1_and_2    );

Using extra whitespace between code blocks like this:

public void doSomething()
{
   //Do something:


   int i = 0;
   foo();


   blaat();
}

camelCase. it'sUtterlyInexcusablyRetarded.SeeHow"Easy"ThatIsToRead.

In TSQL code I do not like when placing inner join after the table

Select 
* 
from
table1 inner join
table2 on table1.id=table2.tbl_id1

For me it is much easier to read code looks like this

select 
*
from table1
inner join table2 on table1.id=table2.tbl_id1

Trying to make Visual Basic comments look like C, C++, C# comments, etc.:

'//My comment

'//My other comment

This kind of fall more in the 'making me laugh' category.

Can't stand this:

if (condition) {
    # ...
}

Not sure why, either. I've always had a new line for braces. I also always keep a new line for return, so this also bothers me:

function a($x)
{
    $y = $x * $x;
    return $y;
}

Macro-filled code where 99.9% of lines of code use at least one if not more macros making the code a language unto itself mostly.

I did have this with my first out of school job where this is what the server developer did that he thought was a good idea. It eventually got to be OK, but by then I had spent a couple of years in it and was the most senior employee that ran it.

I agree with most of the above: hatred of inconsistent indenting, bad variable names, etc. I also hate longer than 80 column lines. I like to have 2 open windows at a time, and always having them be 80col means that I know how big a line I'm working with.

Also: multiple statements on one line:

i = 3; j = 5; k = 8;

And for some reason, I can't stand indented braces:

if(test)
  {
    code...
  }

erph.

Putting statements on multiple lines, especially when it's really not necessary, as in this case:

if (bOne &&
    bTwo &&
    bThree &&
    bFour &&
    (!bFive ||
    !bSix))
{
   // do something
}

And it's even more ugly counterpart:

if (bOne
    && bTwo
    && bThree
    && bFour
    && (!bFive
    || !bSix))
{
   // do something
}

I absolutely hate it when people don't format their SQL in a readable, consistent way. The "L" stands for language, people!

Also, why do people insist so strongly on ALL-CAPS'ing SQL code? It doesn't really serve any purpose I know of other than making it harder to read.

A former colleague of mine insisted on using getter and setter methods for all member variable access - inside the class that owns the variables.

And to make it worse he always made the getters & setters public, even when they didn't need to be.

public Collection getMappingsBySponsor(String Mgr, String spons)throws SQLException, ClassNotFoundException, Exception
{
    StringBuffer sql = new StringBuffer(this.INV_NAME) ;
    sql.append("= '") ;
    sql.append(Mgr) ;
    sql.append("' AND ") ;
    sql.append(this.NAME) ;
    sql.append("= '") ;
    sql.append(spons) ;
    sql.append("' ORDER BY ") ;
    sql.append(this.SHORT_NAME) ;

    return getAMappings(sql.toString()) ;
}

There are better ways to do it, and it was like all functions written with this kind of method.

In C/C++ I always did this:

if (a == b)
{
  // do stuff
}
else
{
  // do different stuff
}

Years of Java had it beaten out of me. Instead it's now:

if (a == b) {
  // do stuff
} else {
  // do different stuff
}

I've learnt to accept that and that's how I write my Java now.

What I absolutely cannot stand however, what absolutely drives me mental is this:

if (a == b) {
  // do stuff
}
else {
  // do different stuff
}

Joe Caldwell -MTG 2009

Reinvent EVERYTHING

Rewrites all of swing because "its too slow" and "has no features"

Symbian C++ coding. Too many types of "strings". So many classes and it doesn't look like C++.

The PHP programming language..

$dollar_variables

Variables/constants/classes with too much scope: global variables which should be local to a file, file scope which should be class scope, etc.

Leads to weird names to avoid naming collisions, and to maintenance nightmares.

Ehh, it's questionable if this response truly ties in to the question you're asking, but I'll offer it anyway. I hate when multiple users edit the same code in different IDE's such that the tab delimiters are different.

For example, one developer might use Eclipse, such that when he presses the TAB key, it inserts a single tab whitespace character. Another developer them might use vi, such that when he presses the TAB key, it inserts 2 single space whitespace characters.

This might look fine to the vi developer, assuming vi is set up to display tab characters such that they span 2 fixed width characters. However, Eclipse might have tab characters displaying such that it spans 4 fixed width characters, resulting in code appearing like this:

    if ($user->save()) {
    $email = EmailManager::generate('register', $user);
        $email->setSubject('Welcome to Example');
      $email->setBcc(array('support@example.com'));
        $email->setTo(array($user->getEmail()));
    $email->send();
  }

So frustrating!

• Everything (functions, variables) declared global or public.
• Functions that are over two pages long.
• Code that is obviously copy-pasted.
• Actual production code that has the "My-" prefix in a variable or function.

Supplemental:

• Implementation directly in an event function rather than in a helper function, and....
• Firing an event from code to have its handler execute.

For these last two, allow me:

Suppose you have some C# for a menu item to print a document:

private void PrintMenuItem_Click(object sender, EventArgs e)
{
    //Code all the printing code here. Formatting, etc.
}

Then you need a button too. No problem, we already have that code!

private void PrintButton_Click(object sender, EventArgs e)
{
    PrintMenuItem_Click(sender, e);
}

Session variable cowboy programming

Especially the kind that doesn't use method params but uses session objects as their own personal litter box.

public GetData()
{
  //blah variable from 5 pages ago. Could have been changed in previous pages.
  r = Session["blah"]
  var c = new HelperClass();
  var h = c.GetMoreData();
}

class HelperClass
{
  public string GetMoreData()
  {
//no friggin idea where mm or n is set. Generally changed in class instance. 
       return Session["n"].ToString + Session["mm"].ToString
  }
}

Just worked on a project like this and lost some more hair..

In Java, prefixing variable names with m_ and s_ to distinguish between member and static class variables. Every modern IDE allows you to differentiate them.

It drives me absolutely crazy when someone does this:

if ( /* condition */ ) { /* proceed to do your code in this same line */ } else { /* other code, also in this same line */ }

Perhaps this is fine for some people, but I have to have my resolution very low due to poor eye sight and having to scroll to see the entire if statement makes me want to quit sometimes.

Code with little or no comments.

I've worked on far too many projects that had zero comments. Creative laziness is a virtue, but slothful laziness is unforgivable.

This is not a style per se, but more of an illness.

Too much safety against NullReference exception or check for null before doing anything with ANY variable:

if (foo != null) {
    foo.something();
}

What if foo is null? Where is the condition to handle that?? Bothers the hell out of me. Imagine a missile launch control system coded this way:

//DEFCON 1
if (missile != null) {
    if (targetPackage != null) {
        missile.setTargetPackage(targetPackage);
    }
    missile.launch();
}

Right aligned SQL:

    SELECT emp.*, dept.*
      FROM employee emp,
           department dept
     WHERE emp.dept_key = dept.dept_key
       AND emp.active = 'Y' 

SQL with capital identifiers:

    SELECT EMP.*, DEPT.*
    FROM   EMPLOYEE EMP,
           DEPARTMENT DEPT,
    WHERE  EMP.DEPT_KEY = DEPT.DEPT_KEY

NATURAL JOIN:

    SELECT EMP.*, DEPT.*
    FROM   EMPLOYEE EMP
    NATURAL JOIN DEPARTMENT DEPT

ANSI SQL Quoted identifiers

    SELECT "EMP".*, "DEPT".*
    FROM   "EMPLOYEE" "EMP",
           "DEPARTMENT" "DEPT",
    WHERE  "EMP"."DEPT_KEY" = "DEPT"."DEPT_KEY"

MySQL quoted identifiers:

    SELECT `EMP`.*, `DEPT`.*
    FROM   `EMPLOYEE` `EMP`,
           `DEPARTMENT` `DEPT`,
    WHERE  `EMP`.`DEPT_KEY` = `DEPT`.`DEPT_KEY`

Using the double quote as string delimiter in MySQL:

    SELECT "I just love C strings too much to give up on my double quotes";

Or this one:

if(someThingWasTrue)
{
    DoSomeThing();
    Print();
}
else
{
    DoSomeOtherThing();
    Print();
}

Print() function must be written after the if else block.

Writing C style code in Java, and not making proper use of the more advanced Java data structures, I/O and currency support provided with Java, like the Collections, NIO and the Concurrency classes.

Arrays are fine, up to a point, but can make algorithms harder to write correctly, standard I/O can less inefficient for file transfer than file channels (e.g. Guava file transfer code), and plenty of synchronized/Thread based code is less safe and efficient than Concurrency classes.

The worst I've seen is Yours...

Redundant comment headers, like the name of a the file in the comment.

// ******************************************************************************
//
// PROJECT:      name of proj
//
// MODULE:       Name of module
// FILE:         Ishityounot.cc
// AUTHOR:       Me
// DATE WRITTEN: the date I started writing this tome.

Making each programming statement one line, regardless of how complex the logic of the statement or the length of the statement.

• Mixing of languages:

SetLargeurAndHauteur

• Abbreviations:

UpdWdth4AllObj

• Hungarian*:

objMyObject

*however I accept Hungarian for Controls à la btnMyButton

• Long ones

<

MyClass.cmbAgent.Init(cnnSQL, objColDefCollection, Infragistics.Win.UltraWinGrid.AllowAddNew.No, Infragistics.Win.DefaultableBoolean.False, Infragistics.Win.DefaultableBoolean.False, Infragistics.Win.UltraWinGrid.SelectType.Single, "NAME", , MyClass._Formulaireappelant, MyClass._Traduction)

When my boss, who 'used' to write code, tells me (who does), how to write code.

Python code indented only 2 spaces.

I guess those people hate that Python forces indentation; but just accept it if you use the language. 4 space indents are much more readable. Just set your editor to that and hit tab.

A really short script is OK (I guess) but when it's a 2,000 line file, it's a nightmare to try to read.

Indentation fail

I know someone who indent his code like this

    if(){
foo();
            }else{
        another();
        }

It looks like he is coding without knowing what he's doing.

And one of the teachers of my college indent his else-if like this

if(){
    //this
}
    else if(){
        //that
    }
        else if(){
            //then this
        }
            else if(){ 
                //...
            }

Horrible Java teacher.

Not actually frustrating, but it's a little hard to read. Here's some side effect of what we call "dynamic programming".

In groovy & grails:

// function in controller class
...
def helloSir = {
  "Hello, sir"
}

def helloMadam = {
  "Hello, madam"
}

...

// call in controller
def test = {
   def input = "Madam"
   render ("hello" + input)()
}

Result: "Hello, madam"

It works.

PHP with lots of sporadic include's and iframes.

Lots of things:

• Spaghetti coding
• Putting data access logic and/or business logic in UI button click handlers
• Programming on concrete stuff instead of interfaces
• Copying and pasting code all around instead of creating reusable modules
• Sticking to 10 year old technologies and refusing to keep up with modern stuff (still using datasets and refusing to learn LINQ and any ORM technology like EF ^[1] or LINQ to SQL ^[2])

This is just a small portion of the stuff I can't stand, and all of this, and more, is regularly done by my only colleague.

Monolithic design.

This one is specific to VB... I really hate multi-line statements - one of those legitimate features that should not be used. For example:

ErrorMessage = "Lorem ipsum dolor sit amet, " _
             & "consectetuer adipiscing elit. " _
             & "Donec pharetra arcu et urna. " _
             & "Vivamus vehicula leo in dui."

Of course, the main problem is not those underscores and not ampersands at the beginnings of the lines. They are eyesores, but the main problem is inability to comment one of those lines...

In C#, using static readonly instead of const for absolutely no reason. Seen many times. Does static readonly look cooler, somehow?!

I'm not one for SESE at all, but this style bothers me when there is only one condition to check:

void Foo(bool bar)
{
    if(!bar)
        return;

    // rest of the method
}

I much prefer this:

void Foo(bool bar)
{
    if(bar)
    {

        // rest of method

    }
}

Languages that force the programmer to insert braces {} after anything like if, while, for... are pretty frustrating (e.g. Perl) when only one statement follows.

Especially for people coming from C, C++.

The programmer should be given the choice to insert or no the {} braces when she estimates that being relevant.

I'm not sure I've seen this one listed, but this "style" is aggravating: Essentially, calling a function, then checking some value not changed by the function, only to realize that the initial function was not the right one to call, so call another after:

int foo = bar();

if (snafu)
{
    foo = bar2();
}

instead of:

int foo;

if (snafu)
{
    foo = bar2();
}
else
{
    foo = bar();
}

I really hate it when people create functions for simple procedures within classes;

private int getMaxValue(){
    return *std::max(m_values.begin(), m_values.end());
}

private someFunction(){
    int value = this->getMaxValue();
    value *=-1;
    etc();
}

or something like;

void createAndAddOpenButton(){
m_openButton = new JButton();
m_basePanel.add(m_openButton);
}

etc

I agree with some of the other posters. Renaming variables is just a waste of time.

Here's an example in PHP:

$user_details = mysql_fetch_assoc($user_details_query);

$firstname = $user_details['firstname'];
$lastname = $user_details['lastname'];
$email = $user_details['email'];
$phone = $user_details['phone'];

Surprisingly I've seen several programmers code this way. It baffles me that they can't use array variables.

Using undescriptive short-hand names for the arguments to main:

int main (int argc, char *argv[])
{
    //..
}

I always make them fully descriptive:

int main (int argumentCount, char *argumentList[])
{
    //..
}

This...

if(condition)
    {
        code
    }

Should be...

if(condition){
    code
}

Switch statement extra indentation. Microsoft Visual C++ typing "whatever-it-is-called" tools lead to that extra indentation, Emacs/XEmacs does not do this, and IMHO this is superfluous indentation without any significance. Though, I may be biased from many programming hours spent in front of Emacs/XEmacs some years ago...

BAD:

switch( foo ) {
    case kFoo:
        DoFoo();
        break;
    case kBar:
        DoBar();
        break;
    default:
        DoDefault();
        break;
}

GOOD:

switch( foo ) {
case kFoo:
    DoFoo();
    break;
case kBar:
    DoBar();
    break;
default:
    DoDefault();
    break;
}

I know, some people may disagree, but IMHO a switch is a switch, it is no conditional and it is no loop construct. That unnecessary indentation just helps monitor vendors to increase their sales numbers.

Ternary operators of any kind.