Who needs ||?



  •  Been running across this bit of code a lot lately (in java).

     if (list != null && list.size() > 0) { } else {

       //do stuff

    }

    I think the original author skipped the day in class where they discussed the OR operator.



  • I think this is my favorite anti-pattern. 



  • I like to call it the "If-Lies-Then" pattern.



  •  @morbiuswilters said:

    I think this is my favorite anti-pattern. 

    I think this is just a fail-pattern. The anti-pattern would be:

    if ( list == null ) {

      //do stuff

      } else {

        if ( list.size() == 0 ) {

        //do stuff

      }

    }



  • I'm hoping the do something populates the list instead of trying to do something with it, otherwise this is a double wtf



  •  Actually, I like this code. The conditional reads positively, and is immediately visually correct. The action case is the else, but who cares? !(cond) may be preferred by some, but -- if (list == null || list.size() == 0) { do stuff } is much more prone to error when it reaches maintainance.

    No "wtf" here...



  • Probably removed a statement and didn't bother to restructure it.



  • @ratboy667 said:

    Actually, I like this code. The conditional reads positively, and is immediately visually correct. The action case is the else, but who cares? !(cond) may be preferred by some, but -- if (list == null || list.size() == 0) { do stuff } is much more prone to error when it reaches maintainance.

    No "wtf" here...

    You're kidding, right?

     

    Please tell me he's kidding... 



  • I find myself guilty of doing this occasionally, when it's very late and I can't be assed to remember de Morgan correctly, and think that just negating it all is ugly. I usually fix it the next time my brain is working though.



  •  I have been known to do something similar - when the conditions are clear one way, but not the other, e.g:

    if( ((option_one != null) &&  (option_one.valid())) || ((option_two != null) && (option_two.valid())))

    {

        // We have at least one valid option

    }

    else

    {

        DoErrorReport()

    }

     



  • @GettinSadda said:

    if( ((option_one != null) &&  (option_one.valid())) || ((option_two != null) && (option_two.valid())))

     

    Needs a container function.

     Seriously, I know a guy who is so fond of Eclipses "Quick Fix"/"Ctrl+1" function that his code reads like this:

     

    function isLargerOrEqualToEightButSmallerThanTen(int var){
        return (var >= 8 && a < 10);
    }
    

    function isSmallerThanEight(int var){
    return (var < 8);
    }

    if(isLargerOrEqualToEightButSmallerThanTen(var)){
    // do something
    }else if(isSmallerThanEight(var){
    // do something
    }else{
    // do something
    }

    // instead of:

    if(var >= 8 && var < 10){
    // do something
    }else if(var < 8){
    // do something
    }else{
    // do something
    }



  • I'm going to be a dope for a minute here. Is the WTF just that the else block is the one that contains the action? The condition looks ok: make sure the list isn't null, then make sure it's not empty.

    Or is it just better to write it as "if ((list == null) || (list.size() == 0))" and that's the WTF? I'm missing something here (and today I can't even use lack of coffee as an excuse).



  • @Justice said:

    Or is it just better to write it as "if ((list == null) || (list.size() == 0))" and that's the WTF?
     

    Yes it is. then you don't need the else block and can just put the stuff in the if block.

     

    if(something()){
    }else{
        foo();
    }
    

    if(!something()){
    foo();
    }



  • @ratboy667 said:

     Actually, I like this code. The conditional reads positively, and is immediately visually correct. The action case is the else, but who cares? !(cond) may be preferred by some, but -- if (list == null || list.size() == 0) { do stuff } is much more prone to error when it reaches maintainance.

    No "wtf" here...

     

    I hope I never have to maintain your code if you think that is a good idea.

     If you really think (list == null || list.size() == 0) is hard to maintain, then put it in a method and call that.  E.g. "if (listIsEmpty(list))...".  I could handle that and it reads well.  Abusing control structures is not how you write maintainable code.



  • Morbiuswilters

    I am not kidding. I am sane. When teaching, I try to impress several "good practices" to students. I will summarize the "if rules" for you:

    1 - Keep conditions positive. Negatives are hardy to comprehend, and repair. This falls into that category. Yes, OR can be used, but AND is more legible. Yes, NOT can be used, but decreases reability. DeMorgan has his place, but we read left to right, and have a limited capacity to retain earlier information (unless, of course, German (or Forth) is your native language, in which case a trailing NOT may be your speed). We write code for the (presumably less talented) maintainers coming later.

    2 - Keep range arrows the same way. Instead of

    if ((i >= 5) && (i <= 10))

    we prefer

    if ((5 <= i) && (i <=10))

    Notice that this is clearly "if (i in 5..10)". Again, it's for the maintainers.

    3. Keeping rules 1 and 2 in mind, empty code blocks are evil, unless they are clearly marked. I prefer, in C:

    #define NOTHING

     

    if (positive_condition)

      NOTHING;

    else {

      ...something...

    }

    4.Use axiomatic code as much as possible, including variable names (for purpose) and structures.

    It is clear that the empty block is meant to be in rule 3. This would be the WTF in the example (in my humble, but usually correct and incisive opinion). Of course, Java has no direct way of expressing. There should have been a comment in the empty block. The reasoning is that "(list != null && ..subcondition involving list...)" would be almost an axiomatic way to write an expression involving list; a standard guard. Writing that part as "list == null || ..." adds confusion when reading the code in order to maintain it.

     

     



  • @ratboy667 said:

    Of course, Java has no direct way of expressing.

    Because it implicitly discourages the use of Java, I will support the ludicrous assertions of this post exclusively on that principle.

    What? Java developers do the opposite all the time. Fight fire with fire, right?



  • @dtech said:

    @Justice said:

    Or is it just better to write it as "if ((list == null) || (list.size() == 0))" and that's the WTF?
     

    Yes it is. then you don't need the else block and can just put the stuff in the if block.

     

    if(something()){
    }else{
        foo();
    }
    

    if(!something()){
    foo();
    }

    Holy shit my sense of sanity made me miss the if(...){} else {  /* code
    here */ } ... The entire time I just thought "this guy just does not know how to write easy to spot if conditions" thend tech opened my eyes...

    Yea this is an awesome anti-pattern. This one goes into
    the gallary.

    Almost as cool as:

    switch(variable){

      case 1: break;

      case 2: break;

      case 3: break;

      // Only possible values are 1-4

      default: do stuff; break;

    }


  • Discourse touched me in a no-no place

    @ratboy667 said:

    2 - Keep range arrows the same way. Instead of

    if ((i >= 5) && (i <= 10))

    we prefer

    if ((5 <= i) && (i <=10))

    Yeuck.

     @ratboy667 said:

    #define NOTHING
    You're kidding me, yes?

    @ratboy667 said:

    When teaching, ...
    Ah, that explains a lot.

     

     



  • @PJH said:

    @ratboy667 said:

    When teaching, ...
    Ah, that explains a lot.

    It apparently is hard to find good teachers these days.  Who really wants to teach ungrateful tenage fucktards for 35k a year?



  • @ratboy667 said:

    (positive_condition)

      NOTHING;

    else {

      ...something...

    }

    That looks pretty bad to me.  The best "nothing" is a comment which explains why the execution got to that point in the code.  eg. "/*List is empty*/".  I will often also put the description of the other condition into the ELSE block, such as "/*List has at least one member*/".  This is useful when the two blocks grow so much that the beginning of the ELSE block is more than half a screenful away from the IF statement.  Once again, this is for maintainability, since you can usually understand what you're writing while you're writing it. 

    For best maintainability, I will of course use the actual name of the list contents, so even if I used a simple variable name like "eList" I can put in little reminders that this is the "list of employees to be searched" or whatever the code is really doing with the list.

    Usually, it's not even for maintainability but the code grows as the code is tested.  I start out with code that just halts if the list is empty (empty THEN block) and only put it in later if it turns out that this is a common problem and needs a proper error message or recovery logic.

    If you have to #define a "nothing" then you're doing it wrong.


  • Discourse touched me in a no-no place

    @amischiefr said:

    Who really wants to teach ungrateful tenage fucktards for 35k a year?
    Is programming (at this level anyway) taught to 12-16 yr olds? I find it surprising if it is. In edu.uk I got the impression they had insufficient time to teach English and Maths, let alone the more esoteric subjects like programming.



  • @PJH said:

    @amischiefr said:

    Who really wants to teach ungrateful tenage fucktards for 35k a year?
    Is programming (at this level anyway) taught to 12-16 yr olds? I find it surprising if it is. In edu.uk I got the impression they had insufficient time to teach English and Maths, let alone the more esoteric subjects like programming.

    In the US, some high schools have programming classes.  Not in the heavily-unionized schools which are incapable of teaching the fundamentals, mind you, but in the better charter and magnet schools, yes.


Log in to reply