Coding Horror

Earthhorse · 21-06-2011 3:28pm

I would have said better loop variable names would be more needed than comments there.

fasty · 21-06-2011 3:32pm

zynaps nails it. Flattening out the logic, sorting the data, batching.

Comments, I don't think matter as much at what the code says, or doesn't say :d

Pygmalion · 23-06-2011 10:34pm

Odaise Gaelach wrote: »

This is probably a stupid question, but... what does it do?

It was the last part of some competition question, can't remember the exact question but it was something along the lines of:
You're given a 2-d grid with a bunch of rectangles (ID'd by a letter) laid down on it.
Each position is either '.' (for an empty square) or a letter representing the top-most rectangle covering that square.
Print the order in which the rectangles were laid down.

The code doesn't even work properly (in case you hadn't guessed), but it got 4/10 of the test cases working, which is better than if I hadn't bothered, and after tidying it up afterwards and using proper sorting when I wasn't pressed for time the same basic algorithm got 7/10.

Lack of comments and untidiness doesn't really bother me as it was something I was never going to look at after the day in question, but that particular part stuck out as being much worse than the rest :P.

Calina · 28-06-2011 6:42pm

this is not directly code per se but....

Please DO NOT GIVE ME CODE TO REVIEW IF YOU HAVEN'T FINISHED WRITING IT FOR CRYING OUT LOUD. It's not like I've nothing to do. Give me code to review when it's Finished. Ready to go to the UAT. Not while you're still tinkering around with it.

srsly78 · 04-07-2011 2:42pm

Code is never finished. Surely a code review is supposed to catch architectural bugs before it goes to UAT?

Sparks · 04-07-2011 2:55pm

Code review isn't supposed to be looking for architectural bugs, but coding bugs. Lower-level stuff. A code inspection or walkthrough might look at architectural bugs, but mainly those are the responsibilty of design review.

Of course, in most Irish SMEs and startups, there simply isn't any code review, at least not formally or regularly...

conor.hogan.2 · 04-07-2011 5:10pm

I'm sure its been mentioned but I hate badly formatted code, where the {} braces etc are all out of sync so you have no idea where to add or remove code without first sorting it out.

fasty · 04-07-2011 5:24pm

I can't think of a single IDE that won't auto format code for you these days.

Evil Phil · 04-07-2011 5:26pm

conor.hogan.2 wrote: »

I'm sure its been mentioned but I hate badly formatted code, where the {} braces etc are all out of sync so you have no idea where to add or remove code without first sorting it out.

Agreed, companies need a coding standards document and everybody should be complying with it.

zynaps · 04-07-2011 8:21pm

Evil Phil wrote: »

Agreed, companies need a coding standards document and everybody should be complying with it.

Yeah.. otherwise, you or me or somebody else will (auto)re-indent the offending files and make it difficult to use diff tools with source control.

A: "Hey, how come it fails that test now? Did someone change the mcguffin traversal filter?"
B: "Just do a diff against a week-old revision? Someone probably changed one line that broke everything."
A: "Ok here we go, looks like... oh, the entire file is different because it got re-indented at some point. GIVE ME STRENGTH....."

fasty · 07-07-2011 3:41pm

I found some great C# abuse going through some of my own code. I must've been REALLY bored that day. Names of stuff changed of course!

Anyway, before I condemned it to exist only in Mercurial's history, I thought I'd share!

private class BaseThing { }
private class SomeThing : BaseThing { }
private class OtherThing : BaseThing { }

private T DoSomethingDumb<T>(BaseThing item, Func<SomeThing, T> somethingHander, Func<OtherThing, T> otherThingHandler)
{
    if (item is SomeThing)
    {
        return somethingHander(item as SomeThing);
    }
    else if (item is OtherThing)
    {
        return otherThingHandler(item as OtherThing);
    }

    throw new NotSupportedException();
}

Usage

BaseThing thing = new SomeThing();
            string fail = DoSomethingDumb<string>(thing, (someThing) => "something fails", (otherThing) => "otherThingFails");

unknownlegend · 13-07-2011 6:52pm

Was looking through some c# I had written earlier in the year. I found this particularly verbose hacked together regular-expression-esque that I thought I'd share:

            // Remove pre-processor cobolLines ^[ ][ ][ ][ AIS^D][ AIS^D][ AIS^D][ ][.]*
            if ((cobolLine != null && cobolLine[0] == AsciiSpace && cobolLine[1] == AsciiSpace && cobolLine[2] == AsciiSpace) &&
                (((cobolLine[3] == 0x41 || cobolLine[3] == 0x49 || cobolLine[3] == 0x53 && cobolLine[3] != AsciiSpace && cobolLine[3] != 0x44) &&
                  (cobolLine[4] == 0x41 || cobolLine[4] == 0x49 || cobolLine[4] == 0x53 || cobolLine[4] == AsciiSpace && cobolLine[4] != 0x44) &&
                  (cobolLine[5] == 0x41 || cobolLine[5] == 0x49 || cobolLine[5] == 0x53 || cobolLine[5] == AsciiSpace && cobolLine[5] != 0x44)) ||
                 ((cobolLine[3] == 0x41 || cobolLine[3] == 0x49 || cobolLine[3] == 0x53 || cobolLine[3] == AsciiSpace && cobolLine[3] != 0x44) &&
                  (cobolLine[4] == 0x41 || cobolLine[4] == 0x49 || cobolLine[4] == 0x53 && cobolLine[4] != AsciiSpace && cobolLine[4] != 0x44) &&
                  (cobolLine[5] == 0x41 || cobolLine[5] == 0x49 || cobolLine[5] == 0x53 || cobolLine[5] == AsciiSpace && cobolLine[5] != 0x44)) ||
                 ((cobolLine[3] == 0x41 || cobolLine[3] == 0x49 || cobolLine[3] == 0x53 || cobolLine[3] == AsciiSpace && cobolLine[3] != 0x44) &&
                  (cobolLine[4] == 0x41 || cobolLine[4] == 0x49 || cobolLine[4] == 0x53 || cobolLine[4] == AsciiSpace && cobolLine[4] != 0x44) &&
                  (cobolLine[5] == 0x41 || cobolLine[5] == 0x49 || cobolLine[5] == 0x53 && cobolLine[5] != AsciiSpace && cobolLine[5] != 0x44))) &&
                (cobolLine[6] != AsciiAsterisk))
            {
                cobolLine = null;
                WriteCSV("Pre-processing", filePath, currentLineNumber, currentColumnNumber, originalLine, cobolLine);
            }

It's simple, but the horror!

Also, I made use of some syntactic sugar in the same class which I think led to the chief architect banning operational shorthands for a while... :cool:

this.lengthHeader = BitConverter.ToInt16(pclength, 0) < 4094 ? 2 : 4;
...
this.loadedrecords / (ts.TotalSeconds > 0 ? ts.TotalSeconds : 1),
...
return _instance ?? (_instance = new Context());
...
this.byteSize = (this.storageType == StorageType.Packed) ? (this.Length + 2) / 2 : this.Length;
...
etc

I find it easy to write it at the time, but it trying to explain it to someone 6 months later means I'll be writing it in all it's verbose glory in future...

Father Todd_Unctious · 02-08-2011 11:46am

asp.net code

// this fires when the page is loaded
protected void Page_Load(object sender, EventArgs e)
{......

followed by 2.5k lines of spaghetti code with NO COMMENTS

Odaise Gaelach · 21-11-2011 2:14pm

A friend of mine is working in a group developing a Java application. Their comments are often deleted by the manager, who claims it's to keep the code files neat and tidy.

Sometimes I'm delighted to be working by myself.

Placebo · 15-12-2011 7:05pm

one of my clients had an ie CSS file, i opened it to find just this comment

Defiler Of The Coffin · 18-12-2011 8:15pm

For those who are nostalgic for Pascal...

#define begin {
#define end }

ocallagh · 21-12-2011 11:19am

[PHP]//@todo log error[/PHP]

srsly78 · 21-12-2011 11:49am

VB code:

On Error Resume Next

Colonel Panic · 21-12-2011 1:09pm

srsly78 wrote: »
VB code:
On Error Resume Next

I've never seen a VB codebase without that in it many, many times!

The funny thing is, if you search Stackoverflow, you'll see loads of people trying to implement that in C#

km991148 · 21-12-2011 3:27pm

If you really must have big long inheritance chains, can you try and keep the ctor params in the same order..

Why!?!:

public class A : B
{
  protected A (X, Y, Z) : (Y, Z, X) { }

}

public class B : C
{
  protected B (Y, Z, X) : (Z, X, Y) { }

}

public class C
{
  protected B (Z, X, Y) : (X, Y, Z) { }

}

Colonel Panic · 21-12-2011 3:46pm

As if deep inheritance wasn't fail enough!

km991148 · 21-12-2011 3:49pm

Colonel Panic wrote: »

As if deep inheritance wasn't fail enough!

exactly.. that was just an example.. the chains run 5-7 classes deep.. arrggghhh!

oh well.. Christmas soon...!

Colonel Panic · 21-12-2011 4:15pm

Reminds me of the OGRE graphics engine. It's a rabbit hole of inheritance and abstractions with added global variables masquerading as singletons for good measure!

G-Money · 23-12-2011 10:10am

Currently smashing my head off a wall as I don't understand jQuery and can't read the syntax.

Ah well, at least it's not Perl!

amen · 24-12-2011 2:58pm

Currently smashing my head off a wall as I don't understand jQuery and can't read the syntax.

Lack of familiarity with a language is not a coding horror. Some like
#define one 1;
#define two 1;

would be!

azzeretti · 29-12-2011 2:24pm

G-Money wrote: »

Ah well, at least it's not Perl!

Yep - as you'd have figured out by now how brilliantly simple and genius perl acutally is

#!/bin/perl
print ''=~('(?{'.('^)@./~'^'.[)@[^').'"'.('[$$^)^//@~/(@.+.~^^)@~)+@^._'^':^^;[;[[)^[@)@@]^.;[,^[^,;]}').',$/})');

bada_bing · 29-12-2011 2:48pm

G-Money wrote: »

Currently smashing my head off a wall as I don't understand jQuery and can't read the syntax.

Ah well, at least it's not Perl!

in fairness , it's not too bad actually. at the start of the year i didn't know any javascript at all and picked it up quite fast in a couple of weeks. You need to learn javascript before dealing with jQuery. The reason for that is JQuery is essentially a javascript library with programmed functions to make programming in javascript easier so that you don't have to write long javascript functions to do simple things.

i'd recommend playing around with javascript for a few weeks and then you should be able to appreciate jQuery, i'd be lost without it!!!!

Creamy Goodness · 30-12-2011 3:57pm

looking at a friends site, as a nixer and came across this gem.

//functions.php
$true = 'true';
$false = 'false';

//index.php

include 'functions.php';

if($iphone == $true) 
{
    // load iphone stylesheet
}

wtf?

zynaps · 30-12-2011 4:04pm

Creamy Goodness wrote: »
if($iphone == $true) 
{
    // load iphone stylesheet
}
wtf?

It's a minimalistic stylesheet

Genghiz Cohen · 06-01-2012 11:50am

Assert.AreSame(0, 0);
Fails, spend half a day trying to figure out why....

Should have been using:
Assert.AreEqual(0, 0);
RTFM...

dnme · 15-01-2012 9:05am

DELETE * FROM tbl_Customers

What?

WHAT??

WHAT DO YOU MEAN BY A "Where Clause".....???

k.p.h · 02-02-2012 9:13pm

Jesus, 10 min of my life wasted and I have no one to blame, I am a tit..!

if (pts < pts-buffer)

Korvanica · 16-02-2012 11:21am

string[] SupervisorArray = new string[1];
int i = -1;

while (RS2.Read())
{
    i++;
    SupervisorArray[i] = RS2["SignedByEmplNo"].ToString();
}

:eek:
Worst piece of coding i've seen in a long time.

zynaps · 16-02-2012 11:57am

Korvanica wrote: »
string[] SupervisorArray = new string[1];
int i = -1;

while (RS2.Read())
{
    i++;
    SupervisorArray[i] = RS2["SignedByEmplNo"].ToString();
}
:eek:
Worst piece of coding i've seen in a long time.

Here's a loop-unrolled optimised version

RS2.Read();
string Supervisor = RS2["SignedByEmplNo"].ToString();
if(RS2.Read())
  throw new ArrayIndexOutOfBoundsException("YAAAAY!!!!");

Defiler Of The Coffin · 23-02-2012 11:05pm

https://www.youtube.com/watch?v=H4YRPdRXKFs

Boards.ie: Paddy · 03-03-2012 1:33pm

Thought I'd share a real beauty from the boards codebase. This was one of the first things I read when I was taking over from Conor. I'm yet to understand how anyone could ever have thought that this was a good idea:

/**
* Essentially a wrapper for the ternary operator.
*
* @deprecated	Deprecated as of 3.5. Use the ternary operator.
*
* @param	string	Expression to be evaluated
* @param	mixed	Return this if the expression evaluates to true
* @param	mixed	Return this if the expression evaluates to false
*
* @return	mixed	Either the second or third parameter of this function
*/
function iif($expression, $returntrue, $returnfalse = '')
{
	return ($expression ? $returntrue : $returnfalse);
}

I guess some people just find ternary operators too confusing?

Sparks · 03-03-2012 1:39pm

Ugh. Love the @deprecated note's subtle way of saying "cop the blank on"

Boards.ie: Paddy · 03-03-2012 2:22pm

Oh the WTFs pile up the longer your stare at this...

"Essentially" a wrapper? No, it's "just" a wrapper.

$expression isn't an expression. It's a variable.

$expression is expected to be a string... so if used strictly this function only evaluates the truthiness of strings.

The default false value isn't FALSE, it's an empty string?!

A small window into the joys of working with the vbulletin core.

Sparks · 03-03-2012 3:40pm

Even better, in C++ any decent compiler would just optimise it away, but in PHP that may not hold...

DaSilva · 05-03-2012 9:16pm

Boards.ie: Paddy wrote: »
Thought I'd share a real beauty from the boards codebase. This was one of the first things I read when I was taking over from Conor. I'm yet to understand how anyone could ever have thought that this was a good idea:
/**
* Essentially a wrapper for the ternary operator.
*
* @deprecated	Deprecated as of 3.5. Use the ternary operator.
*
* @param	string	Expression to be evaluated
* @param	mixed	Return this if the expression evaluates to true
* @param	mixed	Return this if the expression evaluates to false
*
* @return	mixed	Either the second or third parameter of this function
*/
function iif($expression, $returntrue, $returnfalse = '')
{
	return ($expression ? $returntrue : $returnfalse);
}
I guess some people just find ternary operators too confusing?

Looks like a VB programmer, almost exact same parameter names even http://msdn.microsoft.com/en-us/library/27ydhh0d%28v=vs.90%29.aspx

Boards.ie: Paddy · 05-03-2012 10:26pm

Hah good guess, you get a cookie. The developers of VBulletin were indeed originally VB programmers. I guess it's the same mentality that leads people to write monstrosities like this:

http://phpjs.org/functions/substr:558

Boards.ie: Neil · 05-03-2012 10:44pm

the best thing about iif() is the @deprecated comment

php.js should also die in a fire and should be quick and painful so less people use it before it dies.

lenovoguy · 26-03-2012 10:18am

Working on a Win32/MFC app at the moment which also has a COM/ActiveX control inside it. Adding text field to the control required changing 41.cpp files, 21.h files & a couple of others, because our software supports lots of file formats and each file format's class (of which all derive from the same parent) individually has to be updated so copy constructors, overloaded operators, data serialisation functions etc., successfully copy the new text value. We may be missing the point of OOP.

Odaise Gaelach · 08-04-2012 7:28pm

Not exactly coding horror, but I'm working on something in C# and Visual Studio 2010. So...

string[] infoSplits = [I]**some initialisation here**[/I]

for (int secondaryIndex = 5; secondaryIndex < infoSplits.Length; secondaryIndex++)
[B]{[/B]
   string[] moreInfoSplits = infoSplits[secondaryIndex].Split('|');

Don't worry too much about code itself or the variable names; they make sense to me.

Anyway, I just spent the last 30 minutes wondering why secondaryIndex would jump from 5 to 13 in between the for-loop declaration and the moreInfoSplits[] declaration. Where the curly bracket is.

For a while I thought I was going mad. Every time I went across those three lines it would change. Over and over and over again, without fail. Well it turned out that I had a conditional breakpoint set at that bracket. I had the condition set as:

secondaryIndex = 13

Instead of:

secondaryIndex == 13

Sometimes I'm an absolute dumbass. At least I'm not insane.

Giblet · 08-04-2012 8:57pm

Another reason why

13 == secondaryIndex

is the master syntax

Anima · 09-04-2012 11:33am

Giblet wrote: »
Another reason why
13 == secondaryIndex
is the master syntax

I dunno, it just looks bad to me. I think it ruins the flow when reading it. Also any decent compiler will warn you about it (c++ anyway).

Colonel Panic · 09-04-2012 11:54am

And we all have warnings as errors enabled, right?

Some of the code I work with has thousands of warnings, each one a bug waiting to happen.

I think he'd put that into a conditional breakpoint as opposed to code btw!

Regarding 13 == secondaryIndex. It's more readable when dealing with constants and stuff!

SirLemonhead · 10-04-2012 11:54am

i'm a fan of doing

const int kSomeMagicNumber = 13;

then doing "if (kSomeMagicNumber = 13) " should throw up a compiler error

SirLemonhead · 12-04-2012 4:13pm

SirLemonhead wrote: »

i'm a fan of doing

const int kSomeMagicNumber = 13;

then doing "if (kSomeMagicNumber = 13) " should throw up a compiler error

edit: just came back and re-read my post. WHAT was I thinking... disregard it :pac:

Slutmonkey57b · 19-04-2012 7:25pm

Is there a particular reason why there are so many variants of SQL, and why there are so many application-specific addendums? Was someone offering a bounty on these things in the 90's?

"Syntax error on subquery"
*searches for syntax guide for this particular variant, comes up with nothing*