In the spirit of changing the entity/delegator interface more object friendly,
why not take this to then next step and generate POJO interfaces for each
entity. These would extend GenericValue but provide a simple gettor/settor
facade allowing compile time type checking and removing of the "string" code
for much of the business logic written in java.
We have done such a thing (again in our forked application), and it makes the
Java code much more readable and easier to use. The general structure is
public class Person extends AbstractGenericValue<Person>
{
public static final String ENTITY = "Person";
// constructor, only called from makeValue, MUST be associated with a
delegator
protected Person(Delegator delegator) {...}
// factory method
public static Person newInstance(Delegator delegator) {...}
// generate finders, by pkey, etc...
public static Person findOne(Delegator delegator, String partyId){...}
// getter and settors
public String getFirstName() {
return getString("firstName");
}
public Person setFirstName(String value) {
set("firstName", value);
return this;
}
// relationships
public Party getParty() throws GenericEntityException {...}
public PartyNameView getPartyNameView() throws GenericEntityException{...}
}
This allows code that is much easier to debug and less error prone.. example
below is for navigating orders.
OrderHeader orderHeader = OrderHeader.findOne(delegator, orderId);
// get the orderItems
List<OrderItem> orderItems = orderHeader.getOrderItemList();
BigDecimal totalQuantity = BigDecimal.ZERO;
for (OrderItem orderItem: orderItems) {
totalQuantity = totalQuantity.add(orderItem.getQuantity());
}
I know we want to encourage business logic in minlang, etc... but if it is
written in java, and there is a LOT of code in java, shopping cart, etc...
this makes that code MUST more readable and maintainable. The binding between
the entity model and the java implementation can be caught as a compile time
error... significantly lowers the maintenance cost of the code.
This may be pushing a rope, but we use this ALL the time for our groovy and
java code. (would also apply to jsp code obviously). Minilang code can be type
checked by the reader... (want to check for static errors in code, without the
need to "run" the code).
We have implemented the generators, and the refactoring/abstracting to enable
this. We find it works great and doesn't break ANY of the nice ofbiz extend
entity semantics, etc.... Of course if you extend an entity and then want java
business logic to use it... you need to access those items either with
"strings" as stock ofbiz, or redo an entity-gen. But if there is no java code
using the entities, no need to auto-gen.
As another note, we have done a similar thing with the service interface.... as
you might have guessed, we're a fan of ofbiz extensibility, but NOT on how it
encourages poor Java implementation practices. ("String" object references,
non-type safe, public static methods everywhere.... etc...)
Marc
----- Original Message -----
> On 10/12/2010, at 8:54 AM, Adam Heath wrote:
>
> > On 12/09/2010 01:48 PM, Scott Gray wrote:
> >> On 10/12/2010, at 5:53 AM, Adam Heath wrote:
> >>
> >>> On 12/09/2010 12:03 AM, Scott Gray wrote:
> >>>> On 9/12/2010, at 11:50 AM, Adam Heath wrote:
> >>>>
> >>>>> On 12/08/2010 03:00 PM, Scott Gray wrote:
> >>>>>> On 8/12/2010, at 4:29 AM, Adam Heath wrote:
> >>>>>>
> >>>>>>> I've deprecated findByAnd locally, replacing calls with a
> >>>>>>> variant that
> >>>>>>> takes a boolean, which specifies whether to use the cache.
> >>>>>>>
> >>>>>>> Initially, my replacement just added a new findByAnd. However,
> >>>>>>> I'm now starting to lean towards replacing with findList
> >>>>>>> instead. However, in my opinion, I think that will make
> >>>>>>> programming ofbiz more
> >>>>>>> difficult.
> >>>>>>>
> >>>>>>> I'd like to add a findList variant, that takes a Map instead
> >>>>>>> of an
> >>>>>>> EntityCondition. Without this new variant, there would be ton
> >>>>>>> of variants that would only need to import EntityCondition,
> >>>>>>> just to
> >>>>>>> create a condition.
> >>>>>>>
> >>>>>>> There are also performance considerations to consider.
> >>>>>>>
> >>>>>>> EntityCondition.makeCondition(Map) directly takes the map,
> >>>>>>> doing no
> >>>>>>> processing. However, EntityCondition.makeCondition(Object...)
> >>>>>>> eventually calls EntityUtil.makeFields, which does a little
> >>>>>>> more than
> >>>>>>> UtilMisc. In addition to the iteration over the array, it also
> >>>>>>> does a
> >>>>>>> check on the value for Comparable/Serializable. This latter
> >>>>>>> check seems a bit superfluous, as eventually the base
> >>>>>>> condition classes
> >>>>>>> check the values against the model.
> >>>>>>>
> >>>>>>> So, from a purist stand point, even tho findByAnd could be
> >>>>>>> replaced by
> >>>>>>> findList, I think it is too useful; it saves enough code in
> >>>>>>> application layers, imho.
> >>>>>>>
> >>>>>>
> >>>>>> This reminded me of the query objects with method chaining that
> >>>>>> I suggested a while back so I threw them together this morning.
> >>>>>>
> >>>>>> Here are some examples:
> >>>>>> thisContent = delegator.findByPrimaryKeyCache("Content",
> >>>>>> UtilMisc.toMap("contentId", assocContentId));
> >>>>>> // becomes
> >>>>>> thisContent =
> >>>>>> EntityOne.query(delegator).from("Content").where("contentId",
> >>>>>> assocContentId).cache().find();
> >>>>>
> >>>>> api: EntityOne(delegator).from()....
> >>>>>
> >>>>> in foo.groovy:
> >>>>> use(DelegatorCategory) {
> >>>>>
> >>>>> }
> >>>>>
> >>>>> class DelegatorCategory {
> >>>>> public static EntityOneBuilder EntityOne(Delegator delegator) {
> >>>>> return new EntityOneBuilder(delegator);
> >>>>> }
> >>>>> } class EntityOneBuilder {
> >>>>> public EntityOneBuilder from(String entityName) {
> >>>>> return this;
> >>>>> }
> >>>>>
> >>>>> public GenericValue query() {
> >>>>> return delegator.findOne(....);
> >>>>> }
> >>>>> }
> >>>>>
> >>>>> This is almost like what you suggested, but it removes the
> >>>>> query() method that starts thte builder, and changes the find()
> >>>>> to query().
> >>>>>
> >>>>> EntityList would be done the same one.
> >>>>>
> >>>>> The way you have it, is that the start(EntityOne, EntityList)
> >>>>> and the end(find(), list()), are 2 things that have to be
> >>>>> remembered. My version just has one thing(the start of the
> >>>>> call).
> >>>>
> >>>> This is all groovy DSL related though right? I hadn't worried
> >>>> about groovy too much because I knew we had a fair bit of
> >>>> flexibility thanks to the language features. The API I've written
> >>>> to date was solely intended for java development but seems
> >>>> succinct enough that not much would need to change for groovy.
> >>>>
> >>>> Also EntityList's execute methods so far are:
> >>>> list() iterator()
> >>>> first() count()
> >>>
> >>> All primary methods should be query(), imho.
> >>>
> >>> interface GenericQueryBuilder<T> {
> >>> T query();
> >>> }
> >>>
> >>> public class EntityOne implements
> >>> GenericQueryBuilder<GenericValue> {
> >>> public GenericValue query() {}
> >>> }
> >>>
> >>> public class EntityList implements
> >>> GenericQueryBuilder<List<GenericValue>>, Iterable<GenericValue> {
> >>> public List<GenericValue> query() {}
> >>> public Iterator<GenericValue> iterator() {}
> >>> ...
> >>> }
> >>
> >> I'm not opposed to that, but I'll need another method name for
> >> specifying the delegator. How about use(delegator)?
> >
> > public final class EntityBuilderUtil {
> > public static EntityOne one(Delegator delegator) {
> > return new EntityOne(delegator);
> > }
> >
> > public static EntityList list(Delegator delegator) {
> > return new EntityList(delegator);
> > }
> > }
> >
> > This also means that java api code only needs to import one class.
> > Plus, if this class is used as a groovy category, then groovy code
> > can do this:
> >
> > one(delegator).cache(true).....
> >
> > As groovy will see one as a method call, taking a variable with type
> > Delegator, and search all its categories to find a matching method.
> > You'll get all things for free.
>
> Works for me.
>
> Thanks
> Scott
