Really Shallow Cloning



  • Found another gem (sanitized for your safety):

    public ItemType getItem() {
    if (cachedItem == null) {
    // hasn't been calculated yet, so build an empty one
    cachedItem = new ItemType();
    }
    ItemType returnValue = null;
    if ( ( cachedItem != null ) && ( cachedItem instanceof Cloneable ) ) {
    returnValue = cachedItem;
    }
    return returnValue;
    }



  • Does ItemType implement Cloneable? I'm trying to ascertain the depth of the dumbness there.



  • @Someone You Know said:

    Does ItemType implement Cloneable? I'm trying to ascertain the depth of the dumbness there.

     Yep. But, honestly, which would be dumber? Not implementing Cloneable and always getting back null OR implementing Cloneable and not actually using it?



  • So, why is it that things that are obvious to a fifth grader don't cross 90% of developer's minds?  If getItem doesn't get a freakin' item, then something is wrong.  Here, getItem gets an instance of ItemType, unless ItemType happens to not implement Cloneable, then it doesn't get anything (but it does go through the effort of creating it).  Add to that gratuitous abuse of "one and only one return" in a context where I can't see any reason to do it other than to conform to some draconian coding convention that probably has 30 pages on variable naming, but no mention of caching or logging.

    I'm guessing that maybe this is in the ItemType class and "getItem" is a generic method that is actually a weirly named "getInstance" or "getSingleton".  If so, then why not make it static?  It's kinda stupid to force someone to make an instance in order to get the singleton.  If this isn't defined in ItemType, then my naming rant from the previous paragraph applies.

    I had to let off a little steam here because I am currently working with a couple of contractors that very well might run into this page and copy and paste from the OP into my software.  They seem to be perfectly content writing brand-new software with a non-intuitive interface.  I just finished a conversation where we were talking about a method one of them wrote that fails if the object hasn't been recently saved to the database.  I asked why they didn't throw an exception if the object is dirty, or force a save before beginning.  They said "we documented the fact that if you call this method with a dirty object, it will behave badly".  I'll bet it took them longer to warn people that the method is dangerous than it would have taken to make it safe.  Somebody please run these people over.



  • @zelmak said:

    @Someone You Know said:

    Does ItemType implement Cloneable? I'm trying to ascertain the depth of the dumbness there.

     Yep. But, honestly, which would be dumber? Not implementing Cloneable and always getting back null OR implementing Cloneable and not actually using it?

     

    Yeah, now that I think about it, the check against Cloneable is useless either way. Unless ItemType didn't implement Cloneable, but cachedItem were actually of some subbclass of ItemType that did, but it's late and I don't really want to think about that.

    To be honest, though, Cloneable and Object.clone() are a pretty big WTF on their own. I'm guessing this code originally actually used Cloneable for something, but the original coder ran into all the difficulties associated with actually making Cloneable work correctly and gave up, forgetting to remove this.



  • @Someone You Know said:

    @zelmak said:

    @Someone You Know said:

    Does ItemType implement Cloneable? I'm trying to ascertain the depth of the dumbness there.

     Yep. But, honestly, which would be dumber? Not implementing Cloneable and always getting back null OR implementing Cloneable and not actually using it?

     

    Yeah, now that I think about it, the check against Cloneable is useless either way. Unless ItemType didn't implement Cloneable, but cachedItem were actually of some subbclass of ItemType that did, but it's late and I don't really want to think about that.

    To be honest, though, Cloneable and Object.clone() are a pretty big WTF on their own. I'm guessing this code originally actually used Cloneable for something, but the original coder ran into all the difficulties associated with actually making Cloneable work correctly and gave up, forgetting to remove this.

    The number of nested levels of abstraction here is TRWTF.

     

    Does OOP actually help? Or is it an infective meme designed by Satan to make everybody's head hurt?



  • @GreyWolf said:

    Does OOP actually help?
     

    No.



  • @dhromed said:

    @GreyWolf said:

    Does OOP actually help?
     

    No.

     

    OOP is fine, but the abuse-to-use ratio of inheritance is horrifying, in my experience. Grouping logical chunks of code in classes is usually no problem for most coders.

    There are definitely acceptable use cases for inheritance, but I've seen too many horrors.



  • I like the completely useless tautology of

    @zelmak said:

    if (cachedItem == null) {
    // hasn't been calculated yet, so build an empty one
    cachedItem = new ItemType();
    }
    ...
    if ( ( cachedItem != null )...

    Because you can never be sure that a constructor that hasn't thrown an exception will not give you a null value (I know, a lot of negatives there).



  • @Jaime said:

    I'm guessing that maybe this is in the ItemType class and "getItem" is a generic method that is actually a weirly named "getInstance" or "getSingleton".  If so, then why not make it static?  It's kinda stupid to force someone to make an instance in order to get the singleton.  If this isn't defined in ItemType, then my naming rant from the previous paragraph applies.

    No, this is in another object which caches an instance of ItemType because it is 'expensive' (in the programmer's mind) to create. (yet another WTF ... why not create it if it doesn't exist, instead of handing back an empty one to the caller?)



  • @toth said:

    I like the completely useless tautology of @zelmak said:
    if (cachedItem == null) { // hasn't been calculated yet, so build an empty one cachedItem = new ItemType(); } ... if ( ( cachedItem != null )...
    Because you can never be sure that a constructor that hasn't thrown an exception will not give you a null value (I know, a lot of negatives there).

    I don't know how sarcastic you're being, but since this is Java, a constructor will either return the correct object/type or it will throw an exception. I don't know of a way for a Java constructor to be bastardized in such a way as to return a null (not something I put a lot of brain cells into researching either ... why would you want to?)



  • @zelmak said:

    @toth said:

    [b]I like the completely useless tautology of[/b] @zelmak said:
    if (cachedItem == null) { // hasn't been calculated yet, so build an empty one cachedItem = new ItemType(); } ... if ( ( cachedItem != null )...
    Because you can never be sure that a constructor that hasn't thrown an exception will not give you a null value (I know, a lot of negatives there).

    I don't know how sarcastic you're being, but since this is Java, a constructor will either return the correct object/type or it will throw an exception. I don't know of a way for a Java constructor to be bastardized in such a way as to return a null (not something I put a lot of brain cells into researching either ... why would you want to?)

    Pretty safe bet that [b]toth[/b] was being sarcastic.


  • @Jaime said:

    So, why is it that things that are obvious to a fifth grader don't cross 90% of developer's minds?  If getItem doesn't get a freakin' item, then something is wrong.  Here, getItem gets an instance of ItemType, unless ItemType happens to not implement Cloneable, then it doesn't get anything (but it does go through the effort of creating it).  Add to that gratuitous abuse of "one and only one return" in a context where I can't see any reason to do it other than to conform to some draconian coding convention that probably has 30 pages on variable naming, but no mention of caching or logging.

    I'm guessing that maybe this is in the ItemType class and "getItem" is a generic method that is actually a weirly named "getInstance" or "getSingleton".  If so, then why not make it static?  It's kinda stupid to force someone to make an instance in order to get the singleton.  If this isn't defined in ItemType, then my naming rant from the previous paragraph applies.

    I had to let off a little steam here because I am currently working with a couple of contractors that very well might run into this page and copy and paste from the OP into my software.  They seem to be perfectly content writing brand-new software with a non-intuitive interface.  I just finished a conversation where we were talking about a method one of them wrote that fails if the object hasn't been recently saved to the database.  I asked why they didn't throw an exception if the object is dirty, or force a save before beginning.  They said "we documented the fact that if you call this method with a dirty object, it will behave badly".  I'll bet it took them longer to warn people that the method is dangerous than it would have taken to make it safe.  Somebody please run these people over.

    Do these contractors work for your company?  If so, perhaps you should counsel them about this behavior.  If they don't, I offer my condolences.


Log in to reply