Details

    • Type: Sub-task Sub-task
    • Status: Open Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: main
    • Labels:
      None

      Description

      Part of this request includes:

      • PrePostFilterVisitor.java
      • FindProperty.java
      • FindType.java
      • GetParent.java
      • Replace.java

      This one is just a general feedback on visitor names; I know this is no fault of your own....

      There is a much loved/hated class called PostPreProcessFilterSplittingVisitor ... your new abstract base class PrePostFilterVisitor is close enough in name that I am always going to be confused.

      I am going to guess that you use this base class for more than just the search (and replace) examples provided? If not I would love to call this AbstractSearchFilterVisitor; and then the other classes would cleanly follow from it...

      Other than that I gotta say that this preVisit( object, data ) and postVisit( object, data ) is exactly what the visitor pattern is trying to avoid (ie the implementations are performing instance of checks in here to try and recover the type information you just threw out.

      I am going to think a bit more about what you have here and see if I can help; it looks like you were trying to duck out of implementing filter traversal while still being able to subclass quickly and "process" the data structure.

      This is supposed to be possible with DefaultFilterVisitor; it may be a design or documentation flaw if you did not find this usable.

        Activity

        Hide
        Jody Garnett added a comment -
        Okay had a slightly better look at how PrePostFilterVisitor works; it is interesting it is used to keep a little bit of context as what is trying to be accomplished here is doing queries against the tree structure. We have something similar going on with a stack when copying features; but it is not quite as cool.

        Will need to think about this more; I was able to accomplish the functionality for Filters using brute force and ignorance (with what should be a little better performance).
        Show
        Jody Garnett added a comment - Okay had a slightly better look at how PrePostFilterVisitor works; it is interesting it is used to keep a little bit of context as what is trying to be accomplished here is doing queries against the tree structure. We have something similar going on with a stack when copying features; but it is not quite as cool. Will need to think about this more; I was able to accomplish the functionality for Filters using brute force and ignorance (with what should be a little better performance).
        Hide
        Eric Sword added a comment -
        Oops - I should have posted this comment here rather than under the original issue. Sorry for the duplicate:

        AbstractSearchFilterVisitor makes sense because the two Filters where the capability is most useful are the two I originally wrote - FindType and FindProperty. Those do want to check every type of Filter that comes through. The only reason for the instanceof check in FindProperty is because the parent classes implement ExpressionVisitor as well. As for the other two (GetParent and Replace)... mea culpa. I have to think back to when I wrote them a bit over a year ago and guess that I just got in the habit of extending PrePostFilterVisitor rather than thinking about what fit best. Having them extend DefaultFilterVisitor and then override And/Or/Not is cleaner.

        Or, generalizing the use case (which I tend to do too much), we could have "grouping visits" that are called for certain types of related items. e.g. preVisit/postVisit[All, Filter, Expression, Logic, BinaryComparison, BinarySpatial, etc]. With these, FindType would override postVisitAll, FindPropertyVistor would override postVisitFilter and GetParent and Replace would override postVisitLogic. (Pardon the names.) I am just tossing around ideas to avoid having many one line functions that call into some other function to do the guts of their work as will happen with GetParent and Replace when overriding the visit methods for And/Or and maybe Not. This isn't an urgent problem in any sense since only these classes do it at the moment. It is probably easier to just wait and see if additional classes could benefit from having a set of "functional grouping" visit calls.
        Show
        Eric Sword added a comment - Oops - I should have posted this comment here rather than under the original issue. Sorry for the duplicate: AbstractSearchFilterVisitor makes sense because the two Filters where the capability is most useful are the two I originally wrote - FindType and FindProperty. Those do want to check every type of Filter that comes through. The only reason for the instanceof check in FindProperty is because the parent classes implement ExpressionVisitor as well. As for the other two (GetParent and Replace)... mea culpa. I have to think back to when I wrote them a bit over a year ago and guess that I just got in the habit of extending PrePostFilterVisitor rather than thinking about what fit best. Having them extend DefaultFilterVisitor and then override And/Or/Not is cleaner. Or, generalizing the use case (which I tend to do too much), we could have "grouping visits" that are called for certain types of related items. e.g. preVisit/postVisit[All, Filter, Expression, Logic, BinaryComparison, BinarySpatial, etc]. With these, FindType would override postVisitAll, FindPropertyVistor would override postVisitFilter and GetParent and Replace would override postVisitLogic. (Pardon the names.) I am just tossing around ideas to avoid having many one line functions that call into some other function to do the guts of their work as will happen with GetParent and Replace when overriding the visit methods for And/Or and maybe Not. This isn't an urgent problem in any sense since only these classes do it at the moment. It is probably easier to just wait and see if additional classes could benefit from having a set of "functional grouping" visit calls.
        Hide
        Eric Sword added a comment -
        Jody added a comment to GEOT-3508 which I am copying here:

        You are onto something with how you use your PrepPostFilterVisitors; I am just not sure what :-)

        One common thing is that you are trying to look around (parent and child relationships) as you process a filter ...

        1- we already have one good way to "look down" at the children (pass control to another visitor which will go down and have a look)
        2- but there is no good way to look up at parent

        So #2 is our real trouble here; I have an idea ... when we are writing the duplicate visitor we have to keep a stack around so we don't go insane with recursion.

        If we make a visitor that keeps a stack around then in your normal visitor methods you could ask the stack questions about your parents.

        Would that work for you?
        Show
        Eric Sword added a comment - Jody added a comment to GEOT-3508 which I am copying here: You are onto something with how you use your PrepPostFilterVisitors; I am just not sure what :-) One common thing is that you are trying to look around (parent and child relationships) as you process a filter ... 1- we already have one good way to "look down" at the children (pass control to another visitor which will go down and have a look) 2- but there is no good way to look up at parent So #2 is our real trouble here; I have an idea ... when we are writing the duplicate visitor we have to keep a stack around so we don't go insane with recursion. If we make a visitor that keeps a stack around then in your normal visitor methods you could ask the stack questions about your parents. Would that work for you?
        Hide
        Eric Sword added a comment -
        That sounds fine, though are you considering putting that in some general filter capability or just in duplicate visitor?
        Show
        Eric Sword added a comment - That sounds fine, though are you considering putting that in some general filter capability or just in duplicate visitor?
        Hide
        Jody Garnett added a comment -
        Well I figured I would attach a new class to this bug report and you could tell me if it is useful? I would not like to confuse duplicate visitor with an additional responsibility.
        Show
        Jody Garnett added a comment - Well I figured I would attach a new class to this bug report and you could tell me if it is useful? I would not like to confuse duplicate visitor with an additional responsibility.

          People

          • Assignee:
            Jody Garnett
            Reporter:
            Jody Garnett
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated: