Enumerating smart card terminals asynchronously in ViewModel constructor

Question

I started a project the other day and started going down the path of using async and await as I have never used it before and it seems like a good match. The app that I'm making is a WPF solution using MVVM. It is going to read from a smart card and show the information that was saved to the card. It is part of a demo/concept. As it stands right now the View has next to nothing in it so I'm not going to show it. The test and the ViewModel are of the interest. So let me begin with the test as it is where I first asked this question in my head. I'm using NUnit 3.2 and Moq 4.2

MainWindowViewModelTests.cs

[TestFixture]
public class MainWindowViewModelTests
{
    private Mock<ITerminalFactory> _factory;

    [SetUp]
    public void Setup()
    {
        _factory = new Mock<ITerminalFactory>();
    }

    [Test]
    public void WhenNoTerminalIsConnected_ErrorMessageIndicatesNoTerminal()
    {
        _factory.Setup(f => f.EnumerateTerminals()).Returns(async ()=>
        {
            await Task.Yield();
            return new ITerminal[0];
        });

        var target = GetClassUnderTest();

        Assert.That(target.Error, Is.EqualTo("No Terminals"));
    }

    [Test]
    public void WhenAtleastOneTerminalIsConnected_NoErrorMessages()
    {
        _factory.Setup(f => f.EnumerateTerminals()).Returns(async () =>
        {
            await Task.Yield();
            return new ITerminal[] { Mock.Of<ITerminal>()};
        });

        var target = GetClassUnderTest();

        Assert.That(target.Error, Is.Null.Or.Empty);
        Assert.That(target.Terminals.Count, Is.EqualTo(1));
    }

    private MainWindowViewModel GetClassUnderTest()
    {
        var target = new MainWindowViewModel(_factory.Object);
        while (!target.IsLoaded)
        {
            System.Threading.Thread.Sleep(1);
        }
        return target;
    }
}

MainWindowViewModel.cs

public class MainWindowViewModel : HandleErrorsViewModel
{
    public ICollection<ITerminal> Terminals { get; }
    public bool IsLoaded { get; private set; }

    public MainWindowViewModel(ITerminalFactory terminalFactory)
    {
        Terminals = new ObservableCollection<ITerminal>();
        PopulateTerminals(terminalFactory);
    }

    private async void PopulateTerminals(ITerminalFactory terminalFactory)
    {
        IEnumerable<ITerminal> terminals = await terminalFactory.EnumerateTerminals();
        terminals.ToList().ForEach(Terminals.Add);

        if (Terminals.Count == 0)
            Error = "No Terminals";

        IsLoaded = true;
    }
}

HandleErrorsViewModel.cs and BaseViewModel

omitted as it is just a wrapper around IDataErrorInfo and a BaseViewModel which is just a wrapper around INotifyPropertyChanged

ITerminal/ITerminalFactory

public interface ITerminal : System.IDisposable
{
    string Name { get; }

    IChippedCard Connect();
    void Disconnect();
}
public interface ITerminalFactory
{
    Task<IEnumerable<ITerminal>> EnumerateTerminals();
}

And that is as far as I am right now. I've neve used async and await before. I've never used a Task as a return method before, and I always desire to use the best practices so that is why I am here. So please fire away and let me know thoughts/concerns/tips/ideas.

Answer 1

I saw that you use in your test async/await, so you can do the GetClassUnderTest in the same way. Return a Task and wait for it asynchronously.

Also you can change the inner while and Thread.Sleep to something else. I'd change the PopulateTerminals to public (it doesn't change any inner state except for the Terminals collection and you can do a check before the population that verify if is already populated) and then from your test call it.

I think make it public is the best way, but if you prefer not to do it, consider to use some signal to indicate about IsLoading and not do a useless while.

I don't like that EnumerateTerminals returns IEnumerable<ITerminal>. I don't know what is the logic of the enumerating, but I guess you return List from there so change also the return value to Task<List<ITerminal>>. If you indeed return IEnumerable, this can be a problem because the laziness.

In the VM why is your collection ICollection and not ObservableCollection? If you do it purposely for some reason (sorting) it's OK but if not change it to concrete type (its clearer and avoid conversions).

The ITerminalFactory can be injected to VM as property for example and then you don't need to pass it to populate the collection.