• Little Bobby Tables (unregistered)

    The frist thing I would do is change the contents of the exception trap to a throws and see what happens ...

  • WhatEver (unregistered)

    If you have a solution to it then I'd love to see how you transitively compare "Foo", "12" and "13". No hurry, I'll wait.

    And as for "p1.CompareTo(p2)". That's a lexicographic comparison, not a numerical one.

  • Lothar (unregistered)

    First of all. If a.compareTo(b) == 0 it doesn't mean that a.equals(b). And: If the submitter is changing the current implementation to p1.compareTo(p2), "11" will be seen as more important than "2".

  • Wizard (unregistered)

    I love it when the reason give for not fixing bugs is "because its in a critical part of the code". Surely this is paramount to negligence and I would like it if more customers complained about such negligence. Might start to get rid of the idiot's in the IT industry then and start having some professionalism around here.

  • Kleyguerth (github) in reply to Wizard

    You could argue that fixing it might introduce other bugs. A known, not critical bug is better than an unknown, possibly critical one

  • gnasher729 (unregistered)

    MacOS / iOS have a string comparison method with a multitude of options that will among other things sort numbers correctly when they occur inside.a string. So Foo < Priority 1 < priority 9 < priority 20 < 12 < 13 < 101.

  • Little Bobby Tables (unregistered) in reply to gnasher729

    ... except that you really want priority 9 < 12 < 13 < priority 20 < 101.

  • Code Refactorer (unregistered)

    If Communication.getPriority() is a method that returns a String and not a number, then you should start analyzing what is wrong instead of programming a garbage-in-garbage-out algorithm. Even comparing Strings, just to be on the safe side without exception handling, is wrong. If you compare priorites like "A", "B", "C" as strings, it would be sorted correctly, but priorities like "01", "10" or "A9", "A19", or "second", "fourth" not. Conversion to an integer should not be a job done outside the Communication class, but a method of the Communication object. So the first thing a programmer should do is to realize that it is impossible to program a comparison without further analysis of the data. He should not just implement something that is half-brewn, but something that works in all cases. If you print out a list of all possible values from the database then for example you find out that there are only the values "High", "Medium" and "Low" and constraints on the database enforcing only these 3 possible priorities.

    But what if you are not allowed to change the Communication class to add a getPriorityAsInt() mehod and to make a sanity check in the constructor of the object for fail-fast behaviour? Well, then you should use composition (or inheritance): make your own clean communication object: define a new class that holds the original dirty communication object and implement fail-fast checks (in the constructor and whenever somebody wants to change the priority-String). Then only use your own communication objects in your whole program.

    In all cases: The real WTF is believing "if it is not parseable, it is equal" instead letting the parse-error bubble up.

  • Pjrz (unregistered) in reply to WhatEver

    Well "Foo" clearly has top priority. Followed by "Bar". "13" comes after "12", unless its a Friday. In which case "13" is unlucky and has lower priority. Unless it is raining.

  • Brian (unregistered) in reply to Code Refactorer

    What this says to me is that Priority really ought to be an enum (at least if this were C++; I'm not so familiar with Java). Of course enums aren't immune to abuse, but correct usage of enums is easier to enforce than arbitrary strings.

  • I'm not a robot (unregistered) in reply to WhatEver
    If you have a solution to it then I'd love to see how you transitively compare "Foo", "12" and "13". No hurry, I'll wait.
    Comparisons *have* to be transitive. Just because you can't think of how to do it properly (and it really isn't remotely difficult) doesn't mean it's OK to define a broken version.
  • (nodebb) in reply to Code Refactorer

    The everything-is-a-string mentality is a sign that H1Bs are involved. The fear of never ever touching anything even if it's clearly broken is another. What astonishes me, from experience, is how never-ever-touch-anything and rework-rework-rework-forever can co-exist like they do.

  • I am a robot (unregistered) in reply to Pjrz

    Sounds like my colleagues requirements.

  • (nodebb)

    But what about the other big bug in this code? p1 and p2 are of the Integer class, not primitives of type int. So will p1 > p2 not compare the Objects (references) instead of the values?

    Or is my 2000-ish knowledge of Java hopelessly outdated?

  • Anon The Mouse (unregistered) in reply to nerd4sale

    The answer is: It depends. Due to some caching magic, if the Integers p1 and p2 are between -127 and +127, the primitive comparators (< and >) will work properly. Otherwise, they should use p1.compareTo(p2).

  • Rob (unregistered)

    There's a potential bug in there too. Although both p1 and p2 are created using Integer.parseInt (which returns an int), they are assigned to Integer variables. This means they will be autoboxed. Autoboxing uses cached values only for values between -128 and 127 (inclusive). That means that for any two equal values within that range, the autoboxing will use the same Integer object and p1 == p2 will be true. If both are 128, a non-cached, new Integer object will be returned. As a result, p1 != p2.

    Fortunately, the trailing "return 0" will catch this case, but it clearly shows that the author of this piece of code has no idea what he's doing.

    A better solution:

    • If parsing fails, use lexicographical comparison (o1.getPriority().compareTo(o2.getPriority()).
    • If parsing succeeds, use ints and not Integer.
    • If using Java 8 or up, use Integer.compare(p1, p2).
  • mihi (unregistered)

    I would not use lexicographic parsing in case numeric one fails. That way, 9 < 10 < 10a < 8a < 9.

    To get it number-safe, failsafe and transitive, the failure code has to be a lot more robust. One option I can think of (please point out if there are any mistakes) would be:

    • If equal, return 0
    • Use a regex like ^([^0-9])([0-9])(.*)$ to split both values into three parts, prefix (not containing digits), digits and rest (which may contain digits but not at the start)
    • lexicographically compare the prefix
    • if same, and one's digits are empty (it cannot happen for both or the strings would be equal), the empty digits are smaller
    • if same, parse the numbers (beware of overflow in case larger than Integer.MAX_VALUE) and compare numerically
    • if same, compare the suffix lexicographically

    (If you repeat the same algorithm for the rest, you probably have an equivalent of the iOS sorting function)

  • (nodebb) in reply to gnasher729

    The OS has a sting comparison method? This seems wrong.

    Addendum 2019-01-07 16:22: "sting" s.b. "string"

  • Benjamin (unregistered) in reply to mozzis

    It's not that hard to write.

    Bilbo's sword > The wrestler > The thing bees do to hurt you > That godawful singer

  • PenguinF (unregistered) in reply to Kleyguerth

    I have a solution which leaves the non-critical bug intact, doesn't introduce new (possibly critical) bugs -and- improves performance! Your users will thank you: public int compare(Communication o1, Communication o2) { return Random.next(2) - 1; }

  • Chris (unregistered) in reply to PenguinF

    I think you mean Random.nextInt(3) - 1. Random.next(2) returns a 2-bit number (0, 1, 2 or 3). Although, returning a 2 (3 - 1) is valid, it just adds more weight to returning a positive number.

  • Simon (unregistered) in reply to nerd4sale

    Sort of. If this were 2000-era Java, it would be a compile error. It works (for a loose definition of "works") because of autoboxing / autounboxing that appeared in Java 5 - automatic conversion between primitive types and their object counterparts. So just as the primitive types returned by parseInt were automatically converted to Integer objects, the Integer objects are being automatically converted to primitive ints in the comparisons... so it does actually work as intended.

    However, it's not really good practice to write code that's constantly converting between the two forms, since it does incur some small overhead (particularly when converting to objects). Really, p1 and p2 should have been declared primitive, since there's no possibility of them being null.

  • gnasher729 (unregistered) in reply to mozzis

    String comparison in the OS means people don't roll their own, incorrect one. Or use one posted to GitHub by some rank amateurs. File names are sorted intelligently. Like version 10 to 19 of a document not between version 1 and version 2. The whole range of Unicode handled correctly. PLUS localised versions. Would you know how to sort in Swedish? You don't have to.

  • Code Refactorer (unregistered) in reply to mihi

    The main point in my post before is that you should analyze the data before coding. You spend a lot of time developing a string sorting algorithm with prefixes, numbers and suffixes, but it is all for nothing if it turns out that the strings contain only the three possible values "Low", "Medium" and "High".

    But even if they contain numbers, how do you know if "1" is higher priority than "2"? For example "DEFCON 1" has higher priority than "DEFCON 5".

    You just make assumptions which are not better than the assumption of the original programmer, all strings contain parseable numbers. This assumption obviously failed.

    There are some problems that cannot be solved without prior investigation, and as a responsible programmer you should reject programming a "lucky game" that can destroy the reputation of your company.

  • siciac (unregistered) in reply to Little Bobby Tables

    No, you don't want "20" < "priority 21", you want the sender to give you the correct type.

    Change the signature to only accept ints. Not Integers, not strings, just ints.

    Fix the compiler errors, because the compiler will tell you everywhere you're using a string.

    Then you're done and it actually works.

  • (nodebb) in reply to WhatEver

    And as for "p1.CompareTo(p2)". That's a lexicographic comparison, not a numerical one.

    No. The Integer class does numeric comparison. (Really! Look it up if you don't believe me.)

  • WTFGuy (unregistered) in reply to I'm not a robot


    But I'll also raise support for what Code Refactorer said. The fact a dev can concoct an algorithm which transitively compares "foo", 12, and "13" is still wrong if it doesn't compare the same way as the humans who decided those various value should be comparable.

    Our goal as devs is to model a very messy real world in our oh-so-precise and oh-so-delicate code. Ideally along the way we teach the business to be more disciplined and less PHB-driven and messy. But we're stuck with the underlying reality that "correct" is totally in the eye of the customer-user and has nothing to do with lexicography, Unicode, or integer sorting.

    If they're crazy enough to declare "foo" to be a valid priority, they're also the only ones who can tell us whether that sorts after "chocolate" and before "banana" or between 17 & 18. Or between 42 and Unicode U+1F4A9 ๐Ÿ’ฉ.

  • Fernando (unregistered)

    The worst part is that the code contains a hidden bug

    When ran it gives the following:

    compare(1, "a") == 1 compare("a", 1) == 0

    This is because both strings are converted to numbers in the same try-catch block

    The worst part of these kind of bugs is that it may randomly break at a Java update, as Java updates only test correctness based on what the spec for compare say

  • Paul Neumann (unregistered) in reply to gnasher729

    The whole range of Unicode handled correctly.

    Because it's entirely obvious that ๐Ÿ˜ข should come after ๐Ÿ˜„ but before ๐Ÿ˜ป

Leave a comment on “Without Compare”

Log In or post as a guest

Replying to comment #:

« Return to Article