Re: Simplifying GenericDelegator

[David E Jones]

On Oct 25, 2007, at 10:20 AM, Adam Heath wrote:

David E Jones wrote:

trying to use 30 largely similar methods is a burden on the brain...


parameters would be that you wouldn't have so many methods, and you
could select/use the options you want.


consolidated set of feature-complete methods. There are a bunch of

new methods.

Yeah, I'm not sure I like having all of these little methods ANYWHERE, even in better organized in a special class. No matter how you slice it, The idea of the parameters object or a small number of methods with more Anyway, please see the attached patch for my first attempt at creating a changes, so best to focus on the new methods. The other changes are just refactoring so the old big set of methods we want to deprecate use the

Error messages need to be updated; findByPrimaryKey -> findOne,
findByCondition -> findList, etc

Yes, good point. Maybe I'll change those a little so they don't have this problem.

An additional method should be added to GenericEntity(and friends), that

mirrors makePKSingle; instead of returning the appropriate
GenericEntity, it returns a map properly constructed with whatever
files.  That way, you don't create a GenericEntity, just to throw it
away immediately.

This will likely be deprecated anyway.

When introducing a new feature, do not do other unescessary things in
the patch.  Like prefixing unaltered method calls with this.  It makes

it much harder to track down issues when they occur. Please always run svn diff and sanity check your changes(for instance, there is a findByOr that is converted to a this.findByOr, with no other changes on the line).

If it's too complicated for you to review that's your problem.

Oh yeah, Mr. Man... why should I? Why shouldn't I clean things up while refactoring? When people are submitting things for committers to review, great, but if I'm working on code I'll clean up every darn thing I see as that is MY responsibility. If you wanna fight about it get yer dukes up boy! (yeah, them's ma faghtin' wouds, I wanna get me in a faght)

Why aren't findByAnd and findByOr converted to findList? Why not just

convert them to conditions?

Why do I care? I'm cleaning up, not optimizing. If we continue on the discussed path these will be deprecated anyway. And for myself, I have better things to do with my time. Like argue with you... wait that seems unproductive, now why am I doing this again?

In findList, EntityListIterator.getCompleteList() will never return
null.  There is no need to check for that.

Yes, good point. 17 characters removed!

Transaction processing is wrong.  There are error conditions that will
not cause a rollback to occur; namely, Runtime or Errors.  The proper

done processing, then checking that flag in the finally block, and
calling rollback/commit appropriate.  Ie:

public static <V> V runTransaction(Callable<V> proc) {
        boolean success = false;
        boolean beganTx = false;
        try {
                beganTx = TransactionUtil.begin();
                V result = proc.call();
                success = true;
                return result;
        } finally {
                if (success) {
                        TransactionUtil.commit(beganTx);
                } else {
                        TransactionUtil.rollback(beganTx);
                }
        }
}

The above is an example only.  The pattern is so common, that I've
created a local class for our application, to make this easier.
Ideally, I should send that to ofbiz.

Another nice benefit of the above, is that since the meat of the

automatically do transaction processing, caching, or eca stuff.

way to do this, is to set a flag to true right before the inner block is algorithm is pushed into Callable.call, you can use the chain pattern to

-David

I wouldn't say the transaction processing is "wrong". Sounds like it's different from what you expect. This is how it has been, and it isn't my intention right now to change the error handling semantics of these methods. And, BTW, YOU should not either unless you discuss it on the list to maybe catch some things you missed. This is a fairly material change in error handling and might cause dependent code to need changes. Whatever the case, I'd rather focus on solving problems, especially since most of this code and the techniques in it are pretty well vetted and tested.

smime.p7s
Description: S/MIME cryptographic signature