[jira] [Comment Edited] (OFBIZ-10593) ‘EntityConditionVisitor’ is a confused visitor pattern

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-10593?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16642276#comment-16642276 ]

Mathieu Lirzin edited comment on OFBIZ-10593 at 10/8/18 6:28 PM:
-----------------------------------------------------------------

Hello,

So after looking at  the hierarchy it appears that the extra arguments are not desirable.  In fact there is a design issue where {{EntityOperator}} and {{EntityConditionValue}} inherits from {{EntityConditionBase}} even if they can't be considered as proper "entity conditions".  This violates the principle of prefering delegation over inheritance.  This should be fixed later by some refactoring.

In the meantime, I have updated  [^OFBIZ-10593_Fixed-Implement-EntityConditionVisitor-properly.patch]  to only consider subclasses of the {{EntityCondition}} abstract class. Here is the {{EntityConditionVisitor}} interface I consider acceptable to be applied and which can be used in OFBIZ-10579:

{code:java}
public interface EntityConditionVisitor {
    void visit(EntityConditionFunction cond);
    <T extends EntityCondition> void visit(EntityConditionList<T> cond);
    void visit(EntityFieldMap fieldMap);
    void visit(EntityDateFilterCondition condition);
    void visit(EntityExpr expr);
    void visit(EntityWhereString condition);
}
{code}



was (Author: mthl):
Hello,

So after looking at  the hierarchy it appears that the extra arguments are not desirable.  In fact there is a design issue where {{EntityOperator}} and {{EntityConditionValue}} inherits from {{EntityConditionBase}} even if they can't be considered as proper "entity conditions".  This violates the principle of prefering delegation over inheritance.  This should be fixed later by some refactoring.

In the meantime, I have updated the visitor to only consider subclasses of the {{EntityCondition}} abstract class. Here is the {{EntityConditionVisitor}} interface I consider acceptable to be applied and which can be used in OFBIZ-10579.

{code:java}
public interface EntityConditionVisitor {
    void visit(EntityConditionFunction cond);
    <T extends EntityCondition> void visit(EntityConditionList<T> cond);
    void visit(EntityFieldMap fieldMap);
    void visit(EntityDateFilterCondition condition);
    void visit(EntityExpr expr);
    void visit(EntityWhereString condition);
}
{code}

> ‘EntityConditionVisitor’ is a confused visitor pattern
> ------------------------------------------------------
>
>                 Key: OFBIZ-10593
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-10593
>             Project: OFBiz
>          Issue Type: Improvement
>    Affects Versions: Trunk
>            Reporter: Mathieu Lirzin
>            Assignee: Mathieu Lirzin
>            Priority: Major
>         Attachments: OFBIZ-10593_Fixed-Implement-EntityConditionVisitor-properly.patch
>
>
> {{EntityConditionVisitor}} interface is supposed to implement the visitor pattern which supposes a set of classes meant to be visited using an {{accept}} method and a visitor with multiple {{visit}} method overloads (one for each visited class).
> Currently {{EntityConditionVisitor}} contains both {{accept}} and {{visit}} methods which make *no sense*
> {code:java}
> public interface EntityConditionVisitor {
>     <T> void visit(T obj);
>     <T> void accept(T obj);
>     void acceptObject(Object obj);
>     void acceptEntityCondition(EntityCondition condition);
>     <T extends EntityCondition> void acceptEntityJoinOperator(EntityJoinOperator op, List<T> conditions);
>     <L,R,T> void acceptEntityOperator(EntityOperator<L, R, T> op, L lhs, R rhs);
>     <L,R> void acceptEntityComparisonOperator(EntityComparisonOperator<L, R> op, L lhs, R rhs);
>     void acceptEntityConditionValue(EntityConditionValue value);
>     void acceptEntityFieldValue(EntityFieldValue value);
>     void acceptEntityExpr(EntityExpr expr);
>     <T extends EntityCondition> void acceptEntityConditionList(EntityConditionList<T> list);
>     void acceptEntityFieldMap(EntityFieldMap fieldMap);
>     void acceptEntityConditionFunction(EntityConditionFunction func, EntityCondition nested);
>     <T extends Comparable<?>> void acceptEntityFunction(EntityFunction<T> func);
>     void acceptEntityWhereString(EntityWhereString condition);
>     void acceptEntityDateFilterCondition(EntityDateFilterCondition condition);
> }
> {code}
> this confusion is visible in the {{EntityCondition}} which has both a {{visit}} and an {{accept}} method, even if it is only supposed to accept a visitor.

--
This message was sent by Atlassian JIRA
(v7.6.3#76005)