Coding Horror

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