Spot the WTF



  • Since we are on it, spot the WTF here (besides ignoring java.awt.geom.Point2D.Double):

    final class Point {
    public final double x;
    public final double y;

    public Point(double x, double y) {
    this.x = x;
    this.y = y;
    }

    public boolean equals(Point other) {
    return this.x == other.x && this.y == other.y;
    }
    @Override public boolean equals(Object object) {
    return (object instanceof Point) && equals((Point)object);
    }

    private static int hashCode(long value) {
    return (int)(value ^ (value >> 32));
    }
    private static int hashCode(double value) {
    return hashCode(Double.doubleToLongBits(value));
    }
    @Override public int hashCode() {
    return hashCode(this.x) ^ hashCode(this.y);
    }
    }


  • @maratcolumn1 said:

        private static int hashCode(long value) {
    return (int)(value ^ (value >> 32));
    }
    private static int hashCode(double value) {
    return hashCode(Double.doubleToLongBits(value));
    }

    Somebody really, really likes to overload (or really doesn't know about local variables).

    Also, the hashCode(double) implementation doesn't quite match the one documented for Double, even though the author obviously went through some trouble to get his implementation as close as possible.



  • A toughie.  Seems like all points where x == y will end up with the same hashcode.  Surely not what was intended.



  • @UpNDown said:

    A toughie.  Seems like all points where x == y will end up with the same hashcode.  Surely not what was intended.

    That's a typical solution presented at many pages. If you're getting data from "Outside World" (TM), it will be probably better than other implementations anyway :)
    Humans XOR, nature doesn't. HashCodes are not really unique typically...



  • The override on equals is a little screwy too.

     

    Calling 'equals' on two instances of this class will always call the 'public boolean equals(Point other)' method 



  • - not checking for null in #equals(Point)

    - Duplicating features haphazardly (the whole class could be replaced with Point2D.Double, as mentioned; hashing double values is already provided by "Double.valueOf(x).hashCode ()" in Java 1.5+ -- and object creation is not "too expensive" until you've proven it is with a profiler, especially since the java.lang.Number subclasses implement a variety of the Flyweight pattern)

    The "public final double" values are specifically NOT a WTF as such...  Since this is a package-scope class (no "public" before class) that seems to be a simple data container, public final is relatively okay.  Public non-final I'd have issues with.

    Edit: no, the equals override looks okay.  Java always calls the most specific override resolvable from the static type information.  In #equals(Object), the parameter is cast to Point so the next call is going to be #equals(Point). 



  • Since nobody pointed it out yet, how about: double == double ?



  • @snoofle said:

    Since nobody pointed it out yet, how about: double == double ?

     double == double; toil && trouble



  •  Point p1 = null;
     Point p2 = new Point(3.0, 2.0);
     try {
      if (!(p2.equals(p1))) {
       System.out.println("Not equal");
      }
     } catch (NullPointerException e) {
      System.out.println("That's impossible!");
     }

    In the contract for equals(), for any non-null reference x, x.equals(null) should return false.

    OK, here's another thing every Java programmer should know:  How to write an equals() method.  It is on pages 205 to 206 of "Core Java 2, Volume 1 -- Fundamentals", isbn 0-13-089468-0, published by Sun and Prentice Hall.  For later editions, look up "equals" in the index.

    I would say this equals() isn't a WTF; it's just wrong. 

    hashCode() is kind of a WTF.  If you're going to go through all the trouble to look up the implementation of Double.hashCode(), why not just write "new Double(value).hashCode()"?  (Or its auto-boxed Java 5 equivalent.)

     

     



  • @webzter said:

    @snoofle said:

    Since nobody pointed it out yet, how about: double == double ?

     double == double; toil && trouble

    fire.burn();
    cauldron.bubble();



  • @newfweiler said:

     Point p1 = null;
     Point p2 = new Point(3.0, 2.0);
     try {
      if (!(p2.equals(p1))) {
       System.out.println("Not equal");
      }
     } catch (NullPointerException e) {
      System.out.println("That's impossible!");
     }

    In the contract for equals(), for any non-null reference x, x.equals(null) should return false.

    OK, here's another thing every Java programmer should know:  How to write an equals() method.  It is on pages 205 to 206 of "Core Java 2, Volume 1 -- Fundamentals", isbn 0-13-089468-0, published by Sun and Prentice Hall.  For later editions, look up "equals" in the index.

    I would say this equals() isn't a WTF; it's just wrong.

    You're not calling the method you think you're calling.  Look closer.

    The contract is for overriders on #equals(Object), and java doesn't support covariant argument types (because they can't be statically verified in all cases).  Your code calls #equals(Point), which is undocumented in the OP.  Yes, it's a fiddly detail, and yes, I'd yell at anyone who wrote #equals(T) method that didn't obey the same contract as #equals(Object), but it's not wrong.

    Incidentally, "instanceof" returns false for any left-hand expression that evaluates to null.  JLS 3rd Edition, section 15.20.2.

     



  • Somebody really, really likes to overload (or really doesn't know about local variables).

    A matter of style.

    Also, the hashCode(double) implementation doesn't quite match the one documented for Double, even though the author obviously went through some trouble to get his implementation as close as possible.

    It does. Higher bits are discarded anyways.

    A toughie. Seems like all points where x == y will end up with the same hashcode. Surely not what was intended.

    This one is the closest, almost the answer.

    I want to use this equals, I want to hash points in space, not bits. I don't care about NANs, I don't have them. What should I fix here? Or, first, where will it fail?



  • Probably not what you're looking for, but another strike against this WTF:

    Declare two Points p1 and p2, where p1.x = p2.y and p1.y = p2.x.  They hash to the same value, since the hash of the Point is just the XOR of the hash of the two dimensions.

    That isn't illegal, as noted by Object.hashCode()'s Javadoc: o1.equals(o2) implies o1.hashCode() == o2.hashCode(), but not the other way around.  But it's stupidly suboptimal if you're planning on putting, say, a grid of points into a hashtable.



  • Probably not what you're looking for, but another strike against this WTF

    This is an interesting topic, but not WTF. If we cache the hash code in a local field, and it would make a lot of sense in the example, then we could use more elaborate hash function. But we can save one equals(Object) call per HashMap.get(...) at most, I'd not call it "stupidly suboptimal". It is more stupidly suboptimal in 3D case though.

    Generally I agree, at least incrementing y hashcode by 1 is a good practice. rol 16 would be better, but sadly no high-level languages I know support rolls (cyclic shifts).



  • Everyone is too lazy, so I'll give a pair of hints. Adding zero at any stage fixed the problem for me, like this, for instance:

        public Point(double x, double y) {
            this.x = x + 0;
            this.y = y + 0;
        }
    

    But, unless you live in Great Britain, or, in Indonesia, you may never notice the problem at all.



  • Mmm. It took me a while to get that one, even with the hint! Still, more of a bug than a WTF I reckon. And yes, I do live in Great Britain.



  • Okay, okay.  So are you saying that since x and y are declared as final the class doesn't work?  Please enlighten us with your diagnosis.



  • @maratcolumn1 said:

    Everyone is too lazy, so I'll give a pair of hints. Adding zero at any stage fixed the problem for me, like this, for instance:

        public Point(double x, double y) {
    this.x = x + 0;
    this.y = y + 0;
    }

    But, unless you live in Great Britain, or, in Indonesia, you may never notice the problem at all.

    Is this something to do with negative zero? (which, to my math student mind, is a WTF in itself - I know it has its uses, but it still makes me cringe)  As in, positive and negative zero compare equal, but have different hash codes?

    Not sure about the GB/Indonesia reference, unless you mean the prime meridian/equator. 



  • If anyone wants evidence that code readings are useless for finding bugs, this thread is it.

     



  • Is this something to do with negative zero?

    Exactly.

    (which, to my math student mind, is a WTF in itself - I know it has its uses, but it still makes me cringe)

    Some people think it is important to save the sign during overflow/underflow - see Signed Zero in "What Every Computer Scientist Should Know About Floating-Point Arithmetic". For me, one infinity would be enough.

    ... unless you mean the prime meridian/equator.

    Yes. :)

    If anyone wants evidence that code readings are useless for finding bugs, this thread is it.

    Then WTF is not here, but in the IEEE.

    Generally, Microsoft should probably declare floating-point operations unsafe as well :). Even 39.995 * 100 < 3999.5 can be too much for some.



  • So you're saying that if one point is created using -0 as either x or y, when compared to a second point (0,0) it will be equal, but the hash code will be different?  Okay.  How are you getting the -0 into your code in the first place?  Surely not through user input?  If so, isn't that the WTF?



  • So you're saying that if one point is created using -0 as either x or y, when compared to a second point (0,0) it will be equal, but the hash code will be different? Okay. How are you getting the -0 into your code in the first place? Surely not through user input? If so, isn't that the WTF?

    It is pseudo-randomly produced by some calculations instead of zero. You may never know it because it works exactly like zero for all other uses. Finding point of intersection between two lines is an example of such calculation.

    P.S. Probably non-strict (long double) math is involved, not sure.



  • @maratcolumn1 said:

    Finding point of intersection between two lines is an example of such calculation.

    Finding points of intersection is a problem of its own.  You can draw a rectangle and a line such that the line clearly (visually) intersects the corner of the rectangle, but the computer calculates that no points on the line are inside the rectangle.  (Calculus would get it correct; calculation would get it wrong.)

     

     



  • Okay, so let me get this right.  You're producing negative zeroes in some fancy-shmancy calculation, and then you're claiming it's a WTF that this innocent class is having problems with them.  I'd say clean up the negative zeroes at the source.  'Cause, if you allow them into this class, they'll bleed through the whole application.



  • -0 is a legitimate value for floating point numbers and you're more likely to introduce errors by "correcting" -0 to +0.  The "WTF" (I'd say it's more of a gotcha, one of those subtle issues with floating point math in general) is that the contract for Object#equals(Object) and Object#hashCode() are defined such that if a and b are (non-null) Object references and a.equals(b), then a.hashCode () == b.hashCode ().  This is important for things like, say, HashMap and HashSet.  However, the code as written causes (-0, 0), (0, -0), (0, 0), and (-0, -0) to have more than one distinct hashCode value, even though #equals(Object) for any pair of those returns true.

    It's verbose, but writing #equals(Point) as

    public boolean equals(Point other) {
      return Double.valueOf(x).equals(Double.valueOf(other.x))
        && Double.valueOf(y).equals(Double.valueOf(other.y));
    }

    corrects the issue by making those four points no longer #equals(Object) of each other.  I would've left both #equals(Object) and #hashCode() off of a floating-point Point class, leaving the default "equals implies identity" implementation provided by Object.  Equality tests and floating-point math don't mix well except under very controlled circumstances.



  • newfweiler:
    Finding points of intersection is a problem of its own. You can draw a rectangle and a line such that the line clearly (visually) intersects the corner of the rectangle, but the computer calculates that no points on the line are inside the rectangle.

    Can you please post an example? Either you are running out of precision, or you are doing it wrong (like using divisions). BTW Line2D.relativeCCW(...) rules.

    UpNDown:
    You're producing negative zeroes in some fancy-shmancy calculation, and then you're claiming it's a WTF that this innocent class is having problems with them.

    As was correctly pointed many times, this class violates contracts of methods it overrides.

    Angstrom:
    It's verbose, but writing #equals(Point) as

    public boolean equals(Point other) {
      return Double.valueOf(x).equals(Double.valueOf(other.x))
        && Double.valueOf(y).equals(Double.valueOf(other.y));
    }

    corrects the issue [...]

    At least you could think of:

    final class Point {
    public final Double x;
    public final Double y;
    ...

    And, object creation IS too expensive. I HAVE proven it is with a profiler too many times. Everyone telling you that 'object creation is not "too expensive"' is either trying to sell you something (like Sun) or making his life easier (like your professor).

    Finally, do you think I put points into hashmap for the fun of it? You solved the problem by solving completely different problem.



  • As keys?  I'm kind of curious what you're actually doing, then.  Sounds interesting.

    As for creation, there's "too expensive" and there's "too expensive".  It looks like you're doing the kind of CPU-devouring numeric tasks where stopping every so often for memory management really is too expensive, and that you've demonstrated that that was the bottleneck.  Great.  I'm not arguing with you, you know your code and application better than I do.  But in the vast majority of cases that I've actually worked on, the time spent either waiting for the user or on I/O dwarfed the time spent on the GC or on memory allocation, so fretting about each and every "new" was a waste of my time.

    I think I like your approach better; it does the object creation once (at Point#Point(double,double) time) and it's pretty likely that the JIT compiler will inline Double#equals(Object) since it's being called through a final reference to a final class.  It's too bad Double#valueOf(double) doesn't cache instances, but I can't think of a reasonable way to implement that that won't be slower than creating a new Double as needed.



  • I'm kind of curious what you're actually doing, then.

    Building graph from series of line segments - "roads". Every intersection becomes node. Completely real-life task.

    But in the vast majority of cases that I've actually worked on, the time spent either waiting for the user or on I/O dwarfed the time spent on the GC or on memory allocation

    I absolutely agree, factually you are right. The problem is, in the real life code is usually being reused whenever possible. So you cannot really predict usage pattern for primitives like this beforehand.

    Also, you are probably thinking about Java on PC, where JIT compiler is smart and memory is usually not deallocated at all (once after process ends). But there're many other Java devices out there. I knew some cellphones where memory allocation gets terribly slow once many objects are created. Just to be on the safe side, if you can write it right, why not do it from the very beginning?

    Actually, I did some measurements. I'll not comment, but some things were completely surprising for myself. At least I agree profiling is important :)

    iterations
    (more is better)
    constructor equals(Point other)
    implementation, x part
    hashCode()
    implementation
    hashCode(double value)
    implementation
     97 this.x = x; ... Double.valueOf(this.x).equals(other.x) hashCode(this.x) ^ hashCode(this.y) hashCode(Double.doubleToLongBits(this.x))
    107 this.x = new Double(x); ... this.x.equals(other.x) this.x.hashCode() ^ this.y.hashCode()
    117 this.x = x; ... Double.doubleToLongBits(this.x) == ... hashCode(this.x) ^ hashCode(this.y) hashCode(Double.doubleToLongBits(this.x))
    382 this.x = x; ... Double.doubleToRawLongBits(this.x) == ... hashCode(this.x) ^ hashCode(this.y) hashCode(Double.doubleToRawLongBits(this.x))
    401 Point2D.Double
    536 ...;
    this.hashCode = /* same as above */;
    Double.doubleToRawLongBits(this.x) == ... this.hashCode hashCode(Double.doubleToRawLongBits(this.x))
    547 this.x = x + 0.0; ... Double.doubleToRawLongBits(this.x) == ... hashCode(this.x) ^ (hashCode(this.y) rol 16) hashCode(Double.doubleToRawLongBits(this.x))
    573 this.x = x; ... Double.doubleToRawLongBits(this.x) == ... hashCode(this.x) ^ (hashCode(this.y) rol 16) hashCode(Double.doubleToRawLongBits(this.x))

    How figures were obtained: 0x10000 objects with random coordinates from 16x16 grid were created and inserted into LinkedHashSet of default size unless they were already there (i.e. mostly just checking). Then set was clear()-ed and process repeated, number of repetitions in 10 seconds was counted.

    P.S. Nazi forum does not allow me to define own stylesheets!



  • @maratcolumn1 said:

    newfweiler:
    Finding points of intersection is a problem of its own. You can draw a rectangle and a line such that the line clearly (visually) intersects the corner of the rectangle, but the computer calculates that no points on the line are inside the rectangle.

    Can you please post an example? Either you are running out of precision, or you are doing it wrong (like using divisions). BTW Line2D.relativeCCW(...) rules.

    Running out of precision is exactly the problem.  A real line is continuous but nothing in a digital computer is continuous.  There are gaps between the points in a line.  It is possible for the corner of a rectangle to poke through a gap.

    I don't know how Line2D.relativeCCW() works.  If it derives the equations and then manipulates them with calculus, then it should always work.  But most programs use discrete mathematics, not continuous mathematics.

     



  • Running out of precision is exactly the problem.

    By "running out of precision" I mean working with lines defined by points (1.0, 1.0) and (1.000000001, 2.0) for instance.

    I don't know how Line2D.relativeCCW() works. If it derives the equations and then manipulates them with calculus, then it should always work.

    Well, you may call it deriving the equations. Important is that Line2D.relativeCCW() always returns what's promised. I still wonder how you may be doing it.



  • @maratcolumn1 said:

    Running out of precision is exactly the problem.

    By "running out of precision" I mean working with lines defined by points (1.0, 1.0) and (1.000000001, 2.0) for instance.

    I don't know how Line2D.relativeCCW() works. If it derives the equations and then manipulates them with calculus, then it should always work.

    Well, you may call it deriving the equations. Important is that Line2D.relativeCCW() always returns what's promised. I still wonder how you may be doing it.

    I based this on a remembered Scientific American article of a few years back but I can't find the reference now.   Thinking about it, I think they were referring to using a form of integer arithmetic for speed.  (Given a finite set of points on a plane, and a rectangle, there exists a line such that although the line should intersect the rectangle, none of the points on the line are inside the rectangle.)

    I tried but failed to construct an example using double precision in Java.  I still think an example must be possible.  (Maybe if I try tipping the rectangle so that the corner point is "between" two "adjacent" double-precision numbers.  I'll try that tomorrow (if I have time).)

    A practical application would be designing desks and machines to fit in a constrained space, like a submarine.  Imagine if the corner of the desk is JUST half a millimeter too big.  Well, I suppose you could always fix it with a hammer.

     

     


Log in to reply