Missing the point



  • Found this recently. The developer who came up with this is one of the most experienced and best-paid I've ever worked with. If nothing else, it's further corroborating evidence for my lack of belief in a correlation between years of experience and cluefulness.

    for ($i = 1; $i < 6; $i++) {
        if ($i == 1) $entity->setNickname1($nicknames['nickname' . $i]);
        if ($i == 2) $entity->setNickname2($nicknames['nickname' . $i]);
        if ($i == 3) $entity->setNickname3($nicknames['nickname' . $i]);
        if ($i == 4) $entity->setNickname4($nicknames['nickname' . $i]);
        if ($i == 5) $entity->setNickname5($nicknames['nickname' . $i]);
    }

    This is from some code in charge of updating an entity in our system. This entity has a strange 5-to-1 relationship with another entity whose name I have anonymised as "nickname". To be clear: what I'm drawing attention to here is specifically this developer's for-loop implementation.

    This is that special type of code which compiles, executes, and fulfills its requirements, but still fills the heart with misery. It's a for loop with none of the benefits of a for loop. It performs a whole bunch of time-wasting integer incrementations and comparisons, but that's all the loop achieves. There's a line for each iteration, and when the time comes to change the relationship's multiplicity, the loop means more work, not less.

    Better still, as part of the anonymisation process before posting here, I removed one of this person's trademark worthless comments. This comment acknowledges the low quality of the code and somehow implies that it's somebody else's fault. This infuriating comment has the temerity to end with a fucking smiley :-)


  • Discourse touched me in a no-no place

    So, were the methods called setNickname1…5 (well, with the real thing instead of “Nickname” of course)? That's a whole chunk of code smell right there. Whole layers of code malodorousness, in fact.



  • It is called the "for case antipattern", or at least some mild variation on it.



  • Needs more reflection.

    for ($i = 1; $i < 6; $i++) {
        $ref = 'setNickname' . $i;
        $entity->$ref($nicknames['nickname' . $i]);
    }



  • @Salamander said:

    Needs more reflection.



    for ($i = 1; $i < 6; $i++) {

        $ref = 'setNickname' . $i;

        $entity->$ref($nicknames['nickname' . $i]);

    }


    This is the ONE case where reflection is more elegant than the same code without reflection. But why the fuck is $nicknames not indexed by integers? Why is setNickname1 not just $this->nicknames[0] = $nickname;? What kind of drugs was the person who wrote this code on, and where can I get some?



  • @Ben L. said:

    But why the fuck is $nicknames not indexed by integers? Why is setNickname1 not just $this->nicknames[0] = $nickname;?

    To give the author the benefit of the doubt, perhaps each SetNicknameX does something different from $this->nicknames[0]=$nickname, and different from each other SetNicknameX? (perhaps that would be TRWTF? Maybe those methods actually need more descriptive names?)

    EDIT: Also, I would consider that one of the less elegant uses of reflection I've seen



  • @Ben L. said:

    This is the ONE case where reflection is more elegant than the same code without reflection.

    I'd still much rather just write out the 5 versions explicitly. Reflection makes me sick.

    @Ben L. said:

    But why the fuck is $nicknames not indexed by integers?

    I'm guessing that the array is being used like a struct and there is other stuff in there, too.



  • @GNU Pepper said:

    Better still, as part of the anonymisation process before posting here, I removed one of this person's trademark worthless comments. This comment acknowledges the low quality of the code and somehow implies that it's somebody else's fault. This infuriating comment has the temerity to end with a fucking smiley :-)
     

    I know the regular smiley, the winking smiley, the glasses-wearing smiley, the sticking-your-tongue-out smiley, and quite a few others.  But I don't believe I'm familiar with the fucking smiley.



  • @da Doctah said:

    @GNU Pepper said:

    Better still, as part of the anonymisation process before posting here, I removed one of this person's trademark worthless comments. This comment acknowledges the low quality of the code and somehow implies that it's somebody else's fault. This infuriating comment has the temerity to end with a fucking smiley :-)
     

    I know the regular smiley, the winking smiley, the glasses-wearing smiley, the sticking-your-tongue-out smiley, and quite a few others.  But I don't believe I'm familiar with the fucking smiley.

    I believe it's 8==D~.

     



  • @Salamander said:

    I'd still much rather just write out the 5 versions explicitly. Reflection makes me sick.

    Really? Odd. Admittedly, you shouldn't have a bunch of setNicknameX methods where reflection is necessary, but I much prefer it to having the blech of 5 lines of the same crap over and over.



  • @Snowyowl said:

    I believe it's 8==D~. 8======D ~o ~o ~o ~o     >^,,^<

    Morbs'd That For You.



  • I think this forum needs a new Donald Trump inspired feature: "The Apprentice poll".



    Whenever a WTF is posted, we should get to specify the email address of the line manager of the perpetrator.

    Then, every time some stupid shit like this is caused by a some jumped up, overpaid berk, we can all vote for "You're fired."



    After a few days worth of responses are collected, the result of the poll, and the full transcript of the thread is then sent to

    the guys boss, with a little bit of text prefacing it to explain that we, the people who know about this sort of thing, have spoken.



    Lets start making this site useful.



  • @morbiuswilters said:

    @Salamander said:
    I'd still much rather just write out the 5 versions explicitly. Reflection makes me sick.
    Really? Odd. Admittedly, you shouldn't have a bunch of setNicknameX methods where reflection is necessary, but I much prefer it to having the blech of 5 lines of the same crap over and over.
    I bet you wouldn't say this if this were a language like, say, Java... Reflection in Java is atrocious (well, more atrocious than just plain Java).

    		try {
    			entity.getClass().getMethod("setNickname" + i, new Class<?>[] { String.class }).invoke(entity, nicknames[i]);
    		} catch (SecurityException e) {
    			// oh shit, we're fucked
    		} catch (NoSuchMethodException e) {
    			// how did we even get here?
    		}
     

    Blegh.


  • @electronerd said:

    To give the author the benefit of the doubt, perhaps each SetNicknameX does something different from $this->nicknames[0]=$nickname, and different from each other SetNicknameX? (perhaps that would be TRWTF? Maybe those methods actually need more descriptive names?)
    I think you are... missing the point. Why have the for-loop? Just write:

    $entity->setNickname1($nicknames['nickname1']);
    $entity->setNickname2($nicknames['nickname2']);
    $entity->setNickname3($nicknames['nickname3']);
    $entity->setNickname4($nicknames['nickname4']);
    $entity->setNickname5($nicknames['nickname5']);
    


  • I'll assume the original code is PHP. I'll rewrite it to use variable variables :)

    for ($i = 1; $i < 6; $i++) {

        $entity->{'setNickname'.$i}($nicknames['nickname' . $i]);

    }



  • @Zecc said:

    @electronerd said:

    To give the author the benefit of the doubt, perhaps each SetNicknameX does something different from $this->nicknames[0]=$nickname, and different from each other SetNicknameX? (perhaps that would be TRWTF? Maybe those methods actually need more descriptive names?)
    I think you are... missing the point. Why have the for-loop? Just write:

    $entity->setNickname1($nicknames['nickname1']);
    $entity->setNickname2($nicknames['nickname2']);
    $entity->setNickname3($nicknames['nickname3']);
    $entity->setNickname4($nicknames['nickname4']);
    $entity->setNickname5($nicknames['nickname5']);

    No, I got that point. I was just responding to the one part of the one post that I quoted, the part where Ben L. made a gross assumption about the implementations of SetNicknameX (probably an accurate one, but still...)



  • Fair enough.



  • @morbiuswilters said:

    @Snowyowl said:
    I believe it's 8==D~. 8======D ~o ~o ~o ~o     >^,,^<

    Morbs'd That For You.


    Is that supposed to be Ben L. taking the money shot?



  • @mikeTheLiar said:

    @morbiuswilters said:
    @Snowyowl said:
    I believe it's 8==D~. 8======D ~o ~o ~o ~o     >^,,^<

    Morbs'd That For You.


    Is that supposed to be Ben L. taking the money shot?
    @morbiuswilters said:
    Filed under: The cat is Ben L. Not an actual cat.



  • @Zecc said:

    @mikeTheLiar said:

    @morbiuswilters said:
    @Snowyowl said:
    I believe it's 8==D~. 8======D ~o ~o ~o ~o     >^,,^<
    Morbs'd That For You.
    Is that supposed to be Ben L. taking the money shot?
    @morbiuswilters said:
    Filed under: The cat is Ben L. Not an actual cat.


    Derp. Missed that under morb's huge signature.



  • @mikeTheLiar said:

    Derp. Missed that under morb's huge signature.

    I agree with whatever Morbs just said.



  •  @smxlong said:

    It is called the "for case antipattern", or at least some mild variation on it.

    I love that there is actually a Wikipedia article that cites, as its sole source, The Daily WTF.



  • @Anonymouse said:

    I bet you wouldn't say this if this were a language like, say, Java... Reflection in Java is atrocious (well, more atrocious than just plain Java).

    		try {
    			entity.getClass().getMethod("setNickname" + i, new Class<?>[ { String.class }).invoke(entity, nicknames[i]);
    		} catch (SecurityException e) {
    			// oh shit, we're fucked
    		} catch (NoSuchMethodException e) {
    			// how did we even get here?
    		}
     

    Blegh.

    I still kind of prefer it to 5 lines of nearly-identical code.


  • Considered Harmful

    @morbiuswilters said:

    @Anonymouse said:

    I bet you wouldn't say this if this were a language like, say, Java... Reflection in Java is atrocious (well, more atrocious than just plain Java).

    		try {
    			entity.getClass().getMethod("setNickname" + i, new Class<?>[ { String.class }).invoke(entity, nicknames[i]);
    		} catch (SecurityException e) {
    			// oh shit, we're fucked
    		} catch (NoSuchMethodException e) {
    			// how did we even get here?
    		}
     

    Blegh.

    I still kind of prefer it to 5 lines of nearly-identical code.


    I don't know, I think using reflection to turn this into a loop is one of those "programmer thought it looked boring so made it more complicated for fun." Granted, the original WTF is a WTF, why use five properties when one indexed one would do (also, the no-op for-case), but this workaround smells just as bad.

    Programming Sucks! Or At Least, It Ought To



  • @Shinhan7 said:

    I'll assume the original code is PHP. I'll rewrite it to use variable variables :)

    for ($i = 1; $i < 6; $i++) {

        $entity->{'setNickname'.$i}($nicknames['nickname' . $i]);

    }

     

    that's not ugly enough, future proof version !

    $i=1; while (method_exists($entity,"setNickname$i")) $entity ->  {"setNickname$i"}($nicknames['nickname' . $i++]);

     



  • @joe.edwards said:

    I don't know, I think using reflection to turn this into a loop is one of those "programmer thought it looked boring so made it more complicated for fun."

    I don't like seeing nearly-identical lines of code like that. shrug I think the loop is more concise, without being at all confusing or unclear. I actually find the 5 lines less clear because my first thought is "Why isn't this a loop?" and then I start wondering what kind of WTFy differences between the lines I'm missing. If it was in a loop, it would be easy to see what parts were variable and which weren't.



  • @Shinhan7 said:

    I'll assume the original code is PHP. I'll rewrite it to use variable variables :)

    for ($i = 1; $i < 6; $i++) {

        $entity->{'setNickname'.$i}($nicknames['nickname' . $i]);

    }

    Variable variables are basically just another form of reflection, which is what we're already discussing.


  • Considered Harmful

    @morbiuswilters said:

    @joe.edwards said:
    I don't know, I think using reflection to turn this into a loop is one of those "programmer thought it looked boring so made it more complicated for fun."

    I don't like seeing nearly-identical lines of code like that. shrug I think the loop is more concise, without being at all confusing or unclear. I actually find the 5 lines less clear because my first thought is "Why isn't this a loop?" and then I start wondering what kind of WTFy differences between the lines I'm missing. If it was in a loop, it would be easy to see what parts were variable and which weren't.

    I'm thinking in terms of complexity. It just seems kind of Rube Goldberg to me to use reflection to save a few linesactually add a few lines. You take something that is performant, verifiable, and bug-free, and now you're having to deal with possible exceptions and the overhead of the reflection API and string concatenation, and now there are flow-control structures involved (for loop, catch blocks). You also confound static analysis tools when you use reflection, so when someone decides to fix it, they miss your dependency on the interface. ...and for what measurable gain? The code looks prettier somehow?

    Bah, if it were me, I'd go in and fix the damned interface of the data object to use an indexer.



  • @joe.edwards said:

    and now you're having to deal with possible exceptions and the overhead of the reflection API and string concatenation, and now there are flow-control structures involved (for loop, catch blocks). You also confound static analysis tools when you use reflection, so when someone decides to fix it, they miss your dependency on the interface. ...and for what measurable gain?

    This is PHP, so none of that really applies here.

    @joe.edwards said:

    Bah, if it were me, I'd go in and fix the damned interface of the data object to use an indexer.

    Well, the interface is just retarded, anyway.



  • @morbiuswilters said:

    Well, the interface is just retarded, anyway.
     

    Yes. Yes it is. And he would have been welcome to fix it if he wished. Apparently it was easier to write that godawful loop and derisive comment instead.


Log in to reply