>From Delegator.java 139 public GenericDelegator cloneDelegator(); 140 141 public GenericDelegator cloneDelegator(String delegatorName);
This has to be changed. Delegator is an interface, and it has a reference to its implementation "GenericDelegator". Anyone knows of anything that might break, or a reason not to change this ? I will open an issue, for this. On Tue, Mar 27, 2012 at 4:02 PM, Mansour Al Akeel <mansour.alak...@gmail.com> wrote: > Jacopo, > I am still thinking about your suggestion to change the Delegator > interface and use only subset of the methods. > If you have some thought ready, please share them and discuss. > > If you have some plan to introduce these changes, then let me know about them. > > I think we will have to change many things anyway if we need to refactor. > It's not like what I initially thought, just moving code around will be > enough. > > > > On Tue, Mar 27, 2012 at 3:40 AM, Jacopo Cappellato > <jacopo.cappell...@hotwaxmedia.com> wrote: >> Mansour, >> >> it is interesting. >> Maybe it would be easier if we simplify the existing classes (and >> application code that uses them) to use only a subset of the methods >> available (some of them are already deprecated); the refactoring based on >> design patterns will be easier at that point. >> >> Jacopo >> >> On Mar 26, 2012, at 11:25 PM, Mansour Al Akeel wrote: >> >>> Hello all, >>> I am looking at the code for the GenericDelegator. We have 2913 line >>> of code in one class !!! >>> This class handles a lot of functions and I think it's the most >>> critical to the application. These functions can >>> be distributed among more classes, providing better maintainability >>> and extensibility. I can see the best pattern to >>> be used here are chain of responsibilities and decorator, where each >>> class provide a defined set of functionality and implement the >>> Delegator interface. >>> >>> For example, currently this class handles caching, configurations, >>> model Reading, encryption, logging, and the main function of accessing >>> data. >>> It's true that the interface it implements is huge (1217 line of code) >>> but mostly comments and documentation. CoR design pattern will allow >>> easier >>> unit testing, and better maintainability and allow to extend. So let's say: >>> >>> GenericDelegator implements Delegator (Interfaces with the external world ) >>> ===> passes calls to SQLQueriesDelegator (build the sql through >>> implementing findXXX()) >>> =====> passes calls to LoggingDelegator >>> =====> passes calls to ModuleReaderDelegator >>> ======> MultiTenantDelegator >>> ========> CachingDelegator >>> ==========> EncryptionDelegator >>> ============> etc >>> >>> Each delegator does some work and calls the next one in chain until >>> the final functionality is achieved. Of course customization will be >>> easier. >>> For example, if I want to swap the current caching implementation with >>> something like memcahe or ecache, I have to change in many places in >>> the single 2913 line-of-code class !! With CoR all I do is swap the >>> current CachingDelegator class with my own. Let's say I want to access >>> the file system >>> or jcr nodes in the same unified way I access the db. In this case all >>> I need to do is add some thing like "RequestRoutingDelegator", and >>> JCRDelegator. >>> The RequestRouting decides how to access the entity in hand and based >>> on that uses either the SQLDelegator or JCRDelegator. >>> Those who did xml processing with SAX2 filters (CoR) or worked with >>> java IO streams (Decorator Pattern) will appreciate the concept of >>> separating functionality. An example in JavaIO, if one needs to ecrypt >>> a stream, unzip the input, or zip the output .... etc. All these is >>> done though decorator pattern. >>> >>> The whole chain can be created and constructed in the factory method >>> (or using a container config in the future), so existing applications >>> should not see any changes, as all of these delegators will be hidden >>> by package visibility (except for the first one in the chain). >>> In the factory method (hopefully in the future called through a >>> service locator), we can do something like: >>> >>> Delegator encryptionDelegator = new EncryptionDelegator(); >>> Delegator cachingDel = new CachingDelegator( encryptiongDelegaort); >>> ..... >>> ..... // may be do some configuration as we dont want every delegator >>> to configure itself by reading a file or looking up a >>> // of a property, in case we used a container in the future. >>> .... >>> Delegator realOne = new GenericDelegator(); >>> >>> static methods can be moved where needed and ensure there's not >>> duplication of functionality. At some point we may consider >>> making them instance methods. >>> >>> To avoid DRY, we can create a class (implement Delegator) with default >>> functionality of calling the child (next) delegator. >>> Then just extend it and override whatever we need. This way we keep >>> the code minimal, and clean. >>> >>> At the end, assuming no advantage gained, who wants to maintain 2913 >>> line of code with some lines more than 100 character wide ??!! >>> >>> I am not sure about the amount of work it takes to finalize it, but I >>> can see some difficulties. For example, assuming two different methods >>> belongs to two different layers, need one of the static methods in the >>> GenericDelegator. This will lead to duplicate code, we are going to >>> call static methods in other classes, and creating a mess again. >>> Another issue is when an object needs to be accessed in different >>> layers. This means we need to keep a reference for that object in >>> multiple places. What if it changed in one of the layers ? For >>> example, ModelReader is accessed in cloneDelegator and in >>> decryptField. Both of these methods belongs to different Delegators. >>> cloneDelegator goes into GenericDelegator, and decryptField goes into >>> EncryptionDelegator. What if the model reference changed in one of >>> them ?? Caution needed ! >>> >>> Another problem is when the order of calls in GeneralDelegator is not >>> consistent. For example, in one method we accessed the cache then we >>> did the logging. In another method we did the opposite, we logged, >>> then we accessed the cache. When done in layers, it will be harder to >>> do this. >>> >>> Another alternative is something close to the composite pattern, where >>> we have Helper classes with a subset of Delegator implementation. The >>> GenericDelegator class, instantiates them and keeps a reference to >>> each of them. Then calls the appropriate method. This really simple, >>> but less >>> sexy, as extending the functionality still requires looking between >>> the lines. It's just breaking the HUGE class into smaller classes. >>> >>> Not sure if I missed something. Other patterns could fit better. If >>> someone captured something, please let us know. Ofbiz team can decide >>> what path to follow. >>> >>> Since this component is critical to the application, some may prefer a >>> more senior resource on the team and more skilled >>> with the internals of ofbiz, to handle this task. However, I don't >>> have a problem giving it a try. This task may sound small, but I don't >>> know what is a head of us. Someone with deeper knowledge about the >>> entity engine can comment. >>> Alternatively, one of the commiters, can adopt this task, and do the >>> cleaning as needed. For example, every while and then revisit this >>> huge class, and >>> break separate the functionality into the corresponding classes. >>> >>> I didn't want to open this subject now, as I am aware of the cleaning >>> being performed by the team. However, if we can not do it now, and we >>> agreed about the path, it can be added to JIRA for future reference. >>> >>> Please discuss and share knowledge. >>
