The good news is, I can write about it now.


  • Trolleybus Mechanic

    I make it a personal policy to never write a "wtf" about a company I'm working for.  So since I was informed this morning my contract actually _hasn't_ been extended, let the fun begin!

    Aside from the myriad of unsanitized db inputs, access control lists being stored in cookies, and the entire system breaking if register_globals = off (php... hehehe), the Top Three for me are:

    A Shaky Understanding of Everything

    I was told, by a manager, to go through the code I'd just written, and take out all the recursion. When I asked why, she said it was too difficult for non-programmers to understand the code when they go in to change it. Why would a non-programmer need to change the code?  Oh, so they can do things like bold titles and make the left-justified and such.

    The non-programmers also didn't understand what all those CSS classes were for. 

    Row Row Row your wtf:

    And speaking of recursion, it isn't something the programmers grokked either.  A snippet from the menu-rendering code on each page:


    $sql = "select * from menu_items where parent is null";

      $rows =exec($sql);

      foreach($rows as $row)

          // html for menu item, including a 3-page wide multiple ? : statements that all do the same thing

         $sqlsql = "select * from menu_items where parent = " . $row['id'];

         $rowsrows = exec($sqlsql);

         foreach ($rowsrows as $rowrow)

             // html for second-level menu item, copy and pasted from above

             $sqlsqlsql = "select * from menu_items where parent = " . $rowrow['id'];

             // And so forth, up to $sqlsqlsqlsqlsql

     

    Wait, what?

    And to this day, I still don't know what this chunk does.  It may be some attempt to prevent duplicate replies, but since there's no documentation, code comments or error messages, I guess I'll just never know.

    $md5 = "";

    $sql = "INSERT INTO replies (author, comment_text, md5) VALUES ($author, $comment_text, $md5);";

    $md5 = md5($sql);

    $sql = "INSERT INTO replies(author, comment_text, md5) VALUES ($author, $comment_text, $md5);";

    $sql2 = "SELECT * FROM replies WHERE md5 = $md5";

    $rows = exec($sql2);

    if (count($rows) == 0)

    {

       exec($sql);

    }

    And before you ask, no, there was no index on the md5 column of the replies table.  And no, I don't know why, if this was to prevent dupes, they didn't just do md5("$author|$comment_text");

    I Guest That's A Way To Do It

    And finally (my "top 3" was zero-indexed), very recently, one dev asked another how to inside 0 into an auto-increment table. He was told to alter the table to drop the AUTO_INCREMENT constaint, insert his row, then re-enable the AUTO_INCREMENT constraint.  (Note, I just tested this, and it doesn't work.  Re-adding the AUTO_INCREMENT changes the row 0)

    That seemed odd, so I asked instead WHY he neededs a zero row?  "It's in the user table. I need a guest account, and I don't want the guest account to show up in the user list".

     Why not just add another column to the table-- is_guest or something?

    Well,  then he'd have to change the code everywhere to check "if ! user[is_guest] then..."

    Oh.  But, how are you going to keep the user from showing up in the list as it is now?

    Simple.  Just change the code everywhere the user list is used to say "if id > 0 then dowhaever...."

    ---

    Resume, resume, where art thou, o resume?



  •  For the fourth one, it's definately a bad idea...  The question I'd have is why not just make the guest a pseudo account (where it doesn't have a record in the DB, but the user object defaults to 0 if unpopulated)...  Then you don't need to change any SQL (unless you're joining on the table to the users table, but by the looks of what you've shown, I'm not sure that they know about Joins)... But it's possible with something like:


         SET @oldmode =  @@SESSION.sql_mode;

         SET @@SESSION.sql_mode = 'NO_AUTO_VALUE_ON_ZERO';

         /// Do your insert/update/wahtever

         SET @@SESSION.sql_mode = @oldmode;

     

    For the third one, I could see why they did that.  They didn't know enough about collations to know that = obeys the connection collation, but insert obeys the column's collation.  So if they are different, SELECT id FROM comments WHERE author = $author and comment = $comment_text may not return a result even if it is in there (so if you have a unique index set on (author, comment), it may fail even though the select returned 0 rows).  MD5ing the whole query is one may to make sure that the same query wasn't executed before.  The better option would just be to fix your collations, but that would be too easy, right?

     

    For the second one, I've seen that quite often.  Though typically it's hidden behind a recursive function.  Sure, the recursive is more general as it doesn't hardcode the max depth, but still wow...  So something like

    function loadMenuByParentId($parent = null) { 

        //load menu items

        foreach ($menus as &$menu) {

            $menu['children'] = loadMenuByParentId($menu['id']);

        }

        return $menus;

    }

     

    As far as the first, I'd love to see what the guy thinks about objects and inheritance...



  • @Lorne Kates said:

    (my "top 3" was zero-indexed)

    Clearly you don't understand how zero indexing works.  An array that is zero-indexed and has 3 objects in it, still has 3 objects in it.



  • @morbiuswilters said:

    An array that is zero-indexed and has 3 objects in it, still has 3 objects in it.
    That's just superstition.


  • Trolleybus Mechanic

    @morbiuswilters said:

    @Lorne Kates said:

    (my "top 3" was zero-indexed)

    Clearly you don't understand how zero indexing works.  An array that is zero-indexed and has 3 objects in it, still has 3 objects in it.

     

     I know, but if I didn't make a mistake, people would think my account got hacked or something.



  • @Lorne Kates said:

    That seemed odd, so I asked instead WHY he neededs a zero row?  "It's in the user table. I need a guest account, and I don't want the guest account to show up in the user list".

     Why not just add another column to the table-- is_guest or something?

    Well,  then he'd have to change the code everywhere to check "if ! user[is_guest] then..."

    Oh.  But, how are you going to keep the user from showing up in the list as it is now?

    Simple.  Just change the code everywhere the user list is used to say "if id > 0 then dowhaever...."

     

    Your solution doesn't make sense here.

    If there's only to be one row with a guest then,

    a) you still have to have one id with the guest details (and is_guest set to true)

    b) every other row will have is_guest set to 0

    Since you will know the row id of the guest row, whether it be 0, 10 or 251160 there's no point making another column to say the same thing.

    Personally I think having 0 for a guest row is perfectly reasonable.

    Of course if I misunderstood and there are to be multiple guest rows, then your solution is better.


  • Trolleybus Mechanic

    @bairy said:

    Since you will know the row id of the guest row, whether it be 0, 10 or 251160 there's no point making another column to say the same thing.
     

    Well, the problem was two-fold.  First, he wanted to alter the database schema to accomidate the guest account. The way he was given wouldn't work-- and would break every INSERT INTO ... ON DUPLICATE KEY UPDATE statement (since 0 would suddenly not be a duplicate key).

    Second, the reason he gave for not wanting to use a guest flag was that he'd have to put extra logic into the code to check the is_guest flag. But he already has to put in a logic check anyways-- in the exact same places-- to check if id == 0.

    So, for the exact same amount of code changing, my solution doesn't break the db schema, and is more extensible.

    Did I mention I'm not too broken up about moving past that place? 

     


  • Trolleybus Mechanic

    @ircmaxell said:

    The question I'd have is why not just make the guest a pseudo account (where it doesn't have a record in the DB, but the user object defaults to 0 if unpopulated)... 

    I was going to ask that, but was dumbstruck by comment about code changing. And they don't check for nulls when stuff comes out of the db.


    Then you don't need to change any SQL (unless you're joining on the table to the users table, but by the looks of what you've shown, I'm not sure that they know about Joins)...

    Again, asked about that.  "I'll just disabled the foreign key checks on all the tables".

     

    As far as the first, I'd love to see what the guy thinks about objects and inheritance...

    Something like this:


    class UserObject

    {

      function __construct($id)

      {

          $this->id = $id;

      }

     function loadUser()

     {

        $conn = HardCodedSqlConnectionWithUsernameAndPassword;

       $sql = "select * from users where id = " . $this->id;

       $data = exec($sql);

       $this->userData = $data;

       return $data; 

     }

    }

     

     



  • I think front page power is driving you insane. You've started formatting forum posts as front page articles.



  • @Spectre said:

    "I think front page power is driving you insane," Spectre writes.  "You've started formatting forum posts as front page articles.
    FTFY



  • The true wtf in part 1, is that they don't use a system to seperate html and the code.You don't want graphics guys to edit your code at all. (Especially if they use dreamweaver. ARG!)

    The true wtf in the auto_increment part, is that last time i checked with mysql, you CAN specify a value for an auto_increment field, so you could just insert an row with id=0. (Yes, that would still be a wtf solution, but hey).

    And the md5 thing is funny. Don't mysql support a unique/distinct on a field? That way you could just do the insert in a single query, and if it failed the row were already there. And if you calculate the md5 from just the comment_text you would get the bonus that if multiple different users mede a "First post!" message, only the first one would be saved in the database :}

     



  • @bstorer said:

    @Spectre said:


     

    "I think front page power is driving you insane," Spectre writes.

     

    It was a standard PHP WTF, but Spectre was fresh out of collage and excited to leave his comment so that others, like belgariontheking, might read of it.

     

    "You've started formatting forum posts as front page articles," continued Richard.

    FTFY

    FTFY.



  • @morbiuswilters said:

    @bstorer said:

    @Spectre said:


     

    "I think front page power is driving you insane," Spectre writes.

     

    It was a standard PHP WTF, but Spectre was fresh out of collage and excited to leave his comment so that others, like belgariontheking, might read of it.

     

    "You've started formatting forum posts as front page articles," continued Richard.

    FTFY

    FTFY.

    FTFY

  • Trolleybus Mechanic

     





  •  <3



  • Epic derailment.
    I think I like the derail more than the OP, and the OP was fairly decent.



  • @tiller said:

    And the md5 thing is funny.

    It's not funny, it's just wrong! if they do the "$md5 = md5($sql)" only once, the md5 is the md5 of the previous iteration. They would have to keep iterating until it converges on a fixed point. I just hope their server is powerful enough (or their users are very patient).



  • @Lorne Kates said:

    And speaking of recursion, it isn't something the programmers grokked either.
    Maybe the programmers grokked recursion, but didn't use it because the manager told them not to.

    @Lorne Kates said:

    And finally (my "top 3" was zero-indexed), very recently, one dev asked another how to inside 0 into an auto-increment table. He was told to alter the table to drop the AUTO_INCREMENT constaint, insert his row, then re-enable the AUTO_INCREMENT constraint.  (Note, I just tested this, and it doesn't work.  Re-adding the AUTO_INCREMENT changes the row 0)
    He should have used -1 instead. Auto_increment won't change that and id > 0 would still work.

    @Lorne Kates said:

    Resume, resume, where art thou, o resume?
    On Wtf Resume Next?

     

     

     


  • Trolleybus Mechanic

    @julmu said:

    @Lorne Kates said:
    And speaking of recursion, it isn't something the programmers grokked either.
    Maybe the programmers grokked recursion, but didn't use it because the manager told them not to.

    The programmers never used recursion because the management didn't understand it,and the management didn't understand it because the programmers didn't use it.

    (Noun: See Recursion)

    @julmu said:


    @Lorne Kates said:

    And finally (my "top 3" was zero-indexed), very recently, one dev asked another how to inside 0 into an auto-increment table. He was told to alter the table to drop the AUTO_INCREMENT constaint, insert his row, then re-enable the AUTO_INCREMENT constraint.  (Note, I just tested this, and it doesn't work.  Re-adding the AUTO_INCREMENT changes the row 0)
    He should have used -1 instead. Auto_increment won't change that and id > 0 would still work.

    Yup-- though he'd still have to change the code to check where id < 0 (and remember, he didn't want to use is_guest because it would require a code change...)

    @julmu said:


    @Lorne Kates said:

    Resume, resume, where art thou, o resume?
    On Wtf Resume Next?

    Epic. Sig'd.

     

     

     


  • If any solution for the guest row is going to lead to one and only one guest, then add the guest row, query the database for the guest id, and change the code so that it's

    if user_id <> guest_id then :

    Whatever

    End if

    No need to drop the auto increment constraint, no need to add an is_guest table.

     Of course, adding some sort of role based logic and assigning the guest account limited priveledges would be nice.

    And, having a "hidden" user account sounds pretty much like a moajor wtf waiting to happen.

    " Hey guys, someone deleted all the _____ data!  Can anyone tell me who was logged on between X and Y?"

    Print the user log.  No one shows up as logged on.

    "Nobody.  The data deleted itself."

    Invisible Guest: "Bwa ha ha!!"



  • I'm not a DBA, but I think that menu code would make my head explode. That's a truckload of SQL commands, and yeah they're simple, but if they're on each page, that can really add up (I imagine). I'd either just get the whole menu table, and build the table in code, or a create a static menu file once every hour or so.



  •  Hopefully, they have the query cache enabled and, as the menu_items table hardly ever changes, the second time the page is loaded everything is retrieved from cache.

     So it's not  really such a big deal, but either way it's a bad chunk of code. IF you have to retrieve the menu items from sql, it would make sense to retrieve the full list  of menu items and store it in an array and generate the menu recursively.


Log in to reply