[jira] [Commented] (OFBIZ-10593) ‘EntityConditionVisitor’ is a confused visitor pattern
[
https://issues.apache.org/jira/browse/OFBIZ-10593?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16648813#comment-16648813
]
Mathieu Lirzin commented on OFBIZ-10593:
Hello Gil,
I fully agree on creating an issue for isolating {{EntityOperator}} and
{{EntityConditionValue}} from {{EntityConditionBase}}. Please go ahead.
Thanks for the review!
> ‘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: Gil Portenseigne
>Priority: Major
> Attachments:
> OFBIZ-10593_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 {
> void visit(T obj);
> void accept(T obj);
> void acceptObject(Object obj);
> void acceptEntityCondition(EntityCondition condition);
> void
> acceptEntityJoinOperator(EntityJoinOperator op, List conditions);
> void acceptEntityOperator(EntityOperator op, L lhs, R
> rhs);
> void acceptEntityComparisonOperator(EntityComparisonOperator
> op, L lhs, R rhs);
> void acceptEntityConditionValue(EntityConditionValue value);
> void acceptEntityFieldValue(EntityFieldValue value);
> void acceptEntityExpr(EntityExpr expr);
> void
> acceptEntityConditionList(EntityConditionList list);
> void acceptEntityFieldMap(EntityFieldMap fieldMap);
> void acceptEntityConditionFunction(EntityConditionFunction func,
> EntityCondition nested);
> > void acceptEntityFunction(EntityFunction
> 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)
[jira] [Commented] (OFBIZ-10593) ‘EntityConditionVisitor’ is a confused visitor pattern
[
https://issues.apache.org/jira/browse/OFBIZ-10593?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16647939#comment-16647939
]
Gil Portenseigne commented on OFBIZ-10593:
--
Hello [~mthl], i did review your final patch to refactor to a proper visitor
pattern.
Great job ! I only found one typo into the well detailed Javadoc ;).
This implementation is far more understandable, and will hopefully ease
OFBIZ-10579 improvement by [~suraj.khurana].
If nobody already did yet, we need to create a Jira to isolate EntityOperator
and EntityConditionValue from EntityConditionBase. I'll check it out.
Thanks !
> ‘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_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 {
> void visit(T obj);
> void accept(T obj);
> void acceptObject(Object obj);
> void acceptEntityCondition(EntityCondition condition);
> void
> acceptEntityJoinOperator(EntityJoinOperator op, List conditions);
> void acceptEntityOperator(EntityOperator op, L lhs, R
> rhs);
> void acceptEntityComparisonOperator(EntityComparisonOperator
> op, L lhs, R rhs);
> void acceptEntityConditionValue(EntityConditionValue value);
> void acceptEntityFieldValue(EntityFieldValue value);
> void acceptEntityExpr(EntityExpr expr);
> void
> acceptEntityConditionList(EntityConditionList list);
> void acceptEntityFieldMap(EntityFieldMap fieldMap);
> void acceptEntityConditionFunction(EntityConditionFunction func,
> EntityCondition nested);
> > void acceptEntityFunction(EntityFunction
> 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)
[jira] [Commented] (OFBIZ-10593) ‘EntityConditionVisitor’ is a confused visitor pattern
[
https://issues.apache.org/jira/browse/OFBIZ-10593?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16647696#comment-16647696
]
Mathieu Lirzin commented on OFBIZ-10593:
Hello Jacques,
I have updated [^OFBIZ-10593_Implement-EntityConditionVisitor-properly.patch]
with some javadoc containing usage examples which are checked by corresponding
unit tests.
> ‘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_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 {
> void visit(T obj);
> void accept(T obj);
> void acceptObject(Object obj);
> void acceptEntityCondition(EntityCondition condition);
> void
> acceptEntityJoinOperator(EntityJoinOperator op, List conditions);
> void acceptEntityOperator(EntityOperator op, L lhs, R
> rhs);
> void acceptEntityComparisonOperator(EntityComparisonOperator
> op, L lhs, R rhs);
> void acceptEntityConditionValue(EntityConditionValue value);
> void acceptEntityFieldValue(EntityFieldValue value);
> void acceptEntityExpr(EntityExpr expr);
> void
> acceptEntityConditionList(EntityConditionList list);
> void acceptEntityFieldMap(EntityFieldMap fieldMap);
> void acceptEntityConditionFunction(EntityConditionFunction func,
> EntityCondition nested);
> > void acceptEntityFunction(EntityFunction
> 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)
[jira] [Commented] (OFBIZ-10593) ‘EntityConditionVisitor’ is a confused visitor pattern
[
https://issues.apache.org/jira/browse/OFBIZ-10593?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16644706#comment-16644706
]
Jacques Le Roux commented on OFBIZ-10593:
-
Hi Mathieu,
After a close review this sounds good to me, few UI tests are OK too.
> ‘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 {
> void visit(T obj);
> void accept(T obj);
> void acceptObject(Object obj);
> void acceptEntityCondition(EntityCondition condition);
> void
> acceptEntityJoinOperator(EntityJoinOperator op, List conditions);
> void acceptEntityOperator(EntityOperator op, L lhs, R
> rhs);
> void acceptEntityComparisonOperator(EntityComparisonOperator
> op, L lhs, R rhs);
> void acceptEntityConditionValue(EntityConditionValue value);
> void acceptEntityFieldValue(EntityFieldValue value);
> void acceptEntityExpr(EntityExpr expr);
> void
> acceptEntityConditionList(EntityConditionList list);
> void acceptEntityFieldMap(EntityFieldMap fieldMap);
> void acceptEntityConditionFunction(EntityConditionFunction func,
> EntityCondition nested);
> > void acceptEntityFunction(EntityFunction
> 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)
[jira] [Commented] (OFBIZ-10593) ‘EntityConditionVisitor’ is a confused visitor pattern
[
https://issues.apache.org/jira/browse/OFBIZ-10593?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16642276#comment-16642276
]
Mathieu Lirzin commented on OFBIZ-10593:
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);
void visit(EntityConditionList 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 {
> void visit(T obj);
> void accept(T obj);
> void acceptObject(Object obj);
> void acceptEntityCondition(EntityCondition condition);
> void
> acceptEntityJoinOperator(EntityJoinOperator op, List conditions);
> void acceptEntityOperator(EntityOperator op, L lhs, R
> rhs);
> void acceptEntityComparisonOperator(EntityComparisonOperator
> op, L lhs, R rhs);
> void acceptEntityConditionValue(EntityConditionValue value);
> void acceptEntityFieldValue(EntityFieldValue value);
> void acceptEntityExpr(EntityExpr expr);
> void
> acceptEntityConditionList(EntityConditionList list);
> void acceptEntityFieldMap(EntityFieldMap fieldMap);
> void acceptEntityConditionFunction(EntityConditionFunction func,
> EntityCondition nested);
> > void acceptEntityFunction(EntityFunction
> 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)
[jira] [Commented] (OFBIZ-10593) ‘EntityConditionVisitor’ is a confused visitor pattern
[
https://issues.apache.org/jira/browse/OFBIZ-10593?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16638374#comment-16638374
]
Mathieu Lirzin commented on OFBIZ-10593:
I have uploaded [^OFBIZ-10593_Implement-EntityConditionVisitor-properly.patch]
which is a WIP patch trying to improve the situation the following
{{EntityConditionVisitor}} interface:
{code:java}
public interface EntityConditionVisitor {
void visit(EntityJoinOperator op, List
conditions);
void
visit(EntityJoinOperator op, L lhs, R rhs);
void visit(EntityOperator op, L lhs, R rhs);
void visit(EntityComparisonOperator op, L lhs, R rhs);
void visit(EntityConditionValue.ConstantNumberValue value);
void visit(EntityFieldValue value);
void visit(EntityExpr expr);
void visit(EntityConditionList list);
void visit(EntityFieldMap fieldMap);
void visit(EntityConditionFunction func, EntityCondition nested);
> void visit(EntityFunction func);
void visit(EntityWhereString condition);
void visit(EntityDateFilterCondition condition);
}
{code}
I am still puzzled by the presence of {{visit}} methods with multiple
arguments, so I will work with [~gil portenseigne] on understanding the
functional logic of the {{EntityCondition}} class hierarchy to check if that's
weirdness make sense.
> ‘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_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 {
> void visit(T obj);
> void accept(T obj);
> void acceptObject(Object obj);
> void acceptEntityCondition(EntityCondition condition);
> void
> acceptEntityJoinOperator(EntityJoinOperator op, List conditions);
> void acceptEntityOperator(EntityOperator op, L lhs, R
> rhs);
> void acceptEntityComparisonOperator(EntityComparisonOperator
> op, L lhs, R rhs);
> void acceptEntityConditionValue(EntityConditionValue value);
> void acceptEntityFieldValue(EntityFieldValue value);
> void acceptEntityExpr(EntityExpr expr);
> void
> acceptEntityConditionList(EntityConditionList list);
> void acceptEntityFieldMap(EntityFieldMap fieldMap);
> void acceptEntityConditionFunction(EntityConditionFunction func,
> EntityCondition nested);
> > void acceptEntityFunction(EntityFunction
> 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)
