On 16.08.12 14:26, Thomas Fox wrote: > > 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)
Thanks for that. I still need to get used to that method. > 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. Inheritance with static classes is always a bit of a challenge. Although your suggestion will again increase the amount of generated code, it's probably the best solution. At least I don't know any better. > 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 ? As you can see from my "proof of concept" within LargeSelect, the peer instance objects can be quite useful. It should not be that difficult to remove these references. I volunteer. > I am not happy with the getters of TableMap, databaseMap and RecordMapper > in BasePeerImpl throwing runtime exceptions. That's just the usual method I use. These exceptions are not expected to occur in a regular application and mean that you did not initialize the objects accordingly. The exceptions are thrown to remind you of that fact. I would not expect a simple getter to throw a regular exception. That said, I'm fine with changing the exceptions to TorqueException because I don't see any usage of them outside a database operation. > 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'd remove the doDelete(util.Criteria) and doDelete(util.Criteria, Connection) method from the class. They are bound to fail and we have working replacements. > I am wondering whether the doSelect(..., RecordMapper, ...) methods should > be still template methods (meaning using <TT> instead of <T>). The doSelectJoinXXX() methods call these with different RecordMappers so they might return a different data type. We may reduce their visibility to protected to avoid misuse. > 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 Agreed. > 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. Agreed. > The throws clause from addSelectColumns can be removed as no > TorqueException can occur in the current implementation. Agreed. > Is there a hard reason to tie the largeSelect class to the new Criteria > object and not CriteriaInterface<?> There was one select case I failed to manage with the old type Criteria. > 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'd like to strongly suggest again that we don't publish Torque 4 with two different Criteria implementations. That will cause headaches with both, usage and support, I promise. Bye, Thomas. --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
