Representative line: should not happen - report



  • Yet another representative line found while trying to figure out why a value that should never be null, is coming up as null.

            public IItem GetItem(int p_itemId)
            {
                IItem item = null;
                if (!items.TryGetValue(p_itemId, out item))
                {
                    // should not happen - report.
                }
                return item;
            }
    

    Ever heard of these newfangled things called "exceptions", or "logs"? Because we clearly haven't.



  • There is also something called a return value which might be of usage here :}



  • @mt@ilovefactory.com said:

    There is also something called a return value which might be of usage here :}

    Personally I'd much rather see an exception for a case commented 'should not happen'.


  • Considered Harmful

    @Shoreline said:

    Personally I'd much rather see an exception for a case commented 'should not happen'.

    Then why use TryGetValue at all?



  • To have a more precise exception, I guess.



  • @TheLazyHase said:

    To have a more precise no exception whatsoever, I guess.



  • @mt@ilovefactory.com said:

    There is also something called a return value which might be of usage here :}
    Returning null from that method is the cause of the problem of something that should never have been null being null, not the solution.

     



  • @DaveK said:

    @mt@ilovefactory.com said:

    There is also something called a return value which might be of usage here :}
    Returning null from that method is the cause of the problem of something that should never have been null being null, not the solution.

     

     

    You can't be sure about that. items.TryGetValue should not give null in this case, but there may be other cases where TryGetValue could return null without it beeing an error, so it might be the best solution to test the return value and throw an exception in the cases where you know it should find what it is searching for.

    But I think the real wtf is that the method returns a boolean and set the value of item, instead of just returning item and then return null(Or throw an exception) if it is not found.

     

     



  • @mt@ilovefactory.com said:

    @DaveK said:

    @mt@ilovefactory.com said:

    There is also something called a return value which might be of usage here :}
    Returning null from that method is the cause of the problem of something that should never have been null being null, not the solution.

     

    You can't be sure about that.

     

    Philosophically speaking I can't be sure about anything, even that you exist or are or are not trolling, but I think it's a bloody reasonable inference strongly implied by the OP:

    @configurator said:

    Yet another representative line found while trying to figure out why a value that should never be null, is coming up as null.



  • @DaveK said:

    @mt@ilovefactory.com said:

    @DaveK said:

    @mt@ilovefactory.com said:

    There is also something called a return value which might be of usage here :}
    Returning null from that method is the cause of the problem of something that should never have been null being null, not the solution.

     

    You can't be sure about that.

     

    Philosophically speaking I can't be sure about anything, even that you exist or are or are not trolling, but I think it's a bloody reasonable inference strongly implied by the OP:

    @configurator said:

    Yet another representative line found while trying to figure out why a value that should never be null, is coming up as null.

     

     I think you misunderstod.The question is: Should "items.TryGetValue" return null instead of throwing an exception if the result was not found. And my argument is that returning null might be the correct thing to do for that method.

    You can't in general know if TryGetValue not finding anything(And thus returning null) is a general error. It might be legit for the method not to find what you are searching for when called from other places in the code.

    So my fix for this could, would be to have TryGetValue just return the result, instead of using that stupid out thing. And then have the code saying

    // should not happen - report.

      throw the exception.

     

     

     



  • @mt@ilovefactory.com said:

    So my fix for this could, would be to have TryGetValue just return the result,call GetValue directly and delete the entire pointless method instead of using that stupid out thing.



  • @mt@ilovefactory.com said:

    I think you misunderstod.The question is: Should "items.TryGetValue" return null instead of throwing an exception if the result was not found. And my argument is that returning null might be the correct thing to do for that method.

    You can't in general know if TryGetValue not finding anything(And thus returning null) is a general error. It might be legit for the method not to find what you are searching for when called from other places in the code.

    So my fix for this could, would be to have TryGetValue just return the result, instead of using that stupid out thing. And then have the code saying

    // should not happen - report.

      throw the exception.

    In .Net, the TryXxx method is a well-known formula, usually as a non-throwing variant of a corresponding Xxx method, and occasionally the only variant (see e.g. ConcurrentDictionary.TryAdd). TryXxx methods always return bool to indicate success or failure, and use an out-parameter to return a value result. I, for various reasons, think that's often a much better design than returning null (for one thing, returning null tends to violate the Principle of Least Surprise), but regardless of that it is the established formula.



  • @joe.edwards said:

    @Shoreline said:
    Personally I'd much rather see an exception for a case commented 'should not happen'.

    Then why use TryGetValue at all?

     

    @TheLazyHase said:

    To have a more precise exception, I guess.
    This: Ordinary dictionaries throw a KeyNotFoundException without mentioning the key in it (probably for security reasons). When it's not security-critical to hide the data, you may want to use TryGetValue() and throw the exception yourself with more information.

     



  • //shenanigans

  • Discourse touched me in a no-no place

    @MiffTheFox said:

    //shenanigans
    I prefer this:
    shenanigans()
    YMMV.



  • @dkf said:

    @MiffTheFox said:

    //shenanigans
    I prefer this:
    shenanigans()
    YMMV.
    But it doesn't reflect the sheer uselessness of the Shenanigans handler in all its glory.

Log in to reply