Search code examples
c#async-awaitmvp

Update a UI state when all Async methods in different presenters are complete


I have a question regarding the elegance and best practice on the code bellow. The concept of the app:

  1. I have a main form that invokes user controls
  2. This main form has a progress control that show the user the progress of several task through event handlers that are attached to user controls as needed
  3. The View (user control) "ViewPhase1" has the event of progress changing
  4. This View has two presenters
  5. Each of these presenters have async tasks
  6. When a task is about to start progress is changed
  7. When a task has ended I check if both tasks have been completed and if so progress is set to 100% (done)

Because I have two separated presenters that call Async methods, one of them can complete before the other, so I have created two properties "DoneTasksWork" in the View to be able in the presenter that allows to know if Tasks of each Presenter are complete. If both are completed then the progress in the UI is set as 100% (and enables all controls within), if not progress will remain as still running.

Is this a elegant solution? Thinking of this case, progress control only at complete state when Async methods in different classes are complete, can I approach with another solution instead of boolean properties beeing used as flags?

Thank you!

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Drawing;
using System.Data;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using Project.Utilities;

namespace Project
{
    public partial class ViewPhase1 : UserControl, IViewPhase1Work1, IViewPhase1Work2
    {
        private PresenterPhase1Work1 _presenterPhase1Work1;
        private PresenterPhase1Work2 _presenterPhase1Work2;

        public ViewPhase1()
        {
            InitializeComponent();
            _presenterPhase1Work1 = new PresenterPhase1Work1(this);
            _presenterPhase1Work2 = new PresenterPhase1Work2(this);
        }

        /// <summary>
        /// This event is listened by form that invokes this user control and updates progress control.
        /// </summary>
        public event ProgressChangedEventHandler ProgressChangedEvent;
        public void OnProgressHandler(object sender, ProgressChangedEventArgs e)
        {
            this.Invoke((Action)delegate
            {
                if (this.ProgressChangedEvent != null)
                {
                    ProgressChangedEvent(this, e);
                }
            });
        }
        public void ShowException(string exMessage, MessageBoxButtons bts, MessageBoxIcon icon)
        {
            MessageBox.Show("", exMessage, bts, icon);
        }
        public DataGridView GridInvoices { get; set; }
        public DataGridView GridReceipts { get; set; }
        public DataGridView GridProducts { get; set; }
        public DataGridView GridCustomers { get; set; }
        bool IViewPhase1Work1.DoneTasksWork { get; set; }
        bool IViewPhase1Work2.DoneTasksWork { get; set; }
    }

    public interface IViewProgress
    {
        event ProgressChangedEventHandler ProgressChangedEvent;
        void OnProgressHandler(object sender, ProgressChangedEventArgs e);
    }

    public interface IViewPhase1Work1 : IViewProgress, IViewException
    {
        bool DoneTasksWork { get; set; }
        DataGridView GridProducts { get; set; }
        DataGridView GridCustomers { get; set; }
    }

    public interface IViewPhase1Work2 : IViewProgress, IViewException
    {
        bool DoneTasksWork { get; set; }
        DataGridView GridInvoices { get; set; }
        DataGridView GridReceipts { get; set; }
    }

    public interface IViewException
    {
        void ShowException(string exMessage, MessageBoxButtons bts, MessageBoxIcon icon);
    }

    public class PresenterPhase1Work1
    {
        private readonly IViewPhase1Work1 _view;

        public PresenterPhase1Work1(IViewPhase1Work1 view)
        {
            _view = view;
            GetInformation();
        }

        private void GetInformation()
        {
            try
            {
                var task1 = Task.Run(() => GetDataProducts());
                var task2 = Task.Run(() => GetDataCustomers());
                Task.WhenAll(task1, task2);

                _view.GridProducts.DataSource = task1.Result;
                _view.GridCustomers.DataSource = task2.Result;
            }
            catch (Exception ex)
            {
                _view.ShowException(ex.Message, MessageBoxButtons.OK, MessageBoxIcon.Error);
            }

            finally
            {
                _view.DoneTasksWork = true;
                _view.OnProgressHandler(this, new ProgressChangedEventArgs(_view.DoneTasksWork && _view.DoneTasksWork ? 100 : 50, _view.DoneTasksWork && _view.DoneTasksWork ? "Done" : "Getting data"));
            }
        }

        private async Task<object> GetDataCustomers()
        {
            return await Util.GetDataCustomerAsync();
        }
        private async Task<object> GetDataProducts()
        {
            return await Util.GetDataProductsAsync();
        }
    }

    public class PresenterPhase1Work2
    {
        private readonly IViewPhase1Work2 _view;

        public PresenterPhase1Work2(IViewPhase1Work2 view)
        {
            _view = view;
            GetInformation();
        }

        private void GetInformation()
        {
            try
            {
                var task1 = Task.Run(() => GetDataInvoices());
                var task2 = Task.Run(() => GetDataReceipts());
                Task.WhenAll(task1, task2);

                _view.GridInvoices.DataSource = task1.Result;
                _view.GridReceipts.DataSource = task2.Result;
            }
            catch (Exception ex)
            {
                _view.ShowException(ex.Message, MessageBoxButtons.OK, MessageBoxIcon.Error);
            }

            finally
            {
                _view.DoneTasksWork = true;
                _view.OnProgressHandler(this, new ProgressChangedEventArgs(_view.DoneTasksWork && _view.DoneTasksWork ? 100 : 50, _view.DoneTasksWork && _view.DoneTasksWork ? "Done" : "Getting data"));
            }
        }

        private async Task<object> GetDataInvoices()
        {
            return await Util.GetDataInvoicesAsync();
        }

        private async Task<object> GetDataReceipts()
        {
            return await Util.GetDataReceiptsAsync();
        }
    }
}

Solution

  • An elegant solution it is not. Let me explain.

    Exactly what do you expect to happen calling Task.WhenAll(task1, task2); without using await? I will tell you: it runs straight through and will block on _view.GridInvoices.DataSource = task1.Result; and then possible on next line as well.

    So, in order to solve this the method (in PresenterPhase1Work1 but it also applies to the other presenter as well):

    private void GetInformation()
        {
            try
            {
                var task1 = Task.Run(() => GetDataProducts());
                var task2 = Task.Run(() => GetDataCustomers());
                Task.WhenAll(task1, task2);
    
                _view.GridProducts.DataSource = task1.Result;
                _view.GridCustomers.DataSource = task2.Result;
            }
            catch (Exception ex)
            {
                _view.ShowException(ex.Message, MessageBoxButtons.OK, MessageBoxIcon.Error);
            }
    
            finally
            {
                _view.DoneTasksWork = true;
                _view.OnProgressHandler(this, new ProgressChangedEventArgs(_view.DoneTasksWork && _view.DoneTasksWork ? 100 : 50, _view.DoneTasksWork && _view.DoneTasksWork ? "Done" : "Getting data"));
            }
        }
    

    could be changed to:

    private async Task GetInformationAsync()
        {
            try
            {
                var task1 = Task.Run(() => GetDataProducts());
                var task2 = Task.Run(() => GetDataCustomers());
    
                await Task.WhenAny(task1, task2);
                _view.OnProgressHandler(this, new ProgressChangedEventArgs(50, "Getting data"));
    
                await Task.WhenAll(task1, task2);
                _view.OnProgressHandler(this, new ProgressChangedEventArgs(100, "Done"));
    
                _view.GridProducts.DataSource = await task1;
                _view.GridCustomers.DataSource = await task2;
            }
            catch (Exception ex)
            {
                _view.ShowException(ex.Message, MessageBoxButtons.OK, MessageBoxIcon.Error);
            }
        }
    

    Now, calling async methods in the constructor is a bad idea. Constructors don't allow using the async keyword. So you have to redesign your classes. So I would make the GetInformation method public and call and await the method somewhere else in the view. Immediately after the call await presenterPhase1Work1.GetMethodAsync(); in your view you know that piece of work is done. So no more need for this Boolean flag.

    It does mean your ViewPhase1 needs to have another method to call the
    await presenterPhase1Work1.GetMethodAsync(); method because also there you cannot do this in the constructor.

    Waiting for completion of data loading in a constructor is never a good idea IMHO, whether it is done in a background thread or not.