3
\$\begingroup\$

I have a small application that checks some system status and display it details. I have the following code which is working. But I want to make sure it's optimal:

public partial class MainWindow : Window
{

    // Dictionary to hold info
    Dictionary<string, int> dataHolder = new Dictionary<string, string>
    {
        {"status_code", "" },
        {"memory", "" },
        {"process", "" }
    };

    public MainWindow()
    {
        InitializeComponent();
    }

    private void StartBtn_Click(object sender, RoutedEventArgs e)
    {
        StartBtn.IsEnabled = false;

        System.Windows.Threading.DispatcherTimer dispatcherTimer = new System.Windows.Threading.DispatcherTimer();
        dispatcherTimer.Tick += new EventHandler(updateUI);
        dispatcherTimer.Interval = new TimeSpan(0, 5, 0);
        dispatcherTimer.Start();  
    }

    private async void updateUI(object source, EventArgs e)
    {
        await loadInfo();
        StartBtn.IsEnabled = true;

        foreach(KeyValuePair<string, string> entry in dataHolder)
        {
            // Display in the UI
        }
    }

    private async Task loadInfo()
    {
        int page = 1;
        var keys = new List<string>(dataHolder.Keys);            
        foreach (string key in keys)
        {
            dataHolder[key] = "";
            //await asyncCall(key, page);
        }

        // Better asynchronously and in parallel
        await Task.WhenAll(keys.Select(key => asyncCall(key, page)));
    }

    private async Task asyncCall(string key, int page)
    {
        var htClient = new HttpClient();
        // code to retrieve data...
    }
}

So as I said before, this code is working but I read somewhere that having an async void method is not good practice as in private async void updateUI(object source, EventArgs e). I could change it to private async Task updateUI(object source, EventArgs e) but then I would not be able to do dispatcherTimer.Tick += new EventHandler(await updateUI);

Also how to start counting (Timer) only after the previous process is done?

Thank you

EDIT: I am using the DispatcherTimer because I want the application to retrieve the status information and update the UI every 5 minutes.

\$\endgroup\$
  • \$\begingroup\$ Welcome to code review, I hope you get some good answers. \$\endgroup\$ – pacmaninbw Jul 1 '16 at 11:49
2
\$\begingroup\$

Async void should be avoided, but they are acceptable when in an EventHandler so it's fine.

Why are you copying the keys of the dictionary into a list? Do you want a snapshot at a random point in time?

If you have potentially race conditions, I would change the Dictionary to a ConcurrentDictionary.

public partial class MainWindow : Window
{

    // Dictionary to hold info
    private readonly Dictionary<string, string> _dataHolder = new Dictionary<string, string>
    {
        {"status_code", "" },
        {"memory", "" },
        {"process", "" }
    };

    public MainWindow()
    {
        InitializeComponent();
    }

    private async void StartBtn_Click(object sender, RoutedEventArgs e)
    {
        StartBtn.IsEnabled = false;

        while (true) // Your exit condition instead of the true.
        {
            await LoadInfo();

            foreach (var entry in _dataHolder)
            {
                // Display in the UI
            }

            await Task.Delay(TimeSpan.FromMinutes(5));

            //StartBtn.IsEnabled = true;
        }
    }

    private async Task LoadInfo()
    {
        const int page = 1;

        var keys = new List<string>(_dataHolder.Keys);

        foreach (var key in keys)
        {
            _dataHolder[key] = "";
            //await asyncCall(key, page);
        }

        // Better asynchronously and in parallel
        await Task.WhenAll(keys.Select(key => AsyncCall(key, page)));
    }

    private static async Task AsyncCall(string key, int page)
    {
        var htClient = new HttpClient();
        // code to retrieve data...
    }
\$\endgroup\$
  • \$\begingroup\$ I want the app to update the status information every 5 minutes. \$\endgroup\$ – idris Jul 1 '16 at 12:33
  • \$\begingroup\$ If you click on the start button again, what are you expecting to happen? \$\endgroup\$ – Stefano d'Antonio Jul 1 '16 at 12:37
  • \$\begingroup\$ When the application is launched, nothing happens (UI is empty). Then when you click on start button, it updates the information immediately then 5 minutes later, it refreshes and again 5 min, a new refresh and so on \$\endgroup\$ – idris Jul 1 '16 at 12:42
  • \$\begingroup\$ Why were you enabling the start button again? \$\endgroup\$ – Stefano d'Antonio Jul 1 '16 at 12:43
  • \$\begingroup\$ This is just the first part. Eventually I want the user to enter a parameter in a textbox and launch the cycle of status check. But then he can change the parameter and start over. \$\endgroup\$ – idris Jul 1 '16 at 13:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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