yep, anywhere away from the current code.
On Tue, Mar 27, 2012 at 10:44 AM, Jacques Le Roux <jacques.le.r...@les7arts.com> wrote: > From: "Mansour Al Akeel" <mansour.alak...@gmail.com> > >> Jacopo, >> >> I didn't know about the deprecated methods, and I didn't want to open >> the subject of changing methods, because I don't what methods are used >> a lot in the applications code, and don't want to introduce a lot of >> change. However, in my opinion, I don't see a need for something like: >> >> public void clearCacheLine(GenericPK primaryKey); > > > It's used in GenericDelegator.removeByPrimaryKey() and > GenericDelegator.removeAll() > This usage is inside the delegator itself so it can be wrapped, but from applications it is used only. ./product/src/org/ofbiz/product/product/ProductEvents.java Additional components under "framework" needs it. Will need to plan before we start and create a branch. > >> I am not sure if an application programmer will be interested in >> something like this. If I have to design Delegator class, I will >> restrict the methods only to those that offer some sort of >> functionality to the application programmer, and hide everything else. >> However, to avoid breaking any existing application code, I suggested >> only a refactor. However you suggestion is useful, and would make >> things easier, and give a ofbiz entity engine a cleaner code base. >> >> Definitely, I am supporting this idea. If we are going to do a >> refactor, we might as well, remove deprecated methods, and replace >> methods in the application code, that uses those deprecated calls. >> >> So the starting point to tackle this task with minimal risk would be, >> decide and remove deprecated methods from Delegator interface, then >> clean the applications code and replace those calls. >> >> Is that ok ? Do you see a need for a fork ? > > > A branch would be safer > > Jacques > > >> >> >> >> 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. >> >> >
