Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-6291

Update code to check for types rather than throw ClassCastException

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: Trunk, 16.11.01
    • Fix Version/s: 16.11.01
    • Component/s: framework
    • Labels:
      None

      Description

      framework/minilang/src/org/ofbiz/minilang/method/conditional/Compare.java
      framework/base/src/org/ofbiz/base/util/ObjectType.java
      framework/minilang/src/org/ofbiz/minilang/MiniLangUtil.java

      all throw ClassCastExceptions and are ignored instead of checking types

      This caused issues in debugging when adding a ClassCastException breakpoint

      1. LocalizedConverters.patch
        2 kB
        Gareth Carter
      2. minilang_compare_r1675655.patch
        7 kB
        Gareth Carter
      3. minilang_compare.patch
        9 kB
        Gareth Carter

        Activity

        Hide
        gareth.carter Gareth Carter added a comment -

        LocalizedConverters.patch fixes ObjectType.java and MiniLangUtil.java

        Show
        gareth.carter Gareth Carter added a comment - LocalizedConverters.patch fixes ObjectType.java and MiniLangUtil.java
        Hide
        gareth.carter Gareth Carter added a comment -

        minilang_compare.patch fixes Compare.java. I have remove compareBigDecimals method (even though a comment says not). BigDecimal compareTo disregards scale so 2.0 and 2.00 are the same. See java docs

        Show
        gareth.carter Gareth Carter added a comment - minilang_compare.patch fixes Compare.java. I have remove compareBigDecimals method (even though a comment says not). BigDecimal compareTo disregards scale so 2.0 and 2.00 are the same. See java docs
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        As the code comment states, the current comparison behavior is by design. It is not a bug that needs to be fixed.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - As the code comment states, the current comparison behavior is by design. It is not a bug that needs to be fixed.
        Hide
        gareth.carter Gareth Carter added a comment -

        Just realised minilang_compare.patch does not apply against latest trunk. Will provide another patch. The patch will apply against 1663617. The patch could ignore compareBigDecimals, though I still think it's irrelevant 2.0 = 2.00 = 2.000 = 2.0000 with compareTo. You will get the same result, all you've done is create a few more lines of code

        Show
        gareth.carter Gareth Carter added a comment - Just realised minilang_compare.patch does not apply against latest trunk. Will provide another patch. The patch will apply against 1663617. The patch could ignore compareBigDecimals, though I still think it's irrelevant 2.0 = 2.00 = 2.000 = 2.0000 with compareTo. You will get the same result, all you've done is create a few more lines of code
        Hide
        doogie Adam Heath added a comment -
        • The patch introduces new blank lines; that should be done separately.
        • int i = foo.compareTo(baz); return i <= 0; this is an anti-pattern. Remove the use of the local variable.

        Also, doing if (foo instanceof Bar), then casting with (Bar), is slower, than just attempting the cast, and catching an exception. If the fast-path items are done first, then no throw ever happens, and you don't waste cycles on the instanceOf operator, and the jump that happens.

        I haven't attempted to apply the patch, I was just reading it, but a smell test seems to say -1.

        Also, I have done some unit tests against ObjectType in the past; I don't know how current they still are. The change to an instanceOf condiitional needs to pass those tests, and, should also be done with cobertura, and the code coverage should stay the same before and after. But that all hinges on the currentness of the unit testing.

        Show
        doogie Adam Heath added a comment - The patch introduces new blank lines; that should be done separately. int i = foo.compareTo(baz); return i <= 0; this is an anti-pattern. Remove the use of the local variable. Also, doing if (foo instanceof Bar), then casting with (Bar), is slower, than just attempting the cast, and catching an exception. If the fast-path items are done first, then no throw ever happens, and you don't waste cycles on the instanceOf operator, and the jump that happens. I haven't attempted to apply the patch, I was just reading it, but a smell test seems to say -1. Also, I have done some unit tests against ObjectType in the past; I don't know how current they still are. The change to an instanceOf condiitional needs to pass those tests, and, should also be done with cobertura, and the code coverage should stay the same before and after. But that all hinges on the currentness of the unit testing.
        Hide
        gareth.carter Gareth Carter added a comment -

        "int i = foo.compareTo(baz); return i <= 0; this is an anti-pattern. Remove the use of the local variable." - Really? Creating a local variable that is useful for debugging is considered an anti pattern? Can you provide a source?

        I doubt it's that much slower but will do some testing on it. See http://stackoverflow.com/questions/103564/the-performance-impact-of-using-instanceof-in-java for some performance tests.

        Personally, I think catching and swallowing any exceptions into a blackhole for this purpose is an anti pattern. http://www.odi.ch/prog/design/newbies.php

        Show
        gareth.carter Gareth Carter added a comment - "int i = foo.compareTo(baz); return i <= 0; this is an anti-pattern. Remove the use of the local variable." - Really? Creating a local variable that is useful for debugging is considered an anti pattern? Can you provide a source? I doubt it's that much slower but will do some testing on it. See http://stackoverflow.com/questions/103564/the-performance-impact-of-using-instanceof-in-java for some performance tests. Personally, I think catching and swallowing any exceptions into a blackhole for this purpose is an anti pattern. http://www.odi.ch/prog/design/newbies.php
        Hide
        doogie Adam Heath added a comment -

        "int i = foo..." isn't the part that is slower. The instanceOf pattern is slower.

        Good debuggers can check the return value of the expression before returnning, without the need for a local variable. It introduces additional noise.

        Also, I'm differing to Adrian here, who has a bit more familiarity with that code; I have added this to my list of things to check, but I'm working on several other branches right now against trunk, so please be patient.

        Show
        doogie Adam Heath added a comment - "int i = foo..." isn't the part that is slower. The instanceOf pattern is slower. Good debuggers can check the return value of the expression before returnning, without the need for a local variable. It introduces additional noise. Also, I'm differing to Adrian here, who has a bit more familiarity with that code; I have added this to my list of things to check, but I'm working on several other branches right now against trunk, so please be patient.
        Hide
        gareth.carter Gareth Carter added a comment -

        What I have found is the cast to BigDecimal is fastest only if the object being cast is always BigDecimal, casting another object (eg java.lang.String) without type check (eg, catching and ignoring ClassCastException) is much slower. But these are in the nanosecond range of performance, hardly an argument to use one method or another. I just think it would be better to check for types rather than throwing an uncaught exception and not doing anything

        I appreciate all committers are busy with life, work or other. This is a trivial issue, I don't believe this will have any functional impact, its just an improvement in code readability and follows better practices.

        I still have reservations about compareBigDecimals but this is another issue to discuss somewhere else

        Show
        gareth.carter Gareth Carter added a comment - What I have found is the cast to BigDecimal is fastest only if the object being cast is always BigDecimal, casting another object (eg java.lang.String) without type check (eg, catching and ignoring ClassCastException) is much slower. But these are in the nanosecond range of performance, hardly an argument to use one method or another. I just think it would be better to check for types rather than throwing an uncaught exception and not doing anything I appreciate all committers are busy with life, work or other. This is a trivial issue, I don't believe this will have any functional impact, its just an improvement in code readability and follows better practices. I still have reservations about compareBigDecimals but this is another issue to discuss somewhere else
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        Mini-language is a scripting language. One of the nice things about scripting languages is you can compare things like decimal values without worrying about the underlying implementation. That is what the current implementation does: 2 == 2.0 == 2.000000.

        If we were to compare BigDecimal precision, then we lose the scripting abstraction, PLUS pretty much everything that uses Mini-language will break.

        If you need to compare BigDecimal precision, then you can use a scriptlet: groovy:fooBigDecimal.equals(barBigDecimal).

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - Mini-language is a scripting language. One of the nice things about scripting languages is you can compare things like decimal values without worrying about the underlying implementation. That is what the current implementation does: 2 == 2.0 == 2.000000. If we were to compare BigDecimal precision, then we lose the scripting abstraction, PLUS pretty much everything that uses Mini-language will break. If you need to compare BigDecimal precision, then you can use a scriptlet: groovy:fooBigDecimal.equals(barBigDecimal).
        Hide
        gareth.carter Gareth Carter added a comment -

        What about comparing 2.001 and 2.0001? Lets say both numbers are BigDecimal in both groovy and minilang.
        In groovy, == is false, compareTo() is 1 and equals() is false.
        In minilang, if 'if-compare-field' type is set to BigDecimal, the result is true because compareBigDecimals sets the scale to the smallest scale and rounds up, in this instance 2.0001 will result in 2.001 and compareTo equals 0.

        Why is this done in minilang and no where else?
        Is this correct? Condidering the same value in another type float/double/string would be false?
        What about com.ibm.icu.math.BigDecimal?

        On further investigation, I doubt this will affect much in minilang considering the default conversion type on if-compare and if-compare-field is String so compareBigDecimals would only be called when type="BigDecimal"

        eg:
        <if-compare-field operator="equals" field="d1" to-field="e1" type="BigDecimal">
        </if-compare-field>

        Show
        gareth.carter Gareth Carter added a comment - What about comparing 2.001 and 2.0001? Lets say both numbers are BigDecimal in both groovy and minilang. In groovy, == is false, compareTo() is 1 and equals() is false. In minilang, if 'if-compare-field' type is set to BigDecimal, the result is true because compareBigDecimals sets the scale to the smallest scale and rounds up, in this instance 2.0001 will result in 2.001 and compareTo equals 0. Why is this done in minilang and no where else? Is this correct? Condidering the same value in another type float/double/string would be false? What about com.ibm.icu.math.BigDecimal? On further investigation, I doubt this will affect much in minilang considering the default conversion type on if-compare and if-compare-field is String so compareBigDecimals would only be called when type="BigDecimal" eg: <if-compare-field operator="equals" field="d1" to-field="e1" type="BigDecimal"> </if-compare-field>
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        ClassCastException fixed in rev 1680058. Thanks!

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - ClassCastException fixed in rev 1680058. Thanks!
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Smart move, thanks Adrian!

        Show
        jacques.le.roux Jacques Le Roux added a comment - Smart move, thanks Adrian!
        Hide
        gareth.carter Gareth Carter added a comment -

        Thanks Adrian.

        Show
        gareth.carter Gareth Carter added a comment - Thanks Adrian.

          People

          • Assignee:
            adrianc@hlmksw.com Adrian Crum
            Reporter:
            gareth.carter Gareth Carter
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development