HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Notifying the UI that Issues Were Found

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
theissuesnotifyingwerefoundthat

Problem

Our Rubberduck VBA IDE Add In does some static code analysis, and then reports all of the found issues back in a gridview. This analysis can take a long time for larger projects and the UI appeared to be frozen until all of the analysis was complete. I've corrected that issue by updating the number of issues found as they're found and now I'm wondering if there is anything else in this code I can improve prior to merging the branch in.

I'm just learning about async code, so I'm particularly interested in how we've done that. Some of it feels wrong and dirty, but I don't know well enough to be entirely sure. I did my best to apply what I learned in a recent review of some other code.

For reference, this is what the UI looks like.

I extracted the Inspection logic into it's own class and interface to inject into the presenter along with the view. The idea is to have the presenter listen for issues being found by the inspector and update the GUI asynchronously, while the inspector in turn asynchronously searchers for issues in the VBProject.

Headers link to this particular version of the files on GitHub and I can add any code not here that you find relevant upon request.

IInspector:

using System;
using System.Collections.Generic;
using Microsoft.Vbe.Interop;
using System.Threading.Tasks;

namespace Rubberduck.Inspections
{
    public interface IInspector
    {
        Task> FindIssues(VBProject project);
        event EventHandler IssuesFound;
    }

    public class InspectorIssuesFoundEventArg : EventArgs
    {
        private readonly int _count;
        public int Count { get { return _count; } }

        public InspectorIssuesFoundEventArg(int count)
        {
            _count = count;
        }
    }
}


Inspector:

```
using Microsoft.Vbe.Interop;
using Rubberduck.VBA;
using Rubberduck.VBA.Nodes;
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;

namespace Rubber

Solution

-
You are still using the UI thread to do the project parsing _parser.Parse(project), because Task.Yield will return the execution back to UI thread. If this call may take significant time, I would suggest to replace it with Task.Run.

-
It looks like you should get exceptions in OnIssuesFound, as you're updating the UI from non-UI thread. See the fix below.

-
What you do with IssuesFound event is actually reporting progress. .NET has a built-in support for asynchronous progress reporting via IProgress and Progress, which properly handles the synchronization with UI thread. See description of how to use them here: Enabling Progress and Cancellation in Async APIs

-
Instead of new Task() and then task.Start you can just use the Task.Run method to create and run the task.

-
Don't use Task.WaitAll as it blocks the thread until all tasks complete. Use await Task.WhenAll() instead.

As a result, my take on FindIssues refactoring look like this:

public async Task> FindIssuesAsync(VBProject project, IProgress progress)
{
    var code = await Task.Run(() => new VBProjectParseResult(_parser.Parse(project))).ConfigureAwait(false);

    var inspections = _inspections.Where(inspection => inspection.Severity != CodeInspectionSeverity.DoNotShow)
                .Select(inspection =>
                    Task.Run(() =>
                    {
                        var result = inspection.GetInspectionResults(code).ToArray();
                        var count = result.Count();
                        if (progress != null && count > 0)
                            progress.Report(count);

                        return result;
                    }));

    ICodeInspectionResult[][] results = await Task.WhenAll(inspections);

    return results.SelectMany(enumerable => enumerable).ToList();
}

Code Snippets

public async Task<IList<ICodeInspectionResult>> FindIssuesAsync(VBProject project, IProgress<int> progress)
{
    var code = await Task.Run(() => new VBProjectParseResult(_parser.Parse(project))).ConfigureAwait(false);

    var inspections = _inspections.Where(inspection => inspection.Severity != CodeInspectionSeverity.DoNotShow)
                .Select(inspection =>
                    Task.Run(() =>
                    {
                        var result = inspection.GetInspectionResults(code).ToArray();
                        var count = result.Count();
                        if (progress != null && count > 0)
                            progress.Report(count);

                        return result;
                    }));

    ICodeInspectionResult[][] results = await Task.WhenAll(inspections);

    return results.SelectMany(enumerable => enumerable).ToList();
}

Context

StackExchange Code Review Q#82922, answer score: 6

Revisions (0)

No revisions yet.