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

Convert Javolution collections into Java collections

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Implemented
    • Affects Version/s: Trunk
    • Fix Version/s: 16.11.01
    • Component/s: ALL COMPONENTS
    • Labels:
      None
    • Sprint:
      Bug Crush Event - 21/2/2015
    1. OFBIZ-5781.patch
      1.21 MB
      Nicolas Malin
    2. OFBIZ-5781.patch
      104 kB
      Deepak Dixit
    3. OFBIZ-5781-applications.patch
      921 kB
      Nicolas Malin

      Issue Links

        Activity

        Hide
        soledad Nicolas Malin added a comment -

        Ok done on revision 1672752

        I haven't found regression and all unit test are green.

        Thanks Deepak and Adrian

        Show
        soledad Nicolas Malin added a comment - Ok done on revision 1672752 I haven't found regression and all unit test are green. Thanks Deepak and Adrian
        Hide
        soledad Nicolas Malin added a comment -

        Good spotting Deepak, ShipGroups.groovy with the first groovy that I converted.

        I have no time this days, so if you want correct it, let'fun otherwise I will update with your good remarks tomorrow (maybe)

        Show
        soledad Nicolas Malin added a comment - Good spotting Deepak, ShipGroups.groovy with the first groovy that I converted. I have no time this days, so if you want correct it, let'fun otherwise I will update with your good remarks tomorrow (maybe)
        Hide
        deepak.dixit Deepak Dixit added a comment - - edited

        I found an issue in applications/order/webapp/ordermgr/WEB-INF/actions/order/ShipGroups.groovy

        -        line = FastMap.newInstance();
        +        line = new LinkedList();
        It should be map instead of LinkedList:
        line = [:];
        
        and same at line 77
        

        Could you please check this kind of typo once again?

        Show
        deepak.dixit Deepak Dixit added a comment - - edited I found an issue in applications/order/webapp/ordermgr/WEB-INF/actions/order/ShipGroups.groovy - line = FastMap.newInstance(); + line = new LinkedList(); It should be map instead of LinkedList: line = [:]; and same at line 77 Could you please check this kind of typo once again?
        Hide
        deepak.dixit Deepak Dixit added a comment -

        Thanks Nicolas for working on it. I reviewed your patch and it looks good to me.

        I would like to change list/map assignment in groovy:
        Instead on using new to create an list/map we can use groovy utility "as" to initialize list/Map.

        List currentAssetAndExprs = new LinkedList(mainAndExprs);
        Instead of this we can use 
        List currentAssetAndExprs = mainAndExprs as LinkedList;
        
        trail = new LinkedList<String>();
        Instead of this
        trail = [] as  LinkedList<String>;
        
        Show
        deepak.dixit Deepak Dixit added a comment - Thanks Nicolas for working on it. I reviewed your patch and it looks good to me. I would like to change list/map assignment in groovy: Instead on using new to create an list/map we can use groovy utility "as" to initialize list/Map. List currentAssetAndExprs = new LinkedList(mainAndExprs); Instead of this we can use List currentAssetAndExprs = mainAndExprs as LinkedList; trail = new LinkedList< String >(); Instead of this trail = [] as LinkedList< String >;
        Hide
        soledad Nicolas Malin added a comment -

        Ashish, Deepak I load a new patch with convert javolution.

        run-tests is green and I didn't saw error on birt report and screen.

        Do you want review the patch before commit ? otherwise I can commit it this week-end

        Show
        soledad Nicolas Malin added a comment - Ashish, Deepak I load a new patch with convert javolution. run-tests is green and I didn't saw error on birt report and screen. Do you want review the patch before commit ? otherwise I can commit it this week-end
        Hide
        soledad Nicolas Malin added a comment -

        I convert applications code. If somebody would be check it before commit on trunk

        Show
        soledad Nicolas Malin added a comment - I convert applications code. If somebody would be check it before commit on trunk
        Hide
        deepak.dixit Deepak Dixit added a comment -

        Here is the patch for the javolution collection into groovy collection.
        Done following changes :

        • Removed the FastSet, FastList and FastMap from groovy classes.
        • Used groovy collections for all the javolution collection
        • Replace FastList with []
        • Replace FastMap with [:]
        • Replace FastSet with [] as Set

        http://groovy.codehaus.org/Collections

        Show
        deepak.dixit Deepak Dixit added a comment - Here is the patch for the javolution collection into groovy collection. Done following changes : Removed the FastSet, FastList and FastMap from groovy classes. Used groovy collections for all the javolution collection Replace FastList with [] Replace FastMap with [:] Replace FastSet with [] as Set http://groovy.codehaus.org/Collections
        Hide
        deepak.dixit Deepak Dixit added a comment -

        I am not a groovy guru, I'll try to remove Javolution from groovy script and upload patch ASAP.

        Show
        deepak.dixit Deepak Dixit added a comment - I am not a groovy guru, I'll try to remove Javolution from groovy script and upload patch ASAP.
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        I removed Javolution from most of the framework. I need a Groovy guru to remove Javolution from the Groovy scripts.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - I removed Javolution from most of the framework. I need a Groovy guru to remove Javolution from the Groovy scripts.
        Hide
        soledad Nicolas Malin added a comment -

        Thanks Adrian, I can see it in detail

        Show
        soledad Nicolas Malin added a comment - Thanks Adrian, I can see it in detail
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        See revision 1527212.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - See revision 1527212.
        Hide
        soledad Nicolas Malin added a comment -

        For example, Javolution Map Iterators return items in insertion sequence (HashMap does not), so any code relying on the original insertion order must use a LinkedHashMap.

        Adrian, do you have an example, in OFBiz I have a doubt if I have already seen a ordered map with linked association.

        Show
        soledad Nicolas Malin added a comment - For example, Javolution Map Iterators return items in insertion sequence (HashMap does not), so any code relying on the original insertion order must use a LinkedHashMap. Adrian, do you have an example, in OFBiz I have a doubt if I have already seen a ordered map with linked association.
        Hide
        soledad Nicolas Malin added a comment -

        To give an idea :

        • FastMap 1478 instantiations
          • applications 865
            • content 213
            • humanres 14
            • manufacturing 81
            • marketing 2
            • order 117
            • party 34
            • product 232
            • securityext 2
            • workeffort 23
          • framework 283
            • entityext 12
            • service 43
            • sql 6
            • testtools 1
            • webapp 30
            • webtools 37
            • widget 69
          • specialpurpose 330
            • ebay 37
            • ebaystore 150
            • ecommerce 4
            • example 1
            • googlebase 4
            • googlecheckout 9
            • hhfacility 3
            • oagis 59
            • pos 19
            • scrum 11
            • webpos 4
        • FastList 1309
          • applications 707
            • content 109
            • humanres 5
            • manufacturing 46
            • marketing 2
            • order 154
            • party 41
            • product 193
            • workeffort 48
          • framework 388
            • entityext 43
            • service 33
            • sql 40
            • testtools 4
            • webapp 9
            • webtools 50
            • widget 147
          • specialpurpose 206
            • ebay 13
            • ebaystore 75
            • ecommerce 4
            • googlebase 7
            • googlecheckout 1
            • oagis 16
            • pos 10
            • scrum 56
            • webpos 8
        Show
        soledad Nicolas Malin added a comment - To give an idea : FastMap 1478 instantiations applications 865 content 213 humanres 14 manufacturing 81 marketing 2 order 117 party 34 product 232 securityext 2 workeffort 23 framework 283 entityext 12 service 43 sql 6 testtools 1 webapp 30 webtools 37 widget 69 specialpurpose 330 ebay 37 ebaystore 150 ecommerce 4 example 1 googlebase 4 googlecheckout 9 hhfacility 3 oagis 59 pos 19 scrum 11 webpos 4 FastList 1309 applications 707 content 109 humanres 5 manufacturing 46 marketing 2 order 154 party 41 product 193 workeffort 48 framework 388 entityext 43 service 33 sql 40 testtools 4 webapp 9 webtools 50 widget 147 specialpurpose 206 ebay 13 ebaystore 75 ecommerce 4 googlebase 7 googlecheckout 1 oagis 16 pos 10 scrum 56 webpos 8
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        More advice:

        A common design pattern in OFBiz is converting various XML files to Java models. In those cases, collections are assembled once, and then they are accessed many times. Using Javolution in those cases is pointless - because the design depends on objects being returned to the object pool, which will never happen in those cases. So, I recommend those instances be converted first.

        There are many examples of conversion from Javolution to java.util classes in recent code.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - More advice: A common design pattern in OFBiz is converting various XML files to Java models. In those cases, collections are assembled once, and then they are accessed many times. Using Javolution in those cases is pointless - because the design depends on objects being returned to the object pool, which will never happen in those cases. So, I recommend those instances be converted first. There are many examples of conversion from Javolution to java.util classes in recent code.
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        Be very careful when selecting the java.util.* replacement. The replacement must duplicate the behavior of the Javolution class.

        For example, Javolution Map Iterators return items in insertion sequence (HashMap does not), so any code relying on the original insertion order must use a LinkedHashMap.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - Be very careful when selecting the java.util.* replacement. The replacement must duplicate the behavior of the Javolution class. For example, Javolution Map Iterators return items in insertion sequence (HashMap does not), so any code relying on the original insertion order must use a LinkedHashMap.

          People

          • Assignee:
            soledad Nicolas Malin
            Reporter:
            deepak.dixit Deepak Dixit
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development

                Agile