Skip to main content
added 157 characters in body
Source Link
dfhwze
  • 13.9k
  • 3
  • 38
  • 101

API #3

public abstract class Win32WmiClass: IDisposable
{
    protected string WmiClassName { get; }
    protected string WmiKeyPropertyName { get; }
    protected string WmiKeySearchCriteria { get; }
    public string HostName { get; }

    protected Win32WmiClass(string wmiClassName, string wmiKeyPropertyName, 
        string wmiKeySearchCriteria, string hostName) 
    {
        WmiClassName = wmiClassName;
        WmiKeyPropertyName = wmiKeyPropertyName;
        WmiKeySearchCriteria = wmiKeySearchCriteria;
        HostName = hostName;
    }

    // .. CR: other code omitted for brevity
}
 

If Win32Share itself could have derived classes, also provide the protected constructor below. If not, take it out and seal the class.

public class Win32Share: Win32WmiClass
{
    public string ShareName { get; }

    protected Win32Share(string wmiClassName, string wmiKeyPropertyName, 
            string wmiKeySearchCriteria, string hostName, string shareName) 
        : base (wmiClassName, wmiKeyPropertyName, wmiKeySearchCriteria, hostName)
    {
        ShareName = shareName;
    }

    public Win32Share(string hostName, string shareName) 
        : this("Win32_Share", "Name", hostName, shareName)
    { 
    }

    // .. CR: other code omitted for brevity
}
public abstract class Win32WmiClass: IDisposable
{
    protected string WmiClassName { get; }
    protected string WmiKeyPropertyName { get; }
    protected string WmiKeySearchCriteria { get; }
    public string HostName { get; }

    protected Win32WmiClass(string wmiClassName, string wmiKeyPropertyName, 
        string wmiKeySearchCriteria, string hostName) 
    {
        WmiClassName = wmiClassName;
        WmiKeyPropertyName = wmiKeyPropertyName;
        WmiKeySearchCriteria = wmiKeySearchCriteria;
        HostName = hostName;
    }

    // .. CR: other code omitted for brevity
}
 
public class Win32Share: Win32WmiClass
{
    public string ShareName { get; }

    protected Win32Share(string wmiClassName, string wmiKeyPropertyName, 
            string wmiKeySearchCriteria, string hostName, string shareName) 
        : base (wmiClassName, wmiKeyPropertyName, wmiKeySearchCriteria, hostName)
    {
        ShareName = shareName;
    }

    public Win32Share(string hostName, string shareName) 
        : this("Win32_Share", "Name", hostName, shareName)
    { 
    }

    // .. CR: other code omitted for brevity
}

API #3

public abstract class Win32WmiClass: IDisposable
{
    protected string WmiClassName { get; }
    protected string WmiKeyPropertyName { get; }
    protected string WmiKeySearchCriteria { get; }
    public string HostName { get; }

    protected Win32WmiClass(string wmiClassName, string wmiKeyPropertyName, 
        string wmiKeySearchCriteria, string hostName) 
    {
        WmiClassName = wmiClassName;
        WmiKeyPropertyName = wmiKeyPropertyName;
        WmiKeySearchCriteria = wmiKeySearchCriteria;
        HostName = hostName;
    }

    // .. CR: other code omitted for brevity
}

If Win32Share itself could have derived classes, also provide the protected constructor below. If not, take it out and seal the class.

public class Win32Share: Win32WmiClass
{
    public string ShareName { get; }

    protected Win32Share(string wmiClassName, string wmiKeyPropertyName, 
            string wmiKeySearchCriteria, string hostName, string shareName) 
        : base (wmiClassName, wmiKeyPropertyName, wmiKeySearchCriteria, hostName)
    {
        ShareName = shareName;
    }

    public Win32Share(string hostName, string shareName) 
        : this("Win32_Share", "Name", hostName, shareName)
    { 
    }

    // .. CR: other code omitted for brevity
}
Source Link
dfhwze
  • 13.9k
  • 3
  • 38
  • 101

Several legacy Windows WMI API's (LAN adapters, CIM, installed products, ..) have extended state available in the Registry. It's a good idea to provide a wrapper class for consumers, keeping all the magic inside this class. I'm not convinced though about the name Win32WmiClass, it suggest a WMI-only interface, but you also use the Registry.


Naming Concentions (C#)

  • Constants should be PascalCased: DEFAULT_WMI_NAMESPACE -> DefaultWmiNamespace

General Guidelines

Use arrow notation to write more clean and compact code.

public string Caption { get { return GetCimInstanceProperty<string>("Caption"); }}
   public string Caption => GetCimInstanceProperty<string>("Caption");

Use built-in path concatenation functions.

return share.Path + path;
return System.IO.Path.Combine(share.Path, path); // namespace could be using statement

Use built-in memoization support. The problem with the code below, is that it will always retrieve the value from the Registry on each get, if the value is an empty string.

private string WmiNamespace
{
    get
    {
        if (string.IsNullOrWhiteSpace(_wmiNamespace))
        {
            _wmiNamespace = Registry.GetValue(REGISTRY_KEY_NAMESPACE, 
                REGISTRY_KEY_VALUE_NAME, DEFAULT_WMI_NAMESPACE) as string;
        }

        return _wmiNamespace;
    }
}

You only want initialise-once support instead.

private Lazy<string> _wmiNamespace = new Lazy<string>(
    () => Registry.GetValue(REGISTRY_KEY_NAMESPACE, REGISTRY_KEY_VALUE_NAME, 
        DEFAULT_WMI_NAMESPACE) as string);

public string WmiNamespace => _wmiNamespace.Value; // only once initialized

OO-Design

  • Your base class is not abstract, but perhaps it should be. Is there a use case to instantiate the base class, I tend to think not? Your virtual properties are glorified constants. It's better, in my opinion, to make a protected constructor with the values of the properties for derived classes to set. This allows for better encapsulation.

I would do something like this:

public abstract class Win32WmiClass: IDisposable
{
    protected string WmiClassName { get; }
    protected string WmiKeyPropertyName { get; }
    protected string WmiKeySearchCriteria { get; }
    public string HostName { get; }

    protected Win32WmiClass(string wmiClassName, string wmiKeyPropertyName, 
        string wmiKeySearchCriteria, string hostName) 
    {
        WmiClassName = wmiClassName;
        WmiKeyPropertyName = wmiKeyPropertyName;
        WmiKeySearchCriteria = wmiKeySearchCriteria;
        HostName = hostName;
    }

    // .. CR: other code omitted for brevity
}

public class Win32Share: Win32WmiClass
{
    public string ShareName { get; }

    protected Win32Share(string wmiClassName, string wmiKeyPropertyName, 
            string wmiKeySearchCriteria, string hostName, string shareName) 
        : base (wmiClassName, wmiKeyPropertyName, wmiKeySearchCriteria, hostName)
    {
        ShareName = shareName;
    }

    public Win32Share(string hostName, string shareName) 
        : this("Win32_Share", "Name", hostName, shareName)
    { 
    }

    // .. CR: other code omitted for brevity
}