Coding Horror

oscarBravo · 03-07-2014 11:53pm

Creamy Goodness wrote: »

http://www.php.net/manual/en/datetimeimmutable.modify.php

Jaysus.

MajesticDonkey · 04-07-2014 12:18am

Creamy Goodness wrote: »

http://www.php.net/manual/en/datetimeimmutable.modify.php

Despite the name, and unlike <code>DateTime::modify</code>, this function does <i>not</i> modify the DateTimeImmutable object. Code inspection leads me to believe that it <i>copies</i> the DateTimeImmutable object, modifies the copy, and returns that.

It's still bloody stupid though.

BrokenArrows · 18-07-2014 11:31am

long propertyID;
if (//some validation here)
{
 // Some stuff
}
else
{
[B] propertyID = (long)1;[/B]
}

Why the cast?

Tar.Aldarion · 18-07-2014 11:32am

Because it's a big one.

Berserker · 28-07-2014 9:53am

BrokenArrows wrote: »

long propertyID;
if (//some validation here)
{
 // Some stuff
}
else
{
[B] propertyID = (long)1;[/B]
}

Why the cast?

I have seen this with boolean types too.

var somevar = (bool) false;

croo · 28-07-2014 2:17pm

Code:
long propertyID;
if (//some validation here)
{
// Some stuff
}
else
{
propertyID = (long)1;
}
Why the cast?

I've not seen it myself but might it be (mistakenly) instead of adding the L to the end of the literal where it might be misread?

The literal 1 being an int while 1L is a long value, right? Not that leaving off the L will cause an issue with the value 1. And, of course, if the 1 was replace with a real "long" value (i.e. one larger than allowed by int) it would fail anyway.

My guess is someone was told to always use the modify to be explicit but they misunderstood!

I cannot think of any reason why someone would do the same with boolean however.

ChRoMe · 28-07-2014 8:32pm

Opened up the class I've been asked to retro fit with unit tests (the code is 10 years old).

The class is 30k lines....

Colonel Panic · 28-07-2014 8:52pm

One static method in a class with no member variables?

Q_Ball · 28-07-2014 9:46pm

Lads, srsly, stop following me around!

BrokenArrows · 30-07-2014 10:23am

ChRoMe wrote: »

Opened up the class I've been asked to retro fit with unit tests (the code is 10 years old).

The class is 30k lines....

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.

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()

Coding Horror

Comments