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.
