6
\$\begingroup\$

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.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ I see some of the points you are trying to make, however some of them I don't agree with. I agree that I missed the chance to use async/await in my GetClassUnderTest. I agree with the IsLoading part. As for the others. I return IEnumerable because of the laziness. I'm familiar enough with the quirks of it that I prefer it now. As for my collection using ICollection instead of Concrete I don't agree that it is more clear. It's a collection. I add to it, and I get the count. I'll let the VM care only if it is observable or not. The last sentence may be a different from prefered IoC's. \$\endgroup\$ Jun 27, 2016 at 12:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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