David E Jones wrote:
>
> 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,
> trying to use 30 largely similar methods is a burden on the brain...
>
> The idea of the parameters object or a small number of methods with more
> parameters would be that you wouldn't have so many methods, and you
> could select/use the options you want.
>
> Anyway, please see the attached patch for my first attempt at creating a
> consolidated set of feature-complete methods. There are a bunch of
> 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
> new methods.
Error messages need to be updated; findByPrimaryKey -> findOne,
findByCondition -> findList, etc
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.
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).
Why aren't findByAnd and findByOr converted to findList? Why not just
convert them to conditions?
In findList, EntityListIterator.getCompleteList() will never return
null. There is no need to check for that.
Transaction processing is wrong. There are error conditions that will
not cause a rollback to occur; namely, Runtime or Errors. The proper
way to do this, is to set a flag to true right before the inner block is
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
algorithm is pushed into Callable.call, you can use the chain pattern to
automatically do transaction processing, caching, or eca stuff.
