db-torque-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Monroe <Greg.Mon...@dukece.com>
Subject RE: Change semantics of Criteria.add() and Criteria.or()
Date Mon, 19 Dec 2011 14:54:29 GMT
Sounds good. Eventually dropping the add methods but keeping 
the convenience and/or methods will create a much cleaner 
and easily understood structure.

Some take it or leave it suggestions:

We might still want to refactor to the Factory methodology 
even if there is only one class.  This would make it easier
for future versions to do Criteria changes without as much
disruption or for application developers to add in their 
own (DB Type specific?) versions.

I'd suggest that the Column, ColumnImpl, and ColumnValue 
classes be moved into this new criteria package.  AFAIK, 
they are only used by the Criteria class and putting them
there will keep down confusion between ColumnMap and 
Column.

Greg

-----Original Message-----
From: Thomas Fox [mailto:Thomas.Fox@seitenbau.net] 
Sent: Sunday, December 18, 2011 3:38 PM
To: Apache Torque Developers List
Subject: RE: Change semantics of Criteria.add() and Criteria.or()

Thanks, Greg and Thomas, for your feedback.

So my suggestion would be to create a new package,
org.apache,torque.criteria, and copy the Criteria class to the new package.
The old criteria class will stay as and where it is but become deprecated.
Probably there will be a deprecated interface CriteriaInterface which
collects a few Criteria methods, to reduce code duplications in the
SqlBuilder class. Most of the methods will not be in the interface as there
is a problem that most of the classes will return the class instance and
I'd rather not have the new Criteria class have methods whic return a
deprecated type.
The SqlEnum would also fit better in the new criteria package than in the
util package, so in my opinion it should be moved

My idea would be to remove the old util.Criteria class (and the
CriteriaInterface) in Torque 4.1

I like the name of the "where" method but I rather value readable code, so
I'd rather not remove the and or or methods. I'd rather suggest to leave
the and and or methods, but deprecate the add methods and offer them anew
as where methods. This results in very readable code.
criteria.where(AuthorPeer.ID, 1)
    .and(AuthorPeer.name,'Bloch');
(this is of course discussable as the and and the where methods have the
same semantics so one could also argue that one of them is enough)

As for reducing the length of the Criteria method, I'd propose a few things
to do on the new Criteria class
- move the Join and Criterion inner classes to full-blown classes of their
own
- remove all methods which take Strings as column namens.
- remove methods which came from the Hashtable interface, such as size(),
getXXX(), clear() etc

Doing this will result in a Criteria class which is 1739 lines long
(including the deprecated add methods), most of them javadoc, less than the
2000 lines considered good style. I believe this size is ok for a class
with such a lot functionality as the Criteria class.

I'd not bother for factory methods as the plan is to remove the old
util.criteria class anyway.

Is this ok for you ? If yes I'd check the code in in a few days to have a
better basis of discussion.

       Thomas

Greg Monroe wrote:

> Moving Critia to another package is good.  I never thought it was a
"util".
>
> +1
>
> Some thoughts on implementation:
>
> Should we move to a "factory" model that allows for both old and newto
exist?
> E.g. Criteria becomes an abstract class and to get on you need to call
> Criterial.newCriteria();  With a setting that allows for getting old or
new
> Versions of Criteria implimentations...
>
> but this might be too messy with all method stubs  in the abstract
> and possibly
> requiring class casting in places.
>
> Since we're talking about major changes, here are some thoughts on
> method names...
>
> First, I think we've overloaded "add" too much.  There are something like
15
> add methods and 15+ addXXX methods.  These relate to widely
> different parts of
> the statement.. like addJoin(), addAlias(), etc.  I think it would be
nice to
> refactor these to allow for easier identification and selection by using
the
> query area they apply to.  So instead of "add" how about "where" as the
base
> for WHERE clause section of a query.
>
> Here's some example pseudo code:
>
> /* SQL logical flags a.la SqlEnum used below */
> Public Class SqlLogical {
>   Public static final SqlLogical AND = new SqlLogical("AND");
>   Public static final SqlLogical OR = new SqlLogical("OR");
>   Public static final SqlLogical AND_NOT = new SqlLogica("AND NOT");
>   Public static final SqlLogical OR_NOT = new SqlLogica("OR NOT");
>
> /**
>  * Base condition method that all "convenience methods built on"
>  *
>  * @arg col Column object to use for comparison
>  * @arg value Value to compare with
>  * @arg comparison Type of comparison to make
>  * @arg logical Logical conditions to use with clause (null = ADD)
>  */
> Public Criteria where( Column col, Object value, SqlEnum comparison,
>                        SqlLogical logical );
>
> My preference for "convenience" where methods would be to just have
> type specific values and keep all the other options.  E.g.
>
> Public Criteria where( Column col, int value, SqlEnum comparison,
>                        Boolean useOr, Boolean useNot );
>
> This would cut way back on the number of methods in criteria by allowing
> a lot of "combination" methods to be eliminated.
>
> Probably the biggest complaint for this is the "readability" of it.
E.g.:
>
> c.add(USERS.COL_ID, 1, SqlEnum.GREATER_THAN )
>  .add(USERS.NAME, "admin%", SqlEnum.LIKE)
>  .addNotIn(USERS.RIGHTS, rightsArray);
>
> becomes
> c.where(USERS.COL_ID, 1, SqlEnum.GREATER_THAN, null )
>  .where(USERS.NAME, "%admin%", SqlEnum.LIKE, null ),
>  .where(USERS.RIGHTS, rightsArray, SqlEnum.IN, SqlLogical.AND_NOT);
>
> Personally, I like the simplicity of fewer methods to remember and the
> resulting easier editor "autoselection" entry.  But if we wanted to
> there could be more "where" convenience methods.
>
> Of course the criterion methods would be used to do nested conditions.
> And if you wanted the .and().or().andNot() readability, you could do
> the logicals using the Criterion classes.
>
> The same naming convention could be used for things like:
>
> addJoin() => join()
> add...OrderByColumn => orderBy(column, direction)
> addGroupBy => groupBy()
>
> and the like..
>
> Finally, if we don't / can't do a factory to switch syntaxes, then this
> naming conventions would mean the old methods could stay with a
deprecated
> tag and the new methods would "stand out" as separate.
>
> Just some thoughts on this.
>
> Greg
>
> -----Original Message-----
> From: Thomas Vandahl [mailto:tv@apache.org]
> Sent: Monday, December 12, 2011 10:59 AM
> To: Apache Torque Developers List
> Subject: Re: Change semantics of Criteria.add() and Criteria.or()
>
> Am 12.12.2011 15:07, schrieb Thomas Fox:
> > 1) deprecate or remove the existing class and create a new one.
Variants:
> > 1a) move the new class to a new package
>
> +1
> While the danger of an automatic "Organize imports" still exists, it is
> probably the best solution.
>
> > 2) deprecate the existing methods and create new ones. We could
> > deprecate/remove the add methods and tell the user to use the and
methods.
> > But I do not have a good new name for the "or" methods.
>
> Its not really a good solution but what about AND() and OR().
>
> > 3) Because of TORQUE-168, new methods have been added which use a
Column
> > object as column name. Until now, Strings have been used as column
name. We
> > could deprecate/remove the methods which use String values. This means
that
> > people which do NOT use the constants from the Peer classes have more
work.
>
> +2
> Yeah, but I'd prefer more work over the possibility of wrong code.
Anytime.
>
> Bye, Thomas.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: torque-dev-unsubscribe@db.apache.org
> For additional commands, e-mail: torque-dev-help@db.apache.org
>
> DukeCE Privacy Statement:
> Please be advised that this e-mail and any files transmitted with
> it are confidential communication or may otherwise be privileged or
> confidential and are intended solely for the individual or entity
> to whom they are addressed. If you are not the intended recipient
> you may not rely on the contents of this email or any attachments,
> and we ask that you please not read, copy or retransmit this
> communication, but reply to the sender and destroy the email, its
> contents, and all copies thereof immediately. Any unauthorized
> dissemination, distribution or copying of this communication is
> strictly prohibited.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: torque-dev-unsubscribe@db.apache.org
> For additional commands, e-mail: torque-dev-help@db.apache.org
>


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

DukeCE Privacy Statement:
Please be advised that this e-mail and any files transmitted with
it are confidential communication or may otherwise be privileged or
confidential and are intended solely for the individual or entity
to whom they are addressed. If you are not the intended recipient
you may not rely on the contents of this email or any attachments,
and we ask that you please not read, copy or retransmit this
communication, but reply to the sender and destroy the email, its
contents, and all copies thereof immediately. Any unauthorized
dissemination, distribution or copying of this communication is
strictly prohibited.

---------------------------------------------------------------------
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