Overcoding?



  • I don't know what to say:

    public class WeThreeStrings implements Comparable {
      private String first;
      private String second;
      private String third;
    

    .
    .
    .

    @Override
    public int compareTo (Object o) {
    return this.compareTo ( (WeThreeStrings) o );
    }

    public int compareTo (WeThreeStrings o) {
    int rtn = 0;
    WeThreeStrings that = o;

    String thisString = this.first;
    String thatString = that.first;
    
    if ( ( thisString != null ) && ( thatString == null ) ) {
    
      rtn = 0;
    
    } else if ( ( thisString != null ) && ( thatString == null ) ) {
    
      rtn = 3;
    
    } else if ( ( thisString == null ) && ( thatString != null ) ) {
    
      rtn = -3;
    
    } else {
    
      rtn = thisString.compareTo ( thatString );
    
    }
    
    if ( rtn != 0 ) {
    
      return rtn;
    
    }
    
    thisString = this.second;
    thatString = that.second;
    
    if ( ( thisString != null ) && ( thatString == null ) ) {
    
      rtn = 0;
    
    } else if ( ( thisString != null ) && ( thatString == null ) ) {
    
      rtn = 2;
    
    } else if ( ( thisString == null ) && ( thatString != null ) ) {
    
      rtn = -2;
    
    } else {
    
      rtn = thisString.compareTo ( thatString );
    
    }
    
    if ( rtn != 0 ) {
    
      return rtn;
    
    }
    
    thisString = this.third;
    thatString = that.third;
    
    if ( ( thisString != null ) && ( thatString == null ) ) {
    
      rtn = 0;
    
    } else if ( ( thisString != null ) && ( thatString == null ) ) {
    
      rtn = 1;
    
    } else if ( ( thisString == null ) && ( thatString != null ) ) {
    
      rtn = -1;
    
    } else {
    
      rtn = thisString.compareTo ( thatString );
    
    }
    
    return rtn;
    

    }

    }

    Granted, I did a quick scan to see if the magic values were used, but they weren't. In fact, the only references to either compareTo() method are internal to the class ... one in the equals() method. Just another YAGNI ... I think they were coding by the tonne.



  • WHeeeeeeeeeeeeeeeeeeeeeeeeeeThreeStrings!



  • WeThreeStrings of Java code are

    trying to form a tuple thus far.

    Verbosely pleading, eyes are bleeding,

    I hope there's an open bar.



  • @zelmak said:

    ...
    if ( ( thisString != null ) && ( thatString == null ) ) {

    rtn = 0;

    } else if ( ( thisString != null ) && ( thatString == null ) ) {

    rtn = 3;

    }
    ... 

    What the...



  • 🚽 Regular

    @zelmak said:

    Granted, I did a quick scan to see if the magic values were used, but they weren't. In fact, the only references to either compareTo() method are internal to the class ... one in the equals() method. Just another YAGNI ... I think they were coding by the tonne.
     

    Is it possible that instances are being injected into some other class or method that takes a Comparable typed instance? If so, this class has some warrant, although the name (and compareTo implementation) is among the suckiest I've seen.



  • ... I hope you do realize that 'we three strings' was me having fun with anonymization?



  • @Justice said:

    @zelmak said:

    ...
    if ( ( thisString != null ) && ( thatString == null ) ) {

    rtn = 0;

    } else if ( ( thisString != null ) && ( thatString == null ) ) {

    rtn = 3;

    }
    ... 

    What the...


    oops ... typo ...

    first is both null, second is first null, third is second null ... copy-pasta'd the mistake, too ...

     



  • @RHuckster said:

    @zelmak said:

    Granted, I did a quick scan to see if the magic values were used, but they weren't. In fact, the only references to either compareTo() method are internal to the class ... one in the equals() method. Just another YAGNI ... I think they were coding by the tonne.
     

    Is it possible that instances are being injected into some other class or method that takes a Comparable typed instance? If so, this class has some warrant, although the name (and compareTo implementation) is among the suckiest I've seen.

     

    ... in that case, the only guarantee to the coder using this object is that it meets the spec of the Comparable interface ... the magic values have no relevance.



  • @Xyro said:

    WeThreeStrings of Java code are
    trying to form a tuple thus far.
    Verbosely pleading, eyes are bleeding,
    I hope there's an open bar.
     

    Nice!



  • @zelmak said:

    oops ... typo ...

    first is both null, second is first null, third is second null ... copy-pasta'd the mistake, too ...

    So the only 2 parts that are truly wtf you changed or fat-fingered?



  • @zelmak said:

    ... I hope you do realize that 'we three strings' was me having fun with anonymization?

    Now let's come up with a way to duplicate this functionality in the R programming language using OrientDB somehow...

    Granted, I know almost nothing about either R or OrientDB except the names, but...I think my suggestion may actually be not only stupid, but phenomenally stupid.



  • @kilroo said:

    @zelmak said:

    ... I hope you do realize that 'we three strings' was me having fun with anonymization?

    Now let's come up with a way to duplicate this functionality in the R programming language using OrientDB somehow...

    Granted, I know almost nothing about either R or OrientDB except the names, but...I think my suggestion may actually be not only stupid, but phenomenally stupid.

    Cool, I've never seen a comment that implements IDisposable...

    1) Invoke new Comment

    2) m_comment.CreateComment

    3) Dispose() { NeverMindWhatIJustSaid(); }



  • @C-Octothorpe said:

    Cool, I've never seen a comment that implements IDisposable...
    ROFL!



  • @zelmak said:

    public class WeThreeStrings implements Comparable {
     

    public class ThreeWeeStrings extends WeThreeStrings {

       public static final int maxStrLen = 5;

    }



  • @Sutherlands said:

    So the only 2 parts that are truly wtf you changed or fat-fingered?

    They

    • introduced magic values which
    • met the spec but were never used
    • are potentially changing the behavior of String.compareTo() and adding explicit ording for where 'null' falls into a list of Strings
    • before all the null checking on field values, they don't even bother to check the passed-in object for nullness

    So, yeah, I guess you're right.

     

    If it were me, I would probably implement the guts more like:

    if (thisString != null) { 
      rtn = thisString.compareTo(thatString);
    } else if (thatString != null) {
      rtn = - thatString.compareTo(thisString);
    }
    

    if (rtn != 0) {

    return rtn;

    }

    .
    .
    .

    rtn is already 0 so no need to check for both being null; compareTo(null) Does The Right Thing(tm); extraneous/duplicative checks for nullness removed.



  • The biggest WTF I see here is that the parameter is named 'o', and they then create another variable referencing it named 'that', instead of simply naming the parameter 'that' in the first place.



    What the heck this class is for is also questionable.



  • @zelmak said:

    @Sutherlands said:

    So the only 2 parts that are truly wtf you changed or fat-fingered?

    They

    • introduced magic values which
    • met the spec but were never used
    • are potentially changing the behavior of String.compareTo() and adding explicit ording for where 'null' falls into a list of Strings
    • before all the null checking on field values, they don't even bother to check the passed-in object for nullness

    So, yeah, I guess you're right.

     

    If it were me, I would probably implement the guts more like:

    if (thisString != null) { 
      rtn = thisString.compareTo(thatString);
    } else if (thatString != null) {
      rtn = - thatString.compareTo(thisString);
    }
    

    if (rtn != 0) {

    return rtn;

    }

    .
    .
    .

    rtn is already 0 so no need to check for both being null; compareTo(null) Does The Right Thing(tm); extraneous/duplicative checks for nullness removed.

    The magic values are unimportant.  You can return 1, 2, 5, or Int.MaxValue if you want.  The only specification on the compareTo function is that it returns a positive value, negative value, or 0 (depending on the comparison result).  As for nulls, allowing them to be sorted is perfectly fine.  See http://stackoverflow.com/questions/481813/how-to-simplify-a-null-safe-compareto-implementation.  As for checking the passed-in object, see http://www.javapractices.com/topic/TopicAction.do?Id=10.  Seems acceptable to throw a NPE.

    Could it be less verbose? Certainly.  Is it worse than failure?  Not really.  As far as I can see, it's a working implementation of Comparable.

     

    edit: So that no pedants point this out, using MaxValue/MinValue would be bad because of how negative operator can cause an outofbounds exception



  • @Xyro said:

    WeThreeStrings of Java code are
    trying to form a tuple thus far.
    Verbosely pleading, eyes are bleeding,
    I hope there's an open bar.
     

    Oh.....oh! Code of wonder, code of fright!

     

    ...Yeah, I don't know if I can come up with anything to improve upon Xyro's initial verse there.



  • Wow! This is really useful, consider this:

    public class User extends WeThreeStrings {
    	public String getLogin() { return first; }
    	public String getFullname() { return second; }
    	public String getPassword() { return third; }
    
    public void setLogin(String login) { first = login; }
    public void setFullname(String fullname) { second = fullname; }
    public void setPassword(String password) { third = password; }
    

    }

    You get comparison operators for your objects for free!

    If you need more than three strings you can do something like this:

    public class WeThreeWeThreeStrings implements Comparable {
    	public WeThreeStrings first;
    	public WeThreeStrings second;
    	public WeThreeStrings third;
    
    public int compareTo (WeThreeStrings o) {
    	// You can figure this out yourself.
    }
    

    }



  • ...or you could create real classes to hold real fields with meaningful semantics and operations. And if an n-ary tuple of Comparables is truly needed, just fold them into an array and loop over each Comparable.

    It's a shame that Java doesn't have any simple tuple syntax, but I can see why it doesn't: what you really want is a full class that describes its data. structs, even semi-smart ones, often end up being refactored out into real classes after a while anyway. Nevertheless, they are useful in very small scale stuff, like when you really need multi-value returns. That's why I say it's a shame.

    But what's more of a shame is when developers try to make their own "generic" half-baked tuple-like classes. I myself have been guilty of this as well, but I eventually saw the light. If you need a temporary container that holds objects A, B, and C, then make a class ABC { A a; B b; C c; }. If it ought to be immutable then class ABC { final A a; final B b; final C c; public ABC(A a, B b, C c){this.a = a; this.b = b; this.c = c} }. If it needs to implement Comparable<ABC> or have a particular toString() or any other particular behavior, then the {A,B,C} triple probably needs to be a full-blown class rather than just placed in a nondescript container. Full and specific classes rather than generic tuploids are easier to tweak, debug, maintain, understand, etc. Yes, it's a bit more typing, but hey, this is Java, you should be used to more typing.



  • @zelmak said:

    If it were me, I would probably implement the guts more like:
    if (thisString != null) { 
      rtn = thisString.compareTo(thatString);
    } else if (thatString != null) {
      rtn = - thatString.compareTo(thisString);
    }
    

    if (rtn != 0) {

    return rtn;

    }

    .
    .
    .

    rtn is already 0 so no need to check for both being null; compareTo(null) Does The Right Thing(tm); extraneous/duplicative checks for nullness removed.

     

     

    The existing code is simple and easy to maintain. Your code is more complicated and changes the behavior to throw null pointer exceptions. In fact I'm not really sure why you are even bothering to check for null.

     



  • @zelmak said:

    compareTo(null) Does The Right Thing(tm); extraneous/duplicative checks for nullness removed.

    What is the Right Thing™ here? .compareTo(null) throws an NPE. WeThreeStrings does not (after correcting for fatfingering).



  • @Sutherlands said:

    Is it worse than failure?  Not really.  As far as I can see, it's a working implementation of Comparable.


    Comparable requires that is anti-symmetric (that means:  X < Y implies Y > X) and transitive (that means: X < Y and Y < Z imply X < Z). When this not satisfied, sorting algorithms will return random results and some of them may not terminate (infinite loop).

    WeThreeThings implementation claims [null,null,null] < ["A","B","C"] on the one hand ["A","B","C"] == [null,null,null] on the other hand.

    So it's a WTF alright.



  • @JvdL said:

    @Sutherlands said:
    Is it worse than failure?  Not really.  As far as I can see, it's a working implementation of Comparable.


    Comparable requires that is anti-symmetric (that means:  X < Y implies Y > X) and transitive (that means: X < Y and Y < Z imply X < Z). When this not satisfied, sorting algorithms will return random results and some of them may not terminate (infinite loop).

    WeThreeThings implementation claims [null,null,null] < ["A","B","C"] on the one hand ["A","B","C"] == [null,null,null] on the other hand.

    So it's a WTF alright.
    How do you figure?  If you get past the fat-finger that was mentioned earlier in the thread (first comparison they should both be null), you get:

        if ( ( thisString != null ) && ( thatString == null ) ) {

          rtn = 3;

        } else if ( ( thisString == null ) && ( thatString != null ) ) {

          rtn = -3;

        }



  • I shall capitulate.

    I was under the impression that String.compareTo() worked similarly to String.equals() and didn't throw an NPE just because its argument was null. I was wrong.

    With that in mind, WeThreeStrings.compareTo() would throw an NPE if it's argument was null in a similar manner.

    So, stripping all the null-checking away, the only WTF left is the magic values, which are silly and overdone, but not a real WTF.

     



  • @zelmak said:

    So, stripping all the null-checking away, the only WTF left is the magic values, which are silly and overdone, but not a real WTF.

    I don't think they're a WTF at all. The spec just calls for <0, 0, or >0. It doesn't matter how far the value is from zero, and using -1, -2, -3, 1, 2, 3 like they do here might be useful for debugging. Now, they could have thrown in a comment explaining what the "magic numbers" were, but lack of comments is a near-universal WTF in almost all code, so.



  • @blakeyrat said:

    ...but lack of comments is a near-universal WTF in almost all code, so.

    QFT



  • @blakeyrat said:

    Now, they could have thrown in a *comment* explaining what the "magic numbers" were, but lack of comments is a near-universal WTF in almost all code, so.

    Wait ... WHAT?!? You can add comments to your code and explain what the code is doing?!! What is this black magic of which you speak? Surely you are a witch and need to be burned!



  • @zelmak said:

    Wait ... WHAT?!? You can add comments to your code and explain what the code is doing?!! What is this black magic of which you speak? Surely you are a witch and need to be burned!
    If common usage is any guide, it's where the incompetent post their voluminous manifestos before being fired.



  • @zelmak said:

    Wait ... WHAT?!? You can add comments to your code and explain what the code is doing?!! What is this black magic of which you speak? Surely you are a witch and need to be burned!

    Of course you can! Like this:


    a = a + 2 ' Add 1 to a.


  • 🚽 Regular

    // returns 5 
    public int double_think()
    {
        return 2 + 2;
    }


  • @Sutherlands said:

    @JvdL said:
    @Sutherlands said:
    Is it worse than failure?  Not really.
    It's a WTF alright.
    [Not] if you get past the fat-finger that was mentioned earlier in the thread


    Sorry, missed that. Retracted.

    @(Various) said:
    All that null checking is unnecessary. It's OK to throw a NullPointerException


    It's OK to throw a NPE in x.CompareTo(y) when y is null; or a ClassCastException when y is of unexpected type. This is in fact what happens. But when y is not null and of the right type; it should not throw just because one of x's or y's attributes is null.

    A useful application of this class (*) could be to sort people names; with first = last name, second = first name and third = middle initial. It is not uncommon that one of these null because it is not known or does not exist.

    The method should not throw the whole sort to kingdom come only because one name has a missing middle initial. So the null checking is necessary.

    (*) Useful if the implementation would have been cleaner and less repetitive.



  • @RHuckster said:

    // returns 5
    public int double_think()
    {
    return 2 + 2;
    }

    QFT

    +1 for the 1984 reference.


Log in to reply