Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

Regex search/replace

This is the defining interface:

public interface IRegexSearchReplace
{
    List<RegexSearchResult> 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<RegexSearchResult> Find(string searchPattern, RegexSearchReplaceScope scope)
{
    var results = new List<RegexSearchResult>();

    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:
            results.AddRange(GetResultsFromModule(_model.VBE.ActiveCodePane.CodeModule, searchPattern));
            break;

        case RegexSearchReplaceScope.AllOpenedFiles:
            foreach (var codePane in _model.VBE.CodePanes.Cast<CodePane>().Where(codePane => ReferenceEquals(_model.VBE, codePane.VBE)))
            {
                results.AddRange(GetResultsFromModule(codePane.CodeModule, searchPattern));
            }
            break;

        case RegexSearchReplaceScope.CurrentProject:
            foreach (var component in _model.VBE.ActiveVBProject.VBComponents.Cast<VBComponent>())
            {
                var module = component.CodeModule;

                if (!ReferenceEquals(_model.VBE.ActiveVBProject, module.VBE.ActiveVBProject)) { continue; }
                results.AddRange(GetResultsFromModule(module, searchPattern));
            }
            break;

        case RegexSearchReplaceScope.AllOpenProjects:
            foreach (VBProject project in _model.VBE.VBProjects)
            {
                foreach (var component in project.VBComponents.Cast<VBComponent>())
                {
                    var module = component.CodeModule;

                    if (!ReferenceEquals(_model.VBE, module.VBE))
                    {
                        continue;
                    }
                    results.AddRange(GetResultsFromModule(module, searchPattern));
                }
            }
            break;
    }

    return results;
}

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);
}

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);
    }
}

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

    for (var i = 1; i <= module.CountOfLines; i++)
    {
        var matches =
            Regex.Matches(module.Lines[i, 1], searchPattern)
                .OfType<Match>()
                .Select(m => new RegexSearchResult(m, module, i)).ToList();

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

As always, any and all comments are welcome.

share|improve this question
1  
In the Replace () method, how should results.Count ever be < 0 ? –  Heslacher Jul 29 at 15:49
    
@Heslacher Oh, I don't know. –  Hosch250 Jul 29 at 15:49

2 Answers 2

up vote 4 down vote accepted

GetResultsFromModule()

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

    for (var i = 1; i <= module.CountOfLines; i++)
    {
        var matches =
            Regex.Matches(module.Lines[i, 1], searchPattern)
                .OfType<Match>()
                .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<T>.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<RegexSearchResult> 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<RegexSearchResult> can't have a Count property with a value < 0 so the check should be

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

and again you have lots of var usage here. You should either declare the type of results or declare a new variable result with the desired type like so

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

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

    RegexSearchResult result = results[0];

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

an additional comment why you only process the first item in the List<> 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.

share|improve this answer
    
Yeah. The library was really written for VBA devs, so the first line is one instead of zero. You're correct. –  RubberDuck Jul 30 at 10:51

I'd like to make a recommendation for the Find method, following on from Heslacher's answer.

I like to use method overloading to associate related methods together. Basically the Find method branches out to different implementations based on the scope flag. So I recommend creating the following methods:

private Find(string searchPattern, Selection s);
private Find(string searchPattern, CodeModule module);
private Find(string searchPattern, IEnumerable<CodeModule> modules);
private Find(string searchPattern, VBProject project);
private Find(string searchPattern, IEnumerable<VBProject> projects);

These methods should be able to chain together so that Find(string searchPattern, IEnumerable<VBProject> projects) calls Find(string searchPattern, VBProject project) repeatedly for each project in its collection and aggregates the results.

You should be able to move the body of GetResultsFromModule into Find(string searchPattern, CodeModule module).

Then you can do this:

public List<RegexSearchResult> Find(string searchPattern, RegexSearchReplaceScope scope)
{
    switch (scope)
    {
        case RegexSearchReplaceScope.Selection:
            Selection currentSelection;  // TODO assign this.
            return Find(searchPattern, currentSelection);

        case RegexSearchReplaceScope.CurrentBlock:
            // for you to do. :D

        case RegexSearchReplaceScope.CurrentFile:
            return Find(searchPattern, _model.VBE.ActiveCodePane.CodeModule);

        case RegexSearchReplaceScope.AllOpenedFiles:
            IEnumerable<CodeModule> activeCodePanes; // TODO assign this.
            return Find(searchPattern, activeCodePanes);

        case RegexSearchReplaceScope.CurrentProject:
            VBProject activeProject;  // TODO assign this.
            return Find(searchPattern, activeProject);

        case RegexSearchReplaceScope.AllOpenProjects:
            IEnumerable<VBProject> openProjects;  // TODO assign this.
            return Find(searchPattern, openProjects);

        default:
            throw new Exception("Arrrgggghhhh!!!!");
    }
}

I had to use my Notepad IDE to do this, so please excuse (and fix) any errors in my code.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.