0
\$\begingroup\$

I currently use the following code to asynchronously create a cimsession to a remote host, query the remote host, return multiple values and finally updated some textboxes on my WPF window. Just wanting to know if anyone can see any glaring issues (I'm new to all things async and c# :) ) or a better was of doing it:

using System.Threading;
using System.Threading.Tasks;
using Microsoft.Management.Infrastructure;

//creating cimsession to remote host
CimSession session = await Task.Run(() =>
{
     return CimSession.Create(computerHostName);
});

//getting username, bootupstate, manufacturer and model 
var GI1 = await Task.Run(() =>
{
    var results = new List<string>();
    IEnumerable<CimInstance> queryResults = session.QueryInstances(nameSpace, WQL, "SELECT Username, BootUpState, Manufacturer, Model FROM Win32_ComputerSystem");
    foreach(CimInstance i in queryResults)
    {
        results.Add(i.CimInstanceProperties["Username"].Value.ToString());
        results.Add(i.CimInstanceProperties["BootUpState"].Value.ToString());
        results.Add(i.CimInstanceProperties["Manufacturer"].Value.ToString());
        results.Add(i.CimInstanceProperties["Model"].Value.ToString());
    }
    //returning all the variables
    return results;
});
//adding username, bootupstate, manufacturer and model to their textboxes
GIUS.Text = GI1[0];
GIBS.Text = GI1[1];
GIMF.Text = GI1[2];
GIMD.Text = GI1[3];

Any suggestions or help is appreciated :)

\$\endgroup\$
  • \$\begingroup\$ Which library comes CimSession from? \$\endgroup\$ – dfhwze May 31 '19 at 8:34
  • \$\begingroup\$ @dfhwze Apologies, I've updated the question :) \$\endgroup\$ – I.T Delinquent May 31 '19 at 8:37
  • \$\begingroup\$ This is a snippet which is missing important context. We can't give you a proper review unless you provide the full methods (or ideally the full class) and whatever XAML is necessary to show how they're called. (Eg. are we dealing with async event handlers?) \$\endgroup\$ – Peter Taylor May 31 '19 at 12:24
  • \$\begingroup\$ Hi @PeterTaylor, I'm not really able to share any more code. I just wanted some assistance with the particular snippet. I really appreciate everyone's help and have marked dfhwze's response as the answer as it was the most helpful \$\endgroup\$ – I.T Delinquent May 31 '19 at 12:31
2
\$\begingroup\$

Conventions

  • Use meaningful and camel-cased names var GI1 GIUS ..
  • Use var when the instance type is the same as the declaring type CimSession session =

Design

I suggest to create a class to store the info you require from the cim interface.

 class CimInfo
 {
     public CimInfo(CimInstance cim)
     {
         UserName = GetProperty(cim, "UserName");
         BootUpState = GetProperty(cim, "BootUpState");
         Manufacturer = GetProperty(cim, "Manufacturer");
         Model = GetProperty(cim, "Model");
     }

     private static string GetProperty(CimInstance cim, string name)
     {
         if (cim == null) throw new ArgumentNullException(nameof(cim));
         return cim.CimInstanceProperties[name].Value.ToString();
     }

     public string UserName { get; }
     public string BootUpState { get; }
     public string Manufacturer { get; }
     public string Model { get; }
 }

The async operations can be merged and rewritten using the new class. Since you only have need of one cim instance, we could also avoid looping all queried results.

Notes:

  • Perhaps there is an alternative available for QueryInstances that only returns the first result.
  • If CimSession implements IDisposable, use a using block for it.

snippet

var cimInfo = await Task.Run(() =>
  {
      var session = CimSession.Create(computerHostName);
      var queryResults = session.QueryInstances(nameSpace, WQL, 
          "SELECT Username, BootUpState, Manufacturer, Model FROM Win32_ComputerSystem");
      return new CimInfo(queryResults.FirstOrDefault());
  });

And the output could be

TextBoxUserName.Text = cimInfo.UserName;
TextBoxBootUpState.Text = cimInfo.BootUpState;
TextBoxManufacturer.Text = cimInfo.Manufacturer;
TextBoxModel.Text = cimInfo.Model;
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for your quick reply :) I actually have multiple tasks that get information from WMI and other sources so I think creating a class for each one would just create unnecessary code. I like the idea though :) \$\endgroup\$ – I.T Delinquent May 31 '19 at 9:25
3
\$\begingroup\$
//creating cimsession to remote host
CimSession session = await Task.Run(() =>
{
     return CimSession.Create(computerHostName);
});

Task.Run is a red flag. This should be

CimSession session = await CimSession.CreateAsync(computerHostName);

Similarly for every method you call on session which has a ...Async version, you should use the ...Async version.


It seems that you're calling this from a GUI thread. That means you need to be careful about where your continuations are executed. I would suggest pulling the async calls out into a method and using .ConfigureAwait(false) on all of them, and then calling that method from the GUI thread with ContinueWith(...) to update the GUI, but since you refuse to provide more context I can't be more specific.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I'm not sure I understand that last part: using ConfigureAwait(false) in the pulled-out method makes sense, but why use ContinueWith (which will need to be directed to the UI thread) rather than await for the GUI update after calling such a method? Has something horrifying about async been discovered that I've not yet come across? \$\endgroup\$ – VisualMelon Jun 1 '19 at 14:13
  • \$\begingroup\$ @VisualMelon, the GUI update has to be directed to the UI thread somehow, and exceptions also want to be reflected in the UI from the UI thread. \$\endgroup\$ – Peter Taylor Jun 1 '19 at 14:42

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.