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.
