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.