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. >
