Coding Horror

Earthhorse

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

fasty

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

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

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

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

Sparks

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

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

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

Evil Phil

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

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

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

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

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

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

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

Defiler Of The Coffin

For those who are nostalgic for Pascal...

#define begin {
#define end }

ocallagh

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

srsly78

VB code:

On Error Resume Next

Colonel Panic

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

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

As if deep inheritance wasn't fail enough!

km991148

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

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

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

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

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

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

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

Creamy Goodness wrote: »

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

wtf?

It's a minimalistic stylesheet

Genghiz Cohen

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

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