Return to Question

• Hard-coded literals reporting error. Those need to be in the resource files and will be moved later. I need more thoughts on what I need to collect before I will refactor those.
• Currently, the visitor stops as soon as it encounters a invalid context. It will be revised to allow collection of all possible invalid contexts, which is why it's a IList rather than a single instance.

• To avoid storing a state in the visitor, I opted to create a class ExtractMethodValidationResult which importantly has 2 separate lists. ValidContexts gives me all contexts (of one type) that are valid for a user's selection whereas InvalidContexts gives me any other contexts (regardless of type) where there might be a problem. I then pass that into the generic visitor.In a typical sample, a generic visitor will take a IEnumerable of some certain contexts and return that. This is quite different as we take a class and return the results. As a result, the process will create many new instances of the class which is then aggregated. Are there any potential pitfalls with the approach?

• I'm not exactly thrilled with the high amount of duplicative code, notably with those that collects invalid contexts. Can this be done better?

• I'm a little fuzzy whether I need to return base.XXX when visiting contexts that are considered invalid. In case when we visit a invalid context that isn't a part of the selection, we execute the base.XXX

• I'm a bit stumped on where the responsibility of the visitor starts and stops. Right now it returns a raw set of contexts which requires additional processing outside the visit. I'm considering whether I should override the Visit method to something similar to this:

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> Visit(IParseTree tree)
        {
            var result = base.Visit(tree);
            return PostProcessing(result);
        }
  

• Hard-coded literals reporting error. Those need to be in the resource files and will be moved later. I need more thoughts on what I need to collect before I will refactor those.
• Currently, the visitor stops as soon as it encounters an invalid context. It will be revised to allow collection of all possible invalid contexts, which is why it's an IList rather than a single instance.

• To avoid storing a state in the visitor, I opted to create a class ExtractMethodValidationResult which importantly has 2 separate lists. ValidContexts gives me all contexts (of one type) that are valid for a user's selection whereas InvalidContexts gives me any other contexts (regardless of type) where there might be a problem. I then pass that into the generic visitor.In a typical sample, a generic visitor will take an IEnumerable of some certain contexts and return that. This is quite different as we take a class and return the results. As a result, the process will create many new instances of the class which is then aggregated. Are there any potential pitfalls with the approach?

• I'm not exactly thrilled with the high amount of duplicative code, notably with those that collects invalid contexts. Can this be done better?

• I'm a little fuzzy whether I need to return base.XXX when visiting contexts that are considered invalid. In case when we visit a invalid context that isn't a part of the selection, we execute the base.XXX

• I'm a bit stumped on where the responsibility of the visitor starts and stops. Right now it returns a raw set of contexts which requires additional processing outside the visit. I'm considering whether I should override the Visit method to something similar to this:

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> Visit(IParseTree tree)
        {
            var result = base.Visit(tree);
            return PostProcessing(result);
        }
  

• To avoid storing a state in the visitor, I opted to create a class ExtractMethodValidationResult which importantly has 2 separate lists. ValidContexts gives me all contexts (of one type) that are valid for a user's selection whereas InvalidContexts gives me any other contexts (regardless of type) where there might be a problem. I then pass that into the generic visitor.In a typical sample, a generic visitor will take a IEnumerable of some certain contexts and return that. This is quite different as we take a class and return the results. As a result, the process will create many new instances of the class which is then aggregated. Are there any potential pitfalls with the approach?
• I'm not exactly thrilled with the high amount of duplicative code, notably with those that collects invalid contexts. Can this be done better?
• I'm a little fuzzy whether I need to return base.XXX when visiting contexts that are considered invalid. In case when we visit a invalid context that isn't a part of the selection, we execute the base.XXX

• To avoid storing a state in the visitor, I opted to create a class ExtractMethodValidationResult which importantly has 2 separate lists. ValidContexts gives me all contexts (of one type) that are valid for a user's selection whereas InvalidContexts gives me any other contexts (regardless of type) where there might be a problem. I then pass that into the generic visitor.In a typical sample, a generic visitor will take a IEnumerable of some certain contexts and return that. This is quite different as we take a class and return the results. As a result, the process will create many new instances of the class which is then aggregated. Are there any potential pitfalls with the approach?

• I'm not exactly thrilled with the high amount of duplicative code, notably with those that collects invalid contexts. Can this be done better?

• I'm a little fuzzy whether I need to return base.XXX when visiting contexts that are considered invalid. In case when we visit a invalid context that isn't a part of the selection, we execute the base.XXX

• I'm a bit stumped on where the responsibility of the visitor starts and stops. Right now it returns a raw set of contexts which requires additional processing outside the visit. I'm considering whether I should override the Visit method to something similar to this:

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> Visit(IParseTree tree)
        {
            var result = base.Visit(tree);
            return PostProcessing(result);
        }
  

The advantage would be that the visitor now returns a result that needs no more additional processing but it also takes on the responsibility of analyzing the returned contexts from the base.Visit. Have I then violated the SRP here? Thoughts?

private class ExtractMethodValidatorVisitor : VBAParserBaseVisitor<ExtractMethodValidationResult<VBAParser.BlockStmtContext>

    {
        private readonly QualifiedSelection _qualifiedSelection;

        public ExtractMethodValidatorVisitor(QualifiedSelection qualifiedSelection)
        {
            _qualifiedSelection = qualifiedSelection;
        }
        
        protected override ExtractMethodValidationResult<VBAParser.BlockStmtContext> DefaultResult => new ExtractMethodValidationResult<VBAParser.BlockStmtContext>();
        
        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitBlockStmt([NotNull] VBAParser.BlockStmtContext context)
        {
            var children = base.VisitBlockStmt(context);
            if (children.InvalidContexts.Count == 0)
            {
                children.ValidContexts.Add(context);
            }

            return children;
        }

        protected override ExtractMethodValidationResult<VBAParser.BlockStmtContext> AggregateResult(ExtractMethodValidationResult<VBAParser.BlockStmtContext> aggregate, ExtractMethodValidationResult<VBAParser.BlockStmtContext> nextResult)
        {
            if (aggregate.InvalidContexts.Count == 0)
            {
                if (nextResult != null)
                {
                    aggregate.ValidContexts = aggregate.ValidContexts.Concat(nextResult.ValidContexts).ToList();
                    aggregate.InvalidContexts = aggregate.InvalidContexts.Concat(nextResult.InvalidContexts).ToList();
                }
            }
            
            return aggregate;
        }

        protected override bool ShouldVisitNextChild(IRuleNode node, ExtractMethodValidationResult<VBAParser.BlockStmtContext> currentResult)
        {
            // Don't visit any more children if we have any invalid contexts
            return (currentResult.InvalidContexts.Count == 0);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitErrorStmt([NotNull] VBAParser.ErrorStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains an Error statement.");
            }

            return base.VisitErrorStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitEndStmt([NotNull] VBAParser.EndStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains an End statement.");
            }

            return base.VisitEndStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitExitStmt([NotNull] VBAParser.ExitStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains an Exit statement");
            }

            return base.VisitExitStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitGoSubStmt([NotNull] VBAParser.GoSubStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a GoSub statement");
            }

            return base.VisitGoSubStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitGoToStmt([NotNull] VBAParser.GoToStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a GoTo statement");
            }

            return base.VisitGoToStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitOnErrorStmt([NotNull] VBAParser.OnErrorStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a On Error statement");
            }

            return base.VisitOnErrorStmt(context);
        }
        
        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitIdentifierStatementLabel([NotNull] VBAParser.IdentifierStatementLabelContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a Line Label statement");
            }

            return base.VisitIdentifierStatementLabel(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitCombinedLabels([NotNull] VBAParser.CombinedLabelsContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a Line Label statement");
            }

            return base.VisitCombinedLabels(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitOnGoSubStmt([NotNull] VBAParser.OnGoSubStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a On ... GoSub statement");
            }

            return base.VisitOnGoSubStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitOnGoToStmt([NotNull] VBAParser.OnGoToStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a On ... GoTo statement");
            }

            return base.VisitOnGoToStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitResumeStmt([NotNull] VBAParser.ResumeStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a Resume statement");
            }

            return base.VisitResumeStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitReturnStmt([NotNull] VBAParser.ReturnStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a Return statement");
            }

            return base.VisitReturnStmt(context);
        }
    }

private class ExtractMethodValidatorVisitor : VBAParserBaseVisitor<ExtractMethodValidationResult<VBAParser.BlockStmtContext>>

    {
        private readonly QualifiedSelection _qualifiedSelection;

        public ExtractMethodValidatorVisitor(QualifiedSelection qualifiedSelection)
        {
            _qualifiedSelection = qualifiedSelection;
        }
        
        protected override ExtractMethodValidationResult<VBAParser.BlockStmtContext> DefaultResult => new ExtractMethodValidationResult<VBAParser.BlockStmtContext>();
        
        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitBlockStmt([NotNull] VBAParser.BlockStmtContext context)
        {
            var children = base.VisitBlockStmt(context);
            if (children.InvalidContexts.Count == 0)
            {
                children.ValidContexts.Add(context);
            }

            return children;
        }

        protected override ExtractMethodValidationResult<VBAParser.BlockStmtContext> AggregateResult(ExtractMethodValidationResult<VBAParser.BlockStmtContext> aggregate, ExtractMethodValidationResult<VBAParser.BlockStmtContext> nextResult)
        {
            if (aggregate.InvalidContexts.Count == 0)
            {
                if (nextResult != null)
                {
                    aggregate.ValidContexts = aggregate.ValidContexts.Concat(nextResult.ValidContexts).ToList();
                    aggregate.InvalidContexts = aggregate.InvalidContexts.Concat(nextResult.InvalidContexts).ToList();
                }
            }
            
            return aggregate;
        }

        protected override bool ShouldVisitNextChild(IRuleNode node, ExtractMethodValidationResult<VBAParser.BlockStmtContext> currentResult)
        {
            // Don't visit any more children if we have any invalid contexts
            return (currentResult.InvalidContexts.Count == 0);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitErrorStmt([NotNull] VBAParser.ErrorStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains an Error statement.");
            }

            return base.VisitErrorStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitEndStmt([NotNull] VBAParser.EndStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains an End statement.");
            }

            return base.VisitEndStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitExitStmt([NotNull] VBAParser.ExitStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains an Exit statement");
            }

            return base.VisitExitStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitGoSubStmt([NotNull] VBAParser.GoSubStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a GoSub statement");
            }

            return base.VisitGoSubStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitGoToStmt([NotNull] VBAParser.GoToStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a GoTo statement");
            }

            return base.VisitGoToStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitOnErrorStmt([NotNull] VBAParser.OnErrorStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a On Error statement");
            }

            return base.VisitOnErrorStmt(context);
        }
        
        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitIdentifierStatementLabel([NotNull] VBAParser.IdentifierStatementLabelContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a Line Label statement");
            }

            return base.VisitIdentifierStatementLabel(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitCombinedLabels([NotNull] VBAParser.CombinedLabelsContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a Line Label statement");
            }

            return base.VisitCombinedLabels(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitOnGoSubStmt([NotNull] VBAParser.OnGoSubStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a On ... GoSub statement");
            }

            return base.VisitOnGoSubStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitOnGoToStmt([NotNull] VBAParser.OnGoToStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a On ... GoTo statement");
            }

            return base.VisitOnGoToStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitResumeStmt([NotNull] VBAParser.ResumeStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a Resume statement");
            }

            return base.VisitResumeStmt(context);
        }

        public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitReturnStmt([NotNull] VBAParser.ReturnStmtContext context)
        {
            if (_qualifiedSelection.Selection.Contains(context))
            {
                return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a Return statement");
            }

            return base.VisitReturnStmt(context);
        }
    }