db-torque-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Thomas Fox <Thomas....@seitenbau.net>
Subject Re: BasePeerImpl, was: Re: RFD: RecordMappers, Peers and MapBuilders
Date Thu, 16 Aug 2012 12:26:44 GMT

First of all, the generification of BasePeerImpl is a good thing as it
helps reducing the amount of generated code. Thanks a lot, Thomas!
I have created a jira issue for it (TORQUE-220)

Still, I have a few remarks. I can do the implementation if changes are
agreed on.

I am not too happy with the implementation of
public static <T> BasePeerImpl<T> getBasePeerImpl()
as the cast can go astray ?
Also we're currently using two instances from BaseSomePeer: a BasePeerImpl
instance and a SomePeerImpl instance.
This was already a hack when I implemented it but is worse now as it
prevents methods from generated
SomeBasePeer to be moved to BasePeer.
The problem was and is that the instances have information about a specific
table but the instance stored in BasePeer cannot have this information.
The best solution for this I can come up with is to move all methods to the
generated static wrapper and thus only have one instance.
Also then we should remove all references to BasePeer in the runtime and
either deprecate the class or totally remove it.
What do you think ?

I am not happy with the getters of TableMap, databaseMap and RecordMapper
in BasePeerImpl throwing runtime exceptions.
They are used quite often, and this means that RuntimeExceptions can occur
quite often.
It would be better if these methods would throw a checked TorqueException
(as in other places where initialisation has failed)

We should mention the semantic difference between the doDelete
(util.Criteria) and doDelete(criteria.Criteria) in the migration guide.
The doDelete(util.Criteria) and doDelete(util.Criteria, Connection) methods
guess the table name from the Criteria,
whereas the doDelete(criteria.Criteria) and doDelete(criteria.Criteria,
Connection) use the table name from the injected table map.

I am wondering whether the doSelect(..., RecordMapper, ...) methods should
be still template methods (meaning using <TT> instead of <T>).
This means that one can use AuthorPeer to select a book list. In my opinion
it is not necessary that this is possible.
Also the methods
doSelectSingleRecord(Criteria, RecordMapper<T> mapper, TableMap)
and
doSelectSingleRecord(Criteria, RecordMapper<T> mapper, TableMap,
Connection)
are not template methods. At least there should be consistency

Should there still be methods where a table map can be passed in
explicitly ?
In my opinion we can remove those and use the internal table map only.

The throws clause from addSelectColumns can be removed as no
TorqueException can occur in the current implementation.

Is there a hard reason to tie the largeSelect class to the new Criteria
object and not CriteriaInterface<?>

I would also add the methods BasePeerImpl.addSelectColumns(util.Criteria)
and BasePeerImpl.correctBooleans(util.Criteria) for they are necessary when
working with the old-style criteria
(setting the generation option torque.om.criteriaClass =
org.apache.torque.util.Criteria and torque.om.criterionClass =
org.apache.torque.util.Criteria.Criterion)

I removed the commented-out mergepoints and the referenced templates from
basePeerForViewImpl.vm

     Thomas


---------------------------------------------------------------------
To unsubscribe, e-mail: torque-dev-unsubscribe@db.apache.org
For additional commands, e-mail: torque-dev-help@db.apache.org


Mime
View raw message