Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Polymorphic CaseBuilder.Initial.then #702

Closed
dmitrygusev opened this issue Apr 8, 2014 · 7 comments · Fixed by #703
Closed

Polymorphic CaseBuilder.Initial.then #702

dmitrygusev opened this issue Apr 8, 2014 · 7 comments · Fixed by #703
Milestone

Comments

@dmitrygusev
Copy link

Using variable of general type (like Expression<T>) in new CaseBuilder().when().then(HERE) will always call version of then with this signature:

public <A> Cases<A, SimpleExpression<A>> then(Expression<A> expr)

which means you will get SimpleOperation in response and you can't use it in "order by" clauses and other methods that require concrete type because it can't be cast from SimpleOperation.

This could be solved by providing polymorphic version of "then" that will decide which overload to invoke at runtime based on type of expression, here's an example of how this method may look like:

class CaseBuilder {
    // ...
    class Initial {
        // ...
        public <A,Q extends Expression<A>> Cases<A,Q> then2(Expression<A> expr) {
            if (expr instanceof BooleanExpression) {
                return (Cases<A, Q>) then((BooleanExpression) expr);
            }
            if (expr instanceof NumberExpression) {
                return then((NumberExpression) expr);
            }
            if (expr instanceof DateExpression) {
                return then((DateExpression) expr);
            }
            if (expr instanceof DateTimeExpression) {
                return then((DateTimeExpression) expr);
            }
            if (expr instanceof StringExpression) {
                return (Cases<A, Q>) then((StringExpression) expr);
            }
            if (expr instanceof TimeExpression) {
                return then((TimeExpression) expr);
            }
            return (Cases<A, Q>) then(expr);
        }
@timowest
Copy link
Member

timowest commented Apr 8, 2014

@dmitrygusev Could you comment if this change is sufficient #703 ?

The then2 suggestion has the problem that you will need to provide the Q type argument to the call since it can't be derived from the arguments.

@dmitrygusev
Copy link
Author

No, this doesn't help much. Here's what I did: I have a method that builds case expression based on some predefined condition set, but "then"-part of that case expression may differ:

    interface ReminderExpression<T> {
        Expression<T> then(QReminder reminder);
    }

    QReminder reminder1 = new QReminder("reminder1");
    QReminder reminder2 = new QReminder("reminder2");
    QReminder reminder3 = new QReminder("reminder3");

    <T> Expression<T> getReminderCase(ReminderExpression<T> thenExpr) {
        return new BetterCaseBuilder()
            .when(CONDITION1)
            .then2(thenExpr.then(reminder1))
            .when(CONDITION2)
            .then(thenExpr.then(reminder2))
            .otherwise(thenExpr.then(reminder3));
    }

Then I have few another methods that use this method to build actual expression, like:

     DateExpression<Date> getReminderDateCase() {
        return (DateExpression<Date>) getReminderCase(new ReminderExpression<Date>() {
            @Override public Expression<Date> then(QReminder reminder) {
                return dateAdd(invoice.dueOn, reminder.offsetInDays);
            }
        });
    }

As you mentioned, then2 implementation can't derive Q type from arguments, that's why I have to cast result to desired type, and it works fine for me here.

I tried solution from #703 and this example worked, but I have another method that builds actual expression and returns NumberExpression from NumberPath, and it doesn't compile:

    interface ReminderExpression<T extends Comparable<T>> {
        ComparableExpression<T> then(QReminder reminder);
    }

    NumberExpression<Byte> getReminderOrderNumberCase() {
        return (NumberExpression<Byte>) getReminderCase(new ReminderExpression<Byte>() {
            @Override public ComparableExpression<Byte> then(QReminder reminder) {
                // COMPILE ERROR:
                // Type mismatch: cannot convert
                // from NumberPath<Byte> to ComparableExpression<Byte>
                return reminder.orderNumber;
            }
        });
    }

@timowest
Copy link
Member

timowest commented Apr 9, 2014

@dmitrygusev I used now your then2 code in the pull request. Could you take a look?

It should work with a single method, since all the other Expression types are subclasses of SimpleExpression.

@dmitrygusev
Copy link
Author

yep, works good now, thanks

@timowest
Copy link
Member

timowest commented Apr 9, 2014

Ok, good. Are these replacements also ok for you?

-  public Cases<java.sql.Date, DateExpression<java.sql.Date>> thenDate(java.sql.Date date)
+  public Cases<java.sql.Date, DateExpression<java.sql.Date>> then(java.sql.Date date) 
-  public Cases<Timestamp, DateTimeExpression<Timestamp>> thenDateTime(Timestamp ts)
+  public Cases<Timestamp, DateTimeExpression<Timestamp>> then(Timestamp ts)
-  public Cases<java.util.Date, DateTimeExpression<java.util.Date>> thenDateTime(java.util.Date date)
+  public Cases<java.util.Date, DateTimeExpression<java.util.Date>> then(java.util.Date date)

@dmitrygusev
Copy link
Author

For me, yes. All my tests passed with changes from #703.

@timowest timowest reopened this Apr 9, 2014
@timowest timowest added the fixed label Apr 9, 2014
@timowest timowest modified the milestone: 3.3.3 Apr 13, 2014
@timowest timowest modified the milestone: 3.3.3 Apr 30, 2014
@timowest
Copy link
Member

timowest commented May 2, 2014

Released in 3.3.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants