The Daily WTF: Curious Perversions in Information Technology

Bitruder · 2006-08-08 Reply Admin

Would that ever not return null?
Wait... wouldn't that be an infinite loop?

2006-08-08 Reply Admin

You know, there's nothing really wrong with using junior programmers as long as you have someone in charge who knows what s/he's doing and can mentor them. That's the real WTF here.

2006-08-08 Reply Admin

Oh dear God... I may never complain about my coworkers again.

2006-08-08 Reply Admin

You must be one of the junior developers. :)

jspenguin · 2006-08-08 Reply Admin

No... roles.Count increases with each item you add to it... I think. Is that C#? It's not Java.

2006-08-08 Reply Admin

Bitruder:
Would that ever not return null?
Wait... wouldn't that be an infinite loop?

Seem to do what it should - but in a baaaaad way in terms of performance... I actually cried reading this

2006-08-08 Reply Admin

Alex Papadimoulis:

public ArrayList GetRoles()
{
  if (this.getRols() != null)
  {
    ArrayList roles = new ArrayList();
    while (roles.Count < getRols().Count)
    {
      RoleClass role = (RoleClass)getRols().Item[roles.Count];
      roles.Add(role);
    }
    return roles;
  }
  return null;
}

this isn't really making me scream wtf. At least we are seeing code examples again.

2006-08-08 Reply Admin

Anonymous:

Alex Papadimoulis:

public ArrayList GetRoles()
{
  if (this.getRols() != null)
  {
    ArrayList roles = new ArrayList();
    while (roles.Count < getRols().Count)
    {
      RoleClass role = (RoleClass)getRols().Item[roles.Count];
      roles.Add(role);
    }
    return roles;
  }
  return null;
}

this isn't really making me scream wtf. At least we are seeing code examples again.

It's possible you need to look more closely, and realize that every getRols() is hitting the database. Especially all those buggers in the while loop...

2006-08-08 Reply Admin

If I remember right, Frederick Brooks is also the guy who said that throwing more people at a project is certain to make the project even later than it would otherwise be, much like giving nine women a month to make a baby. (Ahh, the Mythical Man-Month.)

2006-08-08 Reply Admin

Alex Papadimoulis:

public ArrayList GetRoles()
{
  if (this.getRols() != null)
  {
    ArrayList roles = new ArrayList();
    while (roles.Count < getRols().Count)
    {
      RoleClass role = (RoleClass)getRols().Item[roles.Count];
      roles.Add(role);
    }
    return roles;
  }
  return null;
}

Honestly, I fail to see the WTF. Apart from the array being duplicated unnecessarily, that is. And not following capitalization conventions (GetRoles). And funny typos (getRolls?). And the fact that the system lacks any design thought whatsoever. Anything else?

2006-08-08 Reply Admin

Anonymous:
Anonymous:
Alex Papadimoulis:
public ArrayList GetRoles()
{
 if (this.getRols() != null)
 {
 ArrayList roles = new ArrayList();
 while (roles.Count < getRols().Count)
 {
 RoleClass role = (RoleClass)getRols().Item[roles.Count];
 roles.Add(role);
 }
 return roles;
 }
 return null;
}
this isn't really making me scream wtf. At least we are seeing code examples again.
It's possible you need to look more closely, and realize that every getRols() is hitting the database. Especially all those buggers in the while loop...

that'll teach me for not reading past the code. i stand corrected WTF!

cronthenoob · 2006-08-08 Reply Admin

I don't get it.

2006-08-08 Reply Admin

To repeat the key point - look closely and realize that every call to getRols() returns the static list of roles in the database.

This means if there are 12 roles in the database, this function could fetch the exact same list 25 times.

"The real WTF...." (I just had to say it)

2006-08-08 Reply Admin

This is basically doing the following in wtfey way.

public ArrayList GetRoles()
{
    ArrayList roles = getRols();
    if (roles == null)
       return null;
    return roles.Clone();
}

2006-08-08 Reply Admin

Well, at least it works. I guess that is what makes it a WTF. In Java, (I'm sure C#, a blatant copy of Java, has similiar) you could reduce it to...

public ArrayList getRoles() {
return getRols() == null ? null : new ArrayList(getRols());
}

This is assuming, of course, that for some reason you need a copy of the ArrayList instead of the actual instance member of it. Also, What The F is a "Rol"?

On the other hand, the coder was rather clever in not having to use a pesky variable to count over the ArrayList, just use the count of the one you are building instead! (WTF is an iterator, anyway! or even the addAll method!)

Not really a horrible WTF, but I am glad to see more code here again.

jspenguin · 2006-08-08 Reply Admin

Anonymous:
Anonymous:
Anonymous:
Alex Papadimoulis:
public ArrayList GetRoles()
{
 if (this.getRols() != null)
 {
 ArrayList roles = new ArrayList();
 while (roles.Count < getRols().Count)
 {
 RoleClass role = (RoleClass)getRols().Item[roles.Count];
 roles.Add(role);
 }
 return roles;
 }
 return null;
}
this isn't really making me scream wtf. At least we are seeing code examples again.
It's possible you need to look more closely, and realize that every getRols() is hitting the database. Especially all those buggers in the while loop...
that'll teach me for not reading past the code. i stand corrected WTF!

That was an edit. The original post said nothing about what getRols() does. I conjectured that it was hitting the database, because why else would it be WTF worthy?

WeatherGod · 2006-08-08 Reply Admin

Correct me if I am wrong, but wouldn't I get the same behavior if I just replaced all calls to GetRoles() with calls to getRols()?

Maybe they just wanted to put an abstractation layer between the database and the code? I could see this function originally being meant to retrieve Roles from a file, and then it was changed to retrieve roles from a database. Whenever I make a feature upgrade in my programs, I always ask myself, "Now that I have this feature, can I improve my functions?". These guys did not.

2006-08-08 Reply Admin

Anonymous:
To repeat the key point - look closely and realize that every call to getRols() returns the static list of roles in the database.

This means if there are 12 roles in the database, this function could fetch the exact same list 25 times.

"The real WTF...." (I just had to say it)

the whole method looks redundant why not call "getRols" directly or at least do something like (assuming getRols returns some kind of list implementation):

public ArrayList GetRoles()
{
return new ArrayList(getRols());
}

2006-08-08 Reply Admin

Anonymous:
Well, at least it works. I guess that is what makes it a WTF. In Java, (I'm sure C#, a blatant copy of Java, has similiar) you could reduce it to...

public ArrayList getRoles() {
return getRols() == null ? null : new ArrayList(getRols());
}

This is assuming, of course, that for some reason you need a copy of the ArrayList instead of the actual instance member of it. Also, What The F is a "Rol"?

On the other hand, the coder was rather clever in not having to use a pesky variable to count over the ArrayList, just use the count of the one you are building instead! (WTF is an iterator, anyway! or even the addAll method!)

Not really a horrible WTF, but I am glad to see more code here again.

Oh LOL, I failed to read the part about getRols() hitting the database (or was this added after I read it?), I shall shut up now!

BiggBru · 2006-08-08 Reply Admin

Alex Papadimoulis:

Thankfully, amongst the Nonbelievers was a single Believer. He knew that the application needed just one good programmer. A long political battle raged and, shortly after the Juniors were let go, Willem was brought in to replace them. It was to be the True Test of the Word of Brooks, though a slightly unfair one.

Rule of thumb: Quality trumps Quantity. Every time.

And welcome back, Alex.

2006-08-08 Reply Admin

every call to getRols() performs a multiple-row-resultant query on the database

so let's expand this function (translated into php for clarity), and include what it's calling

function getRols()
{
       $sql = "SELECT * FROM roles";
       $res = mysql_query($sql);
       $retval = array();
       while ($row = mysql_fetch_object($res))
          $retval[] = $row;
       return $retval;
}

function GetRoles()
{
    $checkRoles = getRols();
    if (count($checkRoles))
    {
       $roles = array();
       while (1)
       {
             $lengthRoles = getRols();
             if (count($roles) >= count($lengthRoles))
                break;
             $fetchRoles = getRols();
             $roles[] = $fetchRoles[count($roles)];
        }
       return $roles;
    }
    return false;
}

i could further example this to be C-code to make the performance wtf-ness be even MORE obvious

each time getRols() is referenced it's: query the database, allocating memory, copying, referencing and IF YOU'RE LUCKY deallocating assuming the return value of getRols() is some sort of object that cleans up it's memory after itself!

gadgetarms · 2006-08-08 Reply Admin

This isn't actually as bad as it may seem. getRols() is probably a private getter method wrapping an instance variable, while GetRoles is the public equivalent and does nothing but clone the Roles returned by getRols(). Most likely, this is an attempt to keep external classes from adding/removing items from the Roles list (since ArrayList is not immutable). Although the choice of method names and the fact that they didn't use roles.clone() leaves something to be desired, I've seen much worse. Of course if I am wrong and getRols() makes a database call, then this code really sucks.

2006-08-08 Reply Admin

Yup, that's Brooks all right. TMMM was controversial at the time, but he followed it up with "No Silver Bullet", an essay that basically claimed that there was nothing that would improve productivity by several orders of magnitude. Not even Agile Scrum methodologies du jour :-)

2006-08-08 Reply Admin

erg... "i could further EXPAND" not example... fried green brain here

CAPTCHA: whiskey! woohoo

2006-08-08 Reply Admin

RTFA gadgetarms - getRols() calls a select on the database, as i tried to show more clearly with my above expansion

CAPTCHA: enterprisey - another winner!

2006-08-08 Reply Admin

Anonymous:

that'll teach me for not reading past the code. i stand corrected WTF!

Don't feel too bad - that wasn't there originally. I use a browser that supports tabs, so I have the main page open in one tab and the story in another. That little bit at the end about what getRols() does was added sometime between when I pulled up the main page and read the thread.

2006-08-08 Reply Admin

Anonymous:
In Java, (I'm sure C#, a blatant copy of Java, has similiar) you could reduce it to...

C'mon. Everyone knows that C# is a blatant copy of Delphi, not Java. :)

bmartensen · 2006-08-08 Reply Admin

It doesn't matter what the actual implementation of getRols is, whether it is just returning a private member or whether it is hitting the database. You shoulnd't make an assumption about how it is implemented because you want to be able to change the implementation of getRols without having an effect on code anywhere else. To assume an implementation is to add an unnecessary dependancy.

Proper implementation would be to call getRols() once and assign it to a local variable, there are no good excuses for anything else. Anybody who doesn't get the WTF here needs to question their chosen profession (assuming it's programming).

2006-08-08 Reply Admin

public Bool GetRollz0rz(int SuperFluous)
{
 EnterpriseTable paula = new WoodenTable();
 paula.Refactor(GetRoles() == null ? null : (new ArrayList()).Add(GetRoles().Item[0]).Add(GetRoles().Item[1]).Add(GetRoles().Item[2]));
 return FileNotFound;
}

2006-08-08 Reply Admin

Anonymous:
RTFA gadgetarms - getRols() calls a select on the database, as i tried to show more clearly with my above expansion

CAPTCHA: enterprisey - another winner!

I checked my browser history and the original posting by Alex said nothing about hitting the db - hence the confusion

2006-08-08 Reply Admin

It's not just junior developers, i started work straight out of university working with someone with 3years in the business (not much but alot more than me). Here is an example of his code

ArrayList al = new ArrayList();

// al gets populated so has stuff in

int count = 0;

for (int i=0; i<al.Count; i++)
{
count++;
}

return count;

And this guy was earning far more than me since he was contracting. I wondered why I even bothered sometimes. Not to mention the webpage he wrote that took approximately 30seconds to be generated. Once he left I rewrote it to take around 1second. And im still on my rubbish wage.

dtfinch · 2006-08-08 Reply Admin

Suppose there were 10 roles. It'd only have to query all the roles from the database 22 times, creating 23 array lists and 220 RoleClass objects. That's acceptable in a CIS project, right? :)

Go easy on them. One of them probably deserves most of the blame, having written 90% of the code. The other nine just watched, discussed, etc.

GoatCheez · 2006-08-08 Reply Admin

For all of you that are missing why this is SO HORRIBLY DOWNRIGHT AWFULLY HORRENDOUS, I'll explaing... although I probably shouldn't since you should know better anyway!!!

First, I'll just note that it looks like GetRoles was created to retrieve the roles without hitting the database. (If it wasn't, then it's a COMPLETELY USELESS function.)

Now...

The first conditional (that means an if statement) calls getRols. getRols it is pointed out hits the database to retrieve the information it returns. The conditional however only checks that the return value is not null, indicating that the call succeded and was able to return some sort of information (an arraylist).

Next, we create the array list that our "non-database-hitting-function" will return.

Now, we start a while loop, saying that until our return list is the size of the database list, we do things (see below). I believe every iteration of this would hit the database again, as I don't think the count will be cached by the compiler in an attempt to optimize the loop. Don't quote me on that though, it could vary greatly depending on the compiler, and other things as well.... Skepticism leads me to believe though that this will in fact hit the database for every iteration to check against the count.

Now, we are in the loop.... in here, we HIT THE DATABASE AGAIN! This time, it's to index a role in the list that is returned, so we can set our corresponding item in our list to that role.

Finally, the new list is returned... only if they had known though that

public ArrayList GetRoles()
{
  return getRols();
}

was all the code they needed for this useless function. Well, they should have at least seen that all their code was doing was the above.... This is definitely a good WTF today lol.

So to recap, let's say that there were 7 roles. Calling this function would result in hitting the database...

initial check
while check 0 == true
      general hit
while check 1 == true
      general hit
   while check 2 == true
      general hit
while check 3 == true
      general hit
   while check 4 == true
      general hit
while check 5 == true
      general hit
   while check 6 == true
      general hit
while check 7 == false

16 times... wow... that's REALLY bad... REALLY REALLY bad. Are you sure these were junior developers and were not monkeys?

;-P

2006-08-08 Reply Admin

The real WTF is that it returns a null array!!! This thing should return an empty array list instead! Terrible conventions!

Although calling the database over and over again... ok that is funny. They could have even added another call to the database by using:

ArrayList roles = new ArrayList(getRols().Count);

2006-08-08 Reply Admin

Anonymous:
This is basically doing the following in wtfey way.

public ArrayList GetRoles()
{
    ArrayList roles = getRols();
    if (roles == null)
       return null;
    return roles.Clone();
}

as a jr developer myself:


public ArrayList GetRoles(){
    ArrayList roles = getRols();
    return roles==null?   null  : roles.Clone();
}

or if I am feeling "clever"...


public ArrayList GetRoles(){
    ArrayList roles;
    return null==roles=getRols()? roles : roles.Clone();
}

2006-08-08 Reply Admin

Anonymous:
It's not just junior developers, i started work straight out of university working with someone with 3years in the business (not much but alot more than me). Here is an example of his code

ArrayList al = new ArrayList();

// al gets populated so has stuff in

int count = 0;

for (int i=0; i<al.Count; i++)
{
count++;
}

return count;

And this guy was earning far more than me since he was contracting. I wondered why I even bothered sometimes. Not to mention the webpage he wrote that took approximately 30seconds to be generated. Once he left I rewrote it to take around 1second. And im still on my rubbish wage.

This is *NOT* a WTF... anyone knows a sufficiently intelligent compiler would optimize the loop out of existence in the machine code. Heh and LOL0rz.

2006-08-08 Reply Admin

GoatCheez:
I believe every iteration of this would hit the database again, as I don't think the count will be cached by the compiler in an attempt to optimize the loop.

Off course it's not cached - how should the compiler know that the same value will be returned every time? If another user adds a role to the database while we're in the loop the count will increase, and we will get the new one in our output as well (unless it is inserted somewhere before the last one we've allready extracted off course, in that case we will get one role twice and missing the new one).

Bus Raker · 2006-08-08 Reply Admin

dtfinch:
One of them probably deserves most of the blame, having written 90% of the code. The other nine just watched, discussed, etc.

The other 9 wrote:

public class paulaBean {
private String paula = "Brillant";
public String getPaula() {
return paula;
}
}

snoofle · 2006-08-08 Reply Admin

GoatCheez:
For all of you that are missing why this is SO HORRIBLY DOWNRIGHT AWFULLY HORRENDOUS, I'll explaing... although I probably shouldn't since you should know better anyway!!!

First, I'll just note that it looks like GetRoles was created to retrieve the roles without hitting the database. (If it wasn't, then it's a COMPLETELY USELESS function.)

Now...

The first conditional (that means an if statement) calls getRols. getRols it is pointed out hits the database to retrieve the information it returns. The conditional however only checks that the return value is not null, indicating that the call succeded and was able to return some sort of information (an arraylist).

Next, we create the array list that our "non-database-hitting-function" will return.

Now, we start a while loop, saying that until our return list is the size of the database list, we do things (see below). I believe every iteration of this would hit the database again, as I don't think the count will be cached by the compiler in an attempt to optimize the loop. Don't quote me on that though, it could vary greatly depending on the compiler, and other things as well.... Skepticism leads me to believe though that this will in fact hit the database for every iteration to check against the count.

Now, we are in the loop.... in here, we HIT THE DATABASE AGAIN! This time, it's to index a role in the list that is returned, so we can set our corresponding item in our list to that role.

Finally, the new list is returned... only if they had known though that
public ArrayList GetRoles()
{
  return getRols();
}
was all the code they needed for this useless function. Well, they should have at least seen that all their code was doing was the above.... This is definitely a good WTF today lol.

So to recap, let's say that there were 7 roles. Calling this function would result in hitting the database...

initial check
while check 0 == true
      general hit
while check 1 == true
      general hit
   while check 2 == true
      general hit
while check 3 == true
      general hit
   while check 4 == true
      general hit
while check 5 == true
      general hit
   while check 6 == true
      general hit
while check 7 == false

16 times... wow... that's REALLY bad... REALLY REALLY bad. Are you sure these were junior developers and were not monkeys?

;-P

You, Sir, have sullied the good name of Monkey!

GoatCheez · 2006-08-08 Reply Admin

Anonymous:
GoatCheez:
I believe every iteration of this would hit the database again, as I don't think the count will be cached by the compiler in an attempt to optimize the loop.

Off course it's not cached - how should the compiler know that the same value will be returned every time? If another user adds a role to the database while we're in the loop the count will increase, and we will get the new one in our output as well (unless it is inserted somewhere before the last one we've allready extracted off course, in that case we will get one role twice and missing the new one).

I was just considering that the compiler might recognize that while in that loop, that the call to Count won't change, and thus optimize. It's not unheard of, just really unlikely, especially since it's using a property of an object returned from a function....

Ok, I bascially said it because I know that if I had not, some fark on here would ream me for it, and prove otherwise for some specific case... blah!

(just look above, and continue to read, someone will surely post it! lol)

2006-08-08 Reply Admin

Reminds me of grade school:

Get Class

Get Class Roles

Role Class, Roles

captcha= Doesn't posting the captcha messages defeat the whole purpose of captcha?

its me · 2006-08-08 Reply Admin

jspenguin:
No... roles.Count increases with each item you add to it... I think. Is that C#? It's not Java.

If it's C# it's got a Java accent....

If I understand just WTF they're trying to do, this would be a standard C# implementation:

        private Role[] roles;
        private Role[] GetRolesFromDB()
        {
            return blah; //db implementation
        }

        public Role[] Roles
        {
            get
            {
                if (roles == null) roles = GetRolesFromDB();
                return roles;
            }
        }

Note the change from ArrayList to standard array.... Unless they're going to implement add/remove logic in a custom collection you probably don't want a mutable array....

-me

2006-08-08 Reply Admin

Hey now. Us junior devs are just trying. Well, some of us.

(Besides... the wtf is not that bad as long as the # of roles are <= 1.)

//runs away... quickly

2006-08-08 Reply Admin

You know your in trouble when you run into naming convention problems like GetRols() & GetRoles(). At that point you have to think there is a better way to do the thing!

SomebodyElse · 2006-08-08 Reply Admin

Anonymous:
every call to getRols() performs a multiple-row-resultant query on the database

so let's expand this function (translated into php for clarity), and include what it's calling

I don't know about the rest of you, but putting it into php certainly doesn't help me with the clarity thing, more like obfuscation with all those damn $.

2006-08-08 Reply Admin

It was supposed to be a caching mechanism. Just a mistake in the first != comparison which should have been ==. The idea would be if the roles were already retrieved from the database, don't fetch them again. Unfortunately for everyone the code still worked ... and in testing, with no load on the system, went completely undetected.

kmerkle · 2006-08-08 Reply Admin

Foosball girl could write better code than that.

TankerJoe · 2006-08-08 Reply Admin

Anonymous:
Anonymous:
In Java, (I'm sure C#, a blatant copy of Java, has similiar) you could reduce it to...

C'mon. Everyone knows that C# is a blatant copy of Delphi, not Java. :)

Yeah! And Delphi is a blatant copy of pascal... oh wait.

2006-08-08 Reply Admin

Try this one!!

public List<RolesObj> GetAllRoles()

{

List<Roles> roles = null;

DataTable dt = null;

try

{

// Execute the stored procedure, first we will get corrospondence transactions

dt = ExecDataTable("dbo.transaction_types_get_all", null);

// Pull out the data

if (dt != null && dt.Rows.Count > 0)

{

roles = new List<RoleObj>();

for (int i = 0; i < dt.Rows.Count; i++)

{

RoleObj r = new RoleObj();

r.Id = GetInt(dt.Rows[i]["id"]);

r.Description = GetString(dt.Rows[i]["description"]);

roles.Add(r);

}

catch (Exception e)

{

// we could log this here if we want, but at this point lets just throw these up the chain

throw e;

}

// return the collection (or if no records returned this will be null)

return roles;

}

2006-08-08 Reply Admin

Anonymous:
It was supposed to be a caching mechanism. Just a mistake in the first != comparison which should have been ==. The idea would be if the roles were already retrieved from the database, don't fetch them again. Unfortunately for everyone the code still worked ... and in testing, with no load on the system, went completely undetected.

There is no way it could be a planned caching mechanism. For one thing caching needs a static variable and there is clearly a call to getRols in the line you are referring to. Not to mention that each item is pulled off the array, then typecasted from a generic object to it's real base class then it's pushed into another array which could accept it as a generic object as well.

BTW, if someone tries to advocate this code, he is certainly within the group of the original developers.

The Juniors' Whopper

Leave a comment on “The Juniors' Whopper”