Coding Horror

Maximus Alexander · 30-07-2014 10:35am

I regularly have to modify SQL procedures that are hundreds of lines long, uncommented and have no consistent format, even internally to themselves. I often wonder how anyone managed to write them to begin with whilst still keeping track of what was going on.

IRLConor · 30-07-2014 11:47am

BrokenArrows wrote: »

I have one with 32793
There are plenty of others around the 20k mark.

Every time a change request comes in for this software I die a little. Annoying thing was it was discontinued years ago but we have a few customers who refuse to let it go and are paying for continued development.

Thank $DEITY for the 64kb method limit, otherwise that software would be all in one method!

Korvanica · 07-08-2014 12:53pm

        public string TellMeDate()
        {
            return DateTime.Today.ToString();
        }

Is this really necessary....

Ludo · 07-08-2014 12:54pm

Korvanica wrote: »

        public string TellMeDate()
        {
            return DateTime.Today.ToString();
        }

Is this really necessary....

It needs comments!

Colonel Panic · 07-08-2014 1:04pm

And unit tests.

BrokenArrows · 08-08-2014 9:59am

Ludo wrote: »

It needs comments!


        /// <summary>
        /// Get the date
        /// </summary>
        /// <returns>The Date string</returns>
        public string TellMeDate()
        {   // Start of method
            return DateTime.Today.ToString(); // Returns the date string from DateTime.Today object in the format defined by regional settings.
             // End of method
        }
    
:)

Earthhorse · 08-08-2014 12:21pm

You could argue that the developer was looking for a more fluid syntax for an oft repeated line in their code but if that's the case (and it's C#) I would write an Extension method and I wouldn't call it TellMeDate!

The_B_Man · 11-08-2014 3:43pm

I found a couple of code snippets today that I thought were just bad code. Not horrors, but not exactly great either.

The first one was:

if(myBool) {
    myOtherBool = true;
}

The other was:

return map.containsKey(mKey) ? true : false;

Note that the method "containsKe()" returns a boolean. It also caused a NullPointerException, which is what highlighted the line of code to me in the first place.

In the spirit of using the ternary operator, I replaced it with:

return (null==map) ? false : map.containsKey(key);

Evil Phil · 12-08-2014 11:59am

        /// <summary>
        /// Get the date
        /// </summary>
        /// <returns>The Date string</returns>
        public string TellMeDate()
        {   // Start of method
            String datetime = DateTime.Today.ToString(); // Returns the date string from DateTime.Today object in the format defined by regional settings, and saves it to a string variable called datetime.
            return datetime; // return the datetime string from the previous line
            // End of method
        }

Fixed that for you :pac:

Procasinator · 13-08-2014 6:05pm

The_B_Man wrote: »

return (null==map) ? false : map.containsKey(key);

You could use:

return map != null && map.containsKey(key);

Unless you want to keep the "spirit of the ternany operator" out of humour.

id rather brie · 16-08-2014 4:30pm

Took over an existing Webforms app when I start a new job recently. The app was initially built in like three weeks by a group of Polish contractors, very rushed job. Everything seemed relatively well coded for how rushed it was (still, a messy job, but functional).

Anyway, I took it over, did some bug fixes and had my first release. Decided to port it over to MVC as I god damn hate Webforms. After about a week and a half porting it over I started running into code written in bloody Polish; the worst part is as I'm debugging/testing the code fully, I'm running into bugs that exist within the Polish code. so for the moment I'm having to use google to translate, then figure out the context of what the gibberish google spits out means within the current scope, and then try to rewrite it in English

Other than noise, there's a lot of other stuff, like get a list of records from DB and THEN filtering them by looping though the list...jesus.

a lot of instances of using Int32.TryParse and Int64.TryParse to populate nullable longs and ints without actually checking to ensure the parsing was successful...which meant they could be potentially trying to use nulls in calculations.

I'll be glad when I've finished porting this project

Colonel Panic · 16-08-2014 5:45pm

id rather brie wrote: »

Other than noise, there's a lot of other stuff, like get a list of records from DB and THEN filtering them by looping though the list...jesus.

I see this all over the place in some code I maintain. Selecting 200 records when 2 would do.

I've been able to improve performance just by changing SQL.

id rather brie · 16-08-2014 7:18pm

Colonel Panic wrote: »

I see this all over the place in some code I maintain. Selecting 200 records when 2 would do.

I've been able to improve performance just by changing SQL.

Tell me about it, there was a single page in this site, that had like 3/4 instances of this happening, and every event that fired, called them (no checking for postbacks) even when the data wasn't necessary. This page, and one DB call in particular, currently accounts for like 31% of our database's activity because of how unnecessarily often it is called.

I can't wait to get the next release out to fix this.

c_man · 08-09-2014 1:28pm

Our scrummaster told us this morning that the Mythical Man-moth, specifically the part about throwing more people at a problem not making the task faster, is itself a myth.

You just need the right people with the right expertise
Which we don't have
Ah ye can get up to speed in no time

Sigh.

Berserker · 11-09-2014 1:44pm

Was in talking to a Principal dev a project this morning. During the conversation we discussed in-house coding standards and he told me that his team has none because he has no Junior developers and experienced developers know when something is "done right or not". He then told me that unit testing was of no use, unless you were using the TDD methodology.

GreeBo · 11-09-2014 6:18pm

I had someone tell me that they could delete a bunch of failing unit tests as "no one would notice and I'm sure no one knows what they do anyway"

Should I mention that we are on a drive to improve test coverage?
And that this person is on that team?

GreeBo · 11-09-2014 6:19pm

c_man wrote: »

Our scrummaster told us this morning that the Mythical Man-moth, specifically the part about throwing more people at a problem not making the task faster, is itself a myth.

You just need the right people with the right expertise
Which we don't have
Ah ye can get up to speed in no time

Sigh.

The man-moth is mythical...not like that evil mothman!:pac:

c_man · 11-09-2014 8:05pm

Berserker wrote: »

He then told me that unit testing was of no use, unless you were using the TDD methodology.

Heard a good one recently. A team was arguing to remove a good percentage of our unit tests, to help reduce CI build time... C'mon, really?

Dacelonid · 11-09-2014 9:25pm

Just on the topic of tests, last year I had to do an analysis of the unit tests on a ~4 year old project that was currently mostly being developed by an offshore team

Apart from using @Ignore to turn off tests that were failing, or deleting asserts so that they wouldn't fail, or tests in the wrong class, or tests named for something stupid rather than what it was testing, the one that really stuck in my mind was something along these lines (method names are made up)

@Test
public void testSomething(){
    final String returnValue = "";
    setupMock();
    DBConnectionManager objUnderTest = new DBConnectionManager();
    objUnderTest.doMethod();
    
    assertNotNull(returnValue);

}

There was a bit more code that that in the test, but that was a general structure. I tried to explain the management and the offshore teams, that this wasn't actually testing anything, they didn't really get it.

GreeBo · 11-09-2014 9:38pm

Dacelonid wrote: »
Just on the topic of tests, last year I had to do an analysis of the unit tests on a ~4 year old project that was currently mostly being developed by an offshore team

Apart from using @Ignore to turn off tests that were failing, or deleting asserts so that they wouldn't fail, or tests in the wrong class, or tests named for something stupid rather than what it was testing, the one that really stuck in my mind was something along these lines (method names are made up)
@Test
public void testSomething(){
    final String returnValue = "";
    setupMock();
    DBConnectionManager objUnderTest = new DBConnectionManager();
    objUnderTest.doMethod();
    
    assertNotNull(returnValue);

}
There was a bit more code that that in the test, but that was a general structure. I tried to explain the management and the offshore teams, that this wasn't actually testing anything, they didn't really get it.

My current codebase is littered with that crap.
Be glad it at least asserted something.

Most of mine are either no assertion or assertTrue()

Maximus Alexander · 12-09-2014 12:33am

Dacelonid wrote: »
@Test
public void testSomething(){
    final String returnValue = "";
    setupMock();
    DBConnectionManager objUnderTest = new DBConnectionManager();
    objUnderTest.doMethod();
 
    assertNotNull(returnValue);
 
}
There was a bit more code that that in the test, but that was a general structure. I tried to explain the management and the offshore teams, that this wasn't actually testing anything, they didn't really get it.

Maybe what they were trying to test is whether or not a blank string is null? :pac:

GreeBo · 12-09-2014 1:18am

Maximus Alexander wrote: »

Maybe what they were trying to test is whether or not a blank string is null? :pac:

Is still not null...who knows what memory register madness those other classes are up to!

Berserker · 12-09-2014 8:19am

GreeBo wrote: »

I had someone tell me that they could delete a bunch of failing unit tests as "no one would notice and I'm sure no one knows what they do anyway"

I've had that before. First thing in the morning someone notices that the tests have failed. Someone goes, "comment the failing tests out, so the tests all pass and all looks ok".

Eogclouder · 12-09-2014 9:45am

Berserker wrote: »

I've had that before. First thing in the morning someone notices that the tests have failed. Someone goes, "comment the failing tests out, so the tests all pass and all looks ok".

How is it working in hell?

SilverScreen · 12-09-2014 10:30am

Dacelonid wrote: »
Just on the topic of tests, last year I had to do an analysis of the unit tests on a ~4 year old project that was currently mostly being developed by an offshore team

Apart from using @Ignore to turn off tests that were failing, or deleting asserts so that they wouldn't fail, or tests in the wrong class, or tests named for something stupid rather than what it was testing, the one that really stuck in my mind was something along these lines (method names are made up)
@Test
public void testSomething(){
    final String returnValue = "";
    setupMock();
    DBConnectionManager objUnderTest = new DBConnectionManager();
    objUnderTest.doMethod();
    
    assertNotNull(returnValue);

}
There was a bit more code that that in the test, but that was a general structure. I tried to explain the management and the offshore teams, that this wasn't actually testing anything, they didn't really get it.

I'm assuming that doMethod() is meant to return a String and that's what was meant to be tested.

Otherwise it makes no sense at all

Aswerty · 12-09-2014 12:21pm

SilverScreen wrote: »

I'm assuming that doMethod() is meant to return a String and that's what was meant to be tested.

Otherwise it makes no sense at all

But returnValue is final when assigned to an empty string!! :pac: WTF?

SilverScreen · 12-09-2014 4:15pm

Aswerty wrote: »

But returnValue is final when assigned to an empty string!! :pac: WTF?

Yeah that's another thing. They were more than likely using a static analysis tool like PMD.

My assumption is to remove line 3 and change line 6 to String returnValue = objUnderTest.doMethod();
That's if doMethod() actually returns something...

jester77 · 12-09-2014 4:17pm

GreeBo wrote: »

I had someone tell me that they could delete a bunch of failing unit tests as "no one would notice and I'm sure no one knows what they do anyway"

Should I mention that we are on a drive to improve test coverage?
And that this person is on that team?

hah, I came across that recently in a project. There was an initiative to get the builds green. Some took this to mean removing the failing tests :eek:

GreeBo · 12-09-2014 6:27pm

jester77 wrote: »

hah, I came across that recently in a project. There was an initiative to get the builds green. Some took this to mean removing the failing tests :eek:

The scarier bit for me was the idea that since the tests were old and that whoever had written them no longer worked at the company, they could be removed as no one would care.
Did I mention that they passed on a local box but not a jenkins box...lets not find out why, just zap them, "no one uses them".
How you use a unit test is beyond me, its just there, quietly doing its thing until you do need it.

Dacelonid · 12-09-2014 8:52pm

SilverScreen wrote: »

I'm assuming that doMethod() is meant to return a String and that's what was meant to be tested.

Otherwise it makes no sense at all

Aswerty wrote: »

But returnValue is final when assigned to an empty string!! :pac: WTF?

SilverScreen wrote: »

Yeah that's another thing. They were more than likely using a static analysis tool like PMD.

My assumption is to remove line 3 and change line 6 to String returnValue = objUnderTest.doMethod();
That's if doMethod() actually returns something...

Just to be clear, doSomething doesn't return anything.
My guess is that at some point in the past it did. Then they changed the method to not return anything and instead of verifying that the test was actually testing something, they just removed the assignment in the test, ran it again and it passed, so they checked that in

id rather brie · 20-09-2014 3:12pm

Came across a ridiculously pointless piece of code recently, the dude who wrote it before me must have been awake for 3, or more days. There was a piece a piece of ASP code that was writing a number into a javascript section, like so:

    var width = <%= Session["ScreenWidth"].ToString() %> - 970 - 1;

I mean if you're going to hardcode all of this poop in...why is this the method you choose...*shakes head*

Eogclouder · 20-10-2014 9:44am

annacsnolan wrote: »

Hi All,

Sorry to butt in on the conversation. I am urgently looking for a tutor in HTML and CSS. I don't suppose any of you are in Cork city and willing to provide some evening/ weekend grinds?

Thank you!

This is off topic and not relevant to this thread.

Lemming · 31-10-2014 5:48pm

Two in one day ....

There was the method that's 5000+ lines long this morning.

And now .... try/catch blocks being used to govern program flow.

/me heads desk.

Berserker · 31-10-2014 5:57pm

Lemming wrote: »

And now .... try/catch blocks being used to govern program flow.

That's a new one on me, I have to admit. What sort of mind came up with that.

BrokenArrows · 01-11-2014 10:57pm

Lemming wrote: »

Two in one day ....

There was the method that's 5000+ lines long this morning.

And now .... try/catch blocks being used to govern program flow.

/me heads desk.

Can you post an example of how he was using the try catch blocks. I can't image how that would work unless he was purposely causing exceptions.

Lemming · 03-11-2014 2:43pm

BrokenArrows wrote: »

Can you post an example of how he was using the try catch blocks. I can't image how that would work unless he was purposely causing exceptions.

double blah;
try
{
	blah = Convert.ToDouble(randomStringValue);
}
catch
{
	blah = -1;
}

Ever heard of .TryParse() matey? .... :-/

The same try/catch is reproduced elsewhere where it does the same with Nullable<T> objects. No basic checks to see if the object has been instantiated and god forbid there be any checking the Nullable<T>.HasValue property ... The original author was just assigning directly and re-assigning of an exception was thrown.

One of the above examples on its own would be enough for a "f*cks sake" moment followed by refactoring; but this is the pattern used quite extensively throughout this particular class. In particular in that 5000+ line Frankenstein travesty of a method I mentioned earlier. Jesus wept.

Edit: the above sample can be found inside and out of method calls, and the string values may or may not be populated based on other factors, so it's not a simple case of "this variable REALLY should contain a value when hitting this particular path/line of code".

Colonel Panic · 03-11-2014 8:22pm

That reeks of someone who comes from a procedural background.

There are some times though when the .Net framework throws exceptions for stupid reasons. HttpWebRequest for example. Why throw an exception for 404s?

Berserker · 03-11-2014 8:45pm

Colonel Panic wrote: »

That reeks of someone who comes from a procedural background.

There are some times though when the .Net framework throws exceptions for stupid reasons. HttpWebRequest for example. Why throw an exception for 404s?

What you would prefer it to do?

Lemming · 03-11-2014 9:41pm

Colonel Panic wrote: »

That reeks of someone who comes from a procedural background.

I disagree. It reeks of someone who doesn't remember one of the first things you're told about try/catch blocks in uni; they are NOT execution control mechanisms. I remember that from being introduced to try/catch blocks in C back in the day in my second year of third level education. That's about fifteen years ago ...

Colonel Panic · 03-11-2014 11:25pm

Berserker wrote: »

What you would prefer it to do?

Make a request, get a response, test the response code, do what you've got to do.

Lemming wrote: »

I disagree. It reeks of someone who doesn't remember one of the first things you're told about try/catch blocks in uni; they are NOT execution control mechanisms. I remember that from being introduced to try/catch blocks in C back in the day in my second year of third level education. That's about fifteen years ago ...

try/catch blocks in C? As in SEH? That's quite a specific and not that useful thing to teach in 2nd year of 3rd level education.

Lemming · 04-11-2014 1:13am

Colonel Panic wrote: »

try/catch blocks in C? As in SEH? That's quite a specific and not that useful thing to teach in 2nd year of 3rd level education.

Sorry you're right; it was C++ (and my third year - did C in second - same lecturer for both so it's all blurred together now) but the point still stands.

Anesthetize · 11-01-2015 1:28pm

Came across something like this during the week:

if (.....) {
<some statement>;
} else
<some statement>;
<some statement>;

It really annoys me when people don't use brackets with if-else statements in Java. Just because you can doesn't mean you should.

yes there · 11-01-2015 8:21pm

Anesthetize wrote: »

Came across something like this during the week:

if (.....) {
<some statement>;
} else
<some statement>;
<some statement>;

It really annoys me when people don't use brackets with if-else statements in Java. Just because you can doesn't mean you should.

Surely that is down to organisation conventions is it not. That's a personal gripe not a coding horror.

conor.hogan.2 · 11-01-2015 9:02pm

yes there wrote: »

Surely that is down to organisation conventions is it not. That's a personal gripe not a coding horror.

Yes, except that there is braces on the if so it is inconsistent.

oscarBravo · 11-01-2015 9:46pm

What makes it a true horror is what was lost by not using code tags on the original post:

if (.....) {
    <some statement>;
} else 
    <some statement>;
    <some statement>;

Now that's pure evil.

Anesthetize · 11-01-2015 10:05pm

oscarBravo wrote: »
What makes it a true horror is what was lost by not using code tags on the original post:
if (.....) {
    <some statement>;
} else 
    <some statement>;
    <some statement>;
Now that's pure evil.

Yes that's what I meant.

Defiler Of The Coffin · 11-01-2015 10:16pm

yes there wrote: »

Surely that is down to organisation conventions is it not. That's a personal gripe not a coding horror.

It's bad practice if it looks deceiving. Especially if the indentation isn't correct.

Maximus Alexander · 12-01-2015 2:43am

Anesthetize wrote: »

Came across something like this during the week:

if (.....) {
<some statement>;
} else
<some statement>;
<some statement>;

It really annoys me when people don't use brackets with if-else statements in Java. Just because you can doesn't mean you should.

You'd hate me then, because I'd probably have written:

if (.....) <some statement>;
else <some statement>;

<some statement>;

oscarBravo · 12-01-2015 10:31am

(It's at times like this I'm really glad I'm a Python guy...)

matrim · 12-01-2015 10:48am

oscarBravo wrote: »

(It's at times like this I'm really glad I'm a Python guy...)

And then someone mixes up tabs and spaces

Coding Horror

Comments