Else return null



  • Not *really* a WTF, more of a pet peeve.

     Why do some people insist on doing this pattern:

    public List<MyObjectType> getMyObject()

    {

      MyManagerObject myManager = getTheManager();

      if ( myManager != null )

      {

        List<MyObjectType> result = new ArrayList<MyObjectType>();

        copyMyObjectsFromManager(myManager, result);

        return result;

      }

      else

      {

        return null;

      }

    }

    *why the else* ???



  • For the same reason it's a good idea to add braces around the if block body even if it contains just a single line?



  • @JoeCool said:

    *why the else* ???

    To make the coder's intent clearer.



  • @frits said:

    @JoeCool said:

    why the else ???

    To make the coder's intent clearer.

    So you don't think this is clear?

    public List<MyObjectType> getMyObject()

    {

      List<MyObjectType> result = null;

      MyManagerObject myManager = getTheManager();

      if ( myManager != null )

      {

        result = new ArrayList<MyObjectType>();

        copyMyObjectsFromManager(myManager, result);

       }

      return result;

    }



  • @JoeCool said:

    @frits said:

    @JoeCool said:

    *why the else* ???

    To make the coder's intent clearer.

    So you don't think this is clear?

    public List<MyObjectType> getMyObject()

    {

      List<MyObjectType> result = null;

      MyManagerObject myManager = getTheManager();

      if ( myManager != null )

      {

        result = new ArrayList<MyObjectType>();

        copyMyObjectsFromManager(myManager, result);

       }

      return result;

    }

    This is fine.  I don't think your OP was clear in that you thought this was the alternate solution. It seemed to me you were complaining about the non-essential "else" block. 

    If you were planning a bait-and-switch troll: kudos, you caught me.



  • @frits said:

    This is fine.  I don't think your OP was clear in that you thought this was the alternate solution. It seemed to me you were complaining about the non-essential "else" block. 

    If you were planning a bait-and-switch troll: kudos, you caught me.

    Sorry if I wasn't clear... I'm not trolling. I was complaining about the non-essential "else" block: my alternate solution was an example of the way I would have done it without doing that else. Not that it is the only way, I just don't care for the original code even though it is technically correct.


  • Trolleybus Mechanic

    Future planning?

    else
    {
         FuckingLogIt("No managers!");
         return null;

    }




  • @Adriano said:

    For the same reason it's a good idea to add braces around the if block body even if it contains just a single line?
     

    hmm... no, that's not it.



  • @dhromed said:

    @Adriano said:

    For the same reason it's a good idea to add braces around the if block body even if it contains just a single line?
     

    hmm... no, that's not it.

    There is one tool called resharper and most people are now following it blindly.



  • Joe, personally, I write my code the way you do. But this is not a WTF, it is merely a style difference. 

    Anger management class maybe???



  • @Rick said:

    Joe, personally, I write my code the way you do. But this is not a WTF, it is merely a style difference. 

    Anger management class maybe???

    hmmm... I'm pretty sure my first line said "not really a WTF, more of a pet peeve". Where did I seem angry?



  • @JoeCool said:

    Where did I seem angry?

     @JoeCool said:

    *why the else* ??? OH WOE IS ME WHYYY????

    Embellishment mine for ultra-hilarious effect.



  • @dhromed said:

    @JoeCool said:

    Where did I seem angry?

     @JoeCool said:

    why the else ??? OH WOE IS ME WHYYY????

    Embellishment mine for ultra-hilarious effect.

    Oh, yes, I remember bolding it and such... yeah, sorry that seemed really angry.



  • My bigger pet peeve is; why return null instead of an empty list?  I think an empty list or an exception are both easier to handle than to have to check for a "null bomb" every time the method is called.



  • @Jaime said:

    My bigger pet peeve is; why return null instead of an empty list?  I think an empty list or an exception are both easier to handle than to have to check for a "null bomb" every time the method is called.

    I disagree - to me returning an empty list means "all went well, and the following is the list of zero results", returning null means "something went wrong"

     



  • @GettinSadda said:

    to me returning an empty list means "all went well, and the following is the list of zero results", returning null means "something went wrong"


    PREEEEEEEcisely. An empty list is a perfectly valid and possible outcome from the GetTheManager call, hence should NOT be used to signal an 'error' condition. But Jaime's right in one way: throwing an exception would be better than just returning NULL, providing the exception is properly handled, of course!



  • @GettinSadda said:

    returning null means "something went wrong"

    Throwing an exception means "something went wrong".  Returning null means "I need to document what returning null means."


  • @JoeCool said:

    @Rick said:

    Joe, personally, I write my code the way you do. But this is not a WTF, it is merely a style difference. 

    Anger management class maybe???

    hmmm... I'm pretty sure my first line said "not really a WTF, more of a pet peeve". Where did I seem angry?

     

    Why is this even a pet peeve?  It's the same damn thing, who cares?



  • public List<MyObjectType> getMyObject()

    {

      MyManagerObject myManager = getTheManager();

      if ( myManager == null ) return null;

      var result = new ArrayList<MyObjectType>();

      copyMyObjectsFromManager(myManager, result);

      return result;

     }

    And, why doesn't copyMyObjectsFromManger be more like  getMyObjectsFromManager(myManager)

    Then you get

     public List<MyObjectType> getMyObject()

    {

      MyManagerObject myManager = getTheManager();

      if ( myManager == null ) return null;

      return getMyObjectsFromManager(myManager);

    }

     



  • I agree with whatever dogbrags just posted.



  • 1) The "nonessential else block" provides a very simple way to put a breakpoint in the event of a null manager.

    2) Returning a null for a collection type from a property or method is *ALWAYS* a WTF. Either return an empty collection, or throw an exception.



  • @TheCPUWizard said:

    1) The "nonessential else block" provides a very simple way to put a breakpoint in the event of a null manager.

     

    Why did it return null? I don't know, I'm stopped at a breakpoint after the fact.... 

    @TheCPUWizard said:

    2) Returning a null for a collection type from a property or method is ALWAYS a WTF. Either return an empty collection, or throw an exception.

    This was just pseudo-code to illustrate the else... I agree with your second point.


  • ♿ (Parody)

    I often follow this pattern. But probably not always. It highlights to me that there are two very different things going on, depending on that condition. In particular, if you come back later, and you've set the variable to be returned to null, it can be easy to notice that it isn't always initialized. I can see how this could bother some people, and I don't claim to be particularly consistent about this sort of thing, but there you go.



  • @Cad Delworth said:

    But Jaime's right in one way: throwing an exception would be better than just returning NULL, providing the exception is properly handled, of course!

    The nice thing about exceptions is that if you forget to handle them, they will abort program execution there and then. Forgetting to check a return value might not become apparent until much later on, if you don't try to use it immediately or if it's a simple error/success code. Exceptions will also fly through multiple levels of function calls without any extra effort. Of course it's possible to handle exceptions incorrectly, but you need to knowingly put that empty catch block in.



  • @JoeCool said:

    So you don't think this is clear?

    public List<MyObjectType> getMyObject()

    {

      List<MyObjectType> result = null;

      MyManagerObject myManager = getTheManager();

      if ( myManager != null )

      {

        result = new ArrayList<MyObjectType>();

        copyMyObjectsFromManager(myManager, result);

       }

      return result;

    }

    Clear, yes, but I would never write that because it needlessly allocates ram and immediately releases it.  IMHO a good programmer never allocates ram they don't need.  Your code will cause more failures than the original, whether it causes an immediate OOM, contributes to memory fragmentation, or even data corruption if "result" experiences a bit flip error or similar.

     



  • @Pascal said:

    Clear, yes, but I would never write that because it needlessly allocates ram and immediately releases it.  IMHO a good programmer never allocates ram they don't need.  Your code will cause more failures than the original, whether it causes an immediate OOM, contributes to memory fragmentation, or even data corruption if "result" experiences a bit flip error or similar.

    Pascal - Sorry, but that argument is misleding at best and wrong at worst. In many languages, it is actually more efficient to create and relase LOTS of memory very quickly than to keep significantly smaller amounts around for longer, under the right circumstances.

     I work on some of the most demanding systems (Realtime Trading, Industrial Control), and the only way to make those types of comments is to *measure* specific conditions. Additionionally, resource utilization is *not* a "problem" provided there are sufficient resources. It provides no value to allow memory to "sit unused" or other similar situations.


  • ♿ (Parody)

    @Pascal said:

    Clear, yes, but I would never write that because it needlessly allocates ram and immediately releases it.  IMHO a good programmer never allocates ram they don't need.  Your code will cause more failures than the original, whether it causes an immediate OOM, contributes to memory fragmentation, or even data corruption if "result" experiences a bit flip error or similar.

    Eh...so you're really concerned about having an extra pointer to theManager? If that's going to cause problems, then you have bigger issues than this code.

    The correct answer is that you wouldn't write this code because it clearly lacks enough frames to securely mediate the manager.



  • @JoeCool said:

    public List<MyObjectType> getMyObject()

    Returning multiple MyObjects when the method name tells me I'm only getting one is TRWTF amirite?
    Its even worse where I work,

         MyObject.Foo = GetSomeFoos();   //  returns one foo
    

    also what's with Java lacking a naming convention to separate classes and interfaces?



  • @DanceMaster said:

    @JoeCool said:
    public List<MyObjectType> getMyObject()

    Returning multiple MyObjects when the method name tells me I'm only getting one is TRWTF amirite?
    Its even worse where I work,

         MyObject.Foo = GetSomeFoos();   //  returns one foo
    

    also what's with Java lacking a naming convention to separate classes and interfaces?

    No, TRWTF are those picking apart redacted pseudocode and being completely off topic



  • @Pascal said:

    Clear, yes, but I would never write that because it needlessly allocates ram and immediately releases it.  IMHO a good programmer never allocates ram they don't need.

    And a great programmer would realise that the compiler is not the retarded child that needs constant hand-holding like it used to in the early 1990's.

    Compilers are smarter than you are. Do us all a favour and just write clear code; Trust the compiler to make the best of it.



  • TRWTF no-one has noticed this thread number. 100x100 (hex, dec)!



  • public List<MyObjectType> getMyObject()
    {
        // Error checking is for wusses.
        MyManagerObject myManager = getTheManager();
        List<MyObjectType> result = new ArrayList<MyObjectType>();
        copyMyObjectsFromManager(myManager, result);
        return result;
    }
    


  • @Pascal said:

    Clear, yes, but I would never write that because it needlessly allocates ram and immediately releases it.  IMHO a good programmer never allocates ram they don't need.  Your code will cause more failures than the original, whether it causes an immediate OOM, contributes to memory fragmentation, or even data corruption if "result" experiences a bit flip error or similar.

    Interesting.  Let's break this down.

    IMHO a good programmer never allocates ram they don't need. Your code will cause more failures than the original, whether it causes an immediate OOM...

    Yup. That's why compilers generate "unused variable" warnings.  Initializing a variable is not declaring unused memory. In fact, in normal operation you are allocating the space for that variable anyway, and uninitialized variables have way more risk than allocating a local with the space of a pointer.

    ...contributes to memory fragmentation...

    The only memory allocated by the initialization to null is in in the stack, where it allocates enough space to hold a pointer.  There's nothing to fragment here.

    ...or even data corruption if "result" experiences a bit flip error or similar.

    If you're that worried about bit flips you're going to be using a system with ECC memory or at the very least parity checking.  Your processor will also have the capability to fire an interrupt in the event of such a parity or ECC error.

     



  • @MiffTheFox said:

    public List getMyObject()
    {
    // Error checking is for wusses.
    MyManagerObject myManager = getTheManager();
    List result = new ArrayList();
    copyMyObjectsFromManager(myManager, result);
    return result;
    }

     IF....(all of the folllowing)

     1) having getTheManager() returning null is an exceptional condition...
     2) there is not need for explicit logging/diagnosis at this level

    Then indeed error checking may be completely unnecessary,  achieving  a "minimal yet complete" solution (always a goal) is indeed to leave out the eror checking.



  • @JoeCool said:

    Why do some people insist on doing this pattern:

    public List<MyObjectType> getMyObject()

    {

      MyManagerObject myManager = getTheManager();

      if ( myManager != null )

      {

        List<MyObjectType> result = new ArrayList<MyObjectType>();

        copyMyObjectsFromManager(myManager, result);

        return result;

      }

      else

      {

        return null;

      }

    }

    why the else ???

    Because with that structure, you can easily comment out the "return result" and then add more "process this stuff a whole bunch more and THEN return the result" after the "else" clause without loosing any logic or having to shuffle anything around or making it any harder to read / understand.



  • Just follow uncle Bob's advice and never return null! Why for the love of god would the manager ever be null?


Log in to reply