Regex Search/Replace

Rubberduck's latest (and greatest?) feature is a regex search/replace:

This is the defining interface:

public interface IRegexSearchReplace
{
    List Find(string pattern, RegexSearchReplaceScope scope = RegexSearchReplaceScope.CurrentFile);
    void Replace(string searchPattern, string replaceValue, RegexSearchReplaceScope scope = RegexSearchReplaceScope.CurrentFile);
    void ReplaceAll(string searchPattern, string replaceValue, RegexSearchReplaceScope scope = RegexSearchReplaceScope.CurrentFile);
}

And here is the implementation:

```
private readonly RegexSearchReplaceModel _model;

public RegexSearchReplace(RegexSearchReplaceModel model)
{
_model = model;
}

public List Find(string searchPattern, RegexSearchReplaceScope scope)
{
var results = new List();

switch (scope)
{
case RegexSearchReplaceScope.Selection:
results.AddRange(GetResultsFromModule(_model.VBE.ActiveCodePane.CodeModule, searchPattern));
results = results.Where(r => _model.Selection.Selection.Contains(r.Selection)).ToList();
break;

case RegexSearchReplaceScope.CurrentBlock:

var declarationTypes = new []
{
DeclarationType.Event,
DeclarationType.Function,
DeclarationType.Procedure,
DeclarationType.PropertyGet,
DeclarationType.PropertyLet,
DeclarationType.PropertySet
};

results.AddRange(GetResultsFromModule(_model.VBE.ActiveCodePane.CodeModule, searchPattern));
dynamic block = _model.ParseResult.Declarations.FindSelection(_model.Selection, declarationTypes).Context.Parent;
var selection = new Selection(block.Start.Line, block.Start.Column, block.Stop.Line,
block.Stop.Column);
results = results.Where(r => selection.Contains(r.Selection)).ToList();
break;

case RegexSearchReplaceScope.CurrentFile:

GetResultsFromModule()

private IEnumerable GetResultsFromModule(CodeModule module, string searchPattern)
{
    var results = new List();

    for (var i = 1; i ()
                .Select(m => new RegexSearchResult(m, module, i)).ToList();

        if (matches.Any())
        {
            results.AddRange(matches);
        }
    }
    return results;
}

There is no need to call ToList() on the result of the Regex.Matches().OffType().Select(). You don't need the check with .Any() either, because the returned enumerable won't be null.

Calling List.AddRange() will only throw if the enumerable will be null, which won't be the case here.

See also what-does-linq-return-when-the-results-are-empty

The other thing I noticed, although it now won't matter anymore is that you are calling ToList() but you are checking with Any(). Here a check with matches.Count > 0 would be better.

Starting the for loop with 1 seems a little bit strange which any C# developer will notice. I assume this is coming from the VBA stuff, hence this would benefit from a comment.

ReplaceAll()

public void ReplaceAll(string searchPattern, string replaceValue, RegexSearchReplaceScope scope)
{
    var results = Find(searchPattern, scope);

    foreach (var result in results)
    {
        var originalLine = result.Module.Lines[result.Selection.StartLine, 1];
        var newLine = originalLine.Replace(result.Match.Value, replaceValue);
        result.Module.ReplaceLine(result.Selection.StartLine, newLine);
    }
}

For my taste I see a too excessive usage of the var keyword. Without having a IDE with Intellisense at hand one would have to guess what Find() will return because it isn't obvious from the right hand side of the assignment. This wouldn't be that worse if the type would be seen in the foreach loop. The next one would have problems would be var originalLine the whole loop would be more obvious if that var would be replaced with string.

Wouln't it be easier to read like so

public void ReplaceAll(string searchPattern, string replaceValue, RegexSearchReplaceScope scope)
{
    List results = Find(searchPattern, scope);

    foreach (var result in results)
    {
        string originalLine = result.Module.Lines[result.Selection.StartLine, 1];
        var newLine = originalLine.Replace(result.Match.Value, replaceValue);
        result.Module.ReplaceLine(result.Selection.StartLine, newLine);
    }
}

or like so

public void ReplaceAll(string searchPattern, string replaceValue, RegexSearchReplaceScope scope)
{
    var results = Find(searchPattern, scope);

    foreach (RegexSearchResult result in results)
    {
        string originalLine = result.Module.Lines[result.Selection.StartLine, 1];
        var newLine = originalLine.Replace(result.Match.Value, replaceValue);
        result.Module.ReplaceLine(result.Selection.StartLine, newLine);
    }
}

Replace()

public void Replace(string searchPattern, string replaceValue, RegexSearchReplaceScope scope)
{
    var results = Find(searchPattern, scope);

    if (results.Count <= 0) { return; }

    var originalLine = results[0].Module.Lines[results[0].Selection.StartLine, 1];
    var newLine = originalLine.Replace(results[0].Match.Value, replaceValue);
    results[0].Module.ReplaceLine(results[0].Selection.StartLine, newLine);
}

As already stated in my comment, the returned List can't have a Count property with a value ` would help to understand this too. This comment can now be more focused on the line RegexSearchResult result = results[0];.

Find()

I don't want to write too much about this method. This method is just too big and each case should be extracted to a separate method.

I understand your idea about initializing the results at the top but would have gone a different way. By extracting the cases to methods which return a List<> the only time you would need the initialized results would be if the switch(scope) would go in the missing default case. So by returning out of the cases and adding the default case you wouldn't need to overwrite the results and the switch` would get more readable. Basically you wouldn't need that variable at all.

StackExchange Code Review Q#98471, answer score: 4