Thank you Adrian. I don't think a lot of developers will see an immediate gain. But in the long run, it will be appreciated. It allows us to introduce new functionality.
For example, in one of my previous emails, I pointed out the need for a Record Level Security, like the one offered by Oracle DB and recently by postgresql. Where the loged in user have specific access to the data. The same principal, is used in most NoSQL data bases. My plan was to have an xml config file next to the entities (ie, acl.xml), and restrict access based to entities I need. The whole idea is, to appends some query parameters to calls to DB, giving the application programmer a nice and clean way to work with records. When I looked into the current code, I didn't want to touch it. So, I thought a bout writing a new JDBC (decorator) driver that wraps the calls, to the DB. But then, I don't see this is wise. Since the JBDC driver will have to read application data, it is part of the framework. It can be done as a hack, but entity engine is more appropriate for this. Plus I may have some issues with caching. If the cache is checked before sending a call to the DB, then it may return results, a user should not see ! So, yes the pattern will make it easier for us, and application programers will find the resulting functionality that can be introduced useful. On Tue, Mar 27, 2012 at 6:12 AM, Adrian Crum <adrian.c...@sandglass-software.com> wrote: > I agree that the decorator pattern could improve the Delegator > implementation. > > Some time ago I had suggested using the decorator pattern to support DB > localization - which would eliminate the need for the clunky LocalizedMap > interface/implementations - but there wasn't much interest in it. > > -Adrian > > > On 3/26/2012 10: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.
