Comment On The Juniors' Whopper

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

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.

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

You must be one of the junior developers. :)

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

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

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.

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...

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

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?

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!

I don't get it.

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)

This is basically doing the following in wtfey way.

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

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.

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?

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.

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

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!

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.

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!  

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.

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

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

CAPTCHA: whiskey! woohoo  

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

CAPTCHA: enterprisey - another winner!

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.

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.  :)

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

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;
}

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

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.

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.

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

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

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

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.

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

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;
  }
}

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!

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)

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

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

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!

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 $.

snoofle:

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!

I'm fairly sure that the monkeys would have gotten this one right.

http://www.newtechusa.com/ppi/main.asp

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.

Foosball girl could write better code than that.

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.

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;

}