5
\$\begingroup\$

I'd like to get some input on whether or not to DRY up this code. And if so how to implement it correctly. How do I know when it's worth refactoring?

public class Win32OperatingSystem
{

    public Win32OperatingSystem() { }

    /// <summary>
    /// Number, in megabytes, of physical memory currently unused and available.
    /// </summary>
    public ulong FreePhysicalMemory()
    {
        using (PropertyGetter getProperty = new PropertyGetter())
        {
            return getProperty.GetUlong("FreePhysicalMemory", "Win32_OperatingSystem");
        }
    }

    /// <summary>
    /// Number, in megabytes, of virtual memory.
    /// </summary>
    public ulong TotalVirtualMemorySize()
    {
        using (PropertyGetter getProperty = new PropertyGetter())
        {
            return getProperty.GetUlong("TotalVirtualMemorySize", "Win32_OperatingSystem");
        }
    }

    /// <summary>
    /// Number, in megabytes, of virtual memory currently unused and available.
    /// </summary>
    public ulong FreeVirtualMemory()
    {
        using (PropertyGetter getProperty = new PropertyGetter())
        {
            return getProperty.GetUlong("FreeVirtualMemory", "Win32_OperatingSystem");
        }
    }
}

/// <summary>
/// The Win32_ComputerSystem WMI class represents a computer system running Windows.
/// </summary>
public class Win32ComputerSystem
{

    public Win32ComputerSystem() { }

    /// <summary>
    /// Key of a CIM_System instance in an enterprise environment.
    /// This property is inherited from CIM_ManagedSystemElement.
    /// </summary>
    public string Name()
    {
        using (PropertyGetter getProperty = new PropertyGetter())
        {
            return getProperty.GetString("Name", "Win32_ComputerSystem");
        }
    }

    /// <summary>
    /// Name of a computer manufacturer.
    /// </summary>
    public string Manufacturer()
    {
        using (PropertyGetter getProperty = new PropertyGetter())
        {
            return getProperty.GetString("Manufacturer", "Win32_ComputerSystem");
        }
    }

    /// <summary>
    /// Product name that a manufacturer gives to a computer. This property must have a value.
    /// </summary>
    public string Model()
    {
        using (PropertyGetter getProperty = new PropertyGetter())
        {
            return getProperty.GetString("Model", "Win32_ComputerSystem");
        }
    }
}

/// <summary>
/// The Win32_Processor WMI class represents a device that can interpret a sequence of instructions on a computer 
/// running on a Windows operating system. On a multiprocessor computer, one instance of the Win32_Processor class 
/// exists for each processor.
/// </summary>
public class Win32Processor
{

    public Win32Processor() { }

    /// <summary>
    /// Processor architecture used by the platform.
    /// </summary>
    public ushort Architecture()
    {
        {
            using (PropertyGetter getProperty = new PropertyGetter())
            {
                return getProperty.GetUshort("Architecture", "Win32_Processor");
            }
        }
    }

    /// <summary>
    /// Description of the object. This property is inherited from CIM_ManagedSystemElement.
    /// </summary>
    public string Description()
    {
        using (PropertyGetter getProperty = new PropertyGetter())
        {
            return getProperty.GetString("Description", "Win32_Processor");
        }
    }
}

/// <summary>
/// The Win32_BIOS WMI class represents the attributes of the computer system's basic input/output services (BIOS) 
/// that are installed on a computer.
/// </summary>
public class Win32BIOS
{

    public Win32BIOS() { }

    /// <summary>
    /// Version of the BIOS. This string is created by the BIOS manufacturer. 
    /// This property is inherited from CIM_SoftwareElement.
    /// </summary>
    public string Version()
    {
        using (PropertyGetter getProperty = new PropertyGetter())
        {
            return getProperty.GetString("Version", "Win32_BIOS");
        }
    }

    /// <summary>
    /// Assigned serial number of the software element. This property is inherited from CIM_SoftwareElement.
    /// </summary>
    public string SerialNumber()
    {
        using (PropertyGetter getProperty = new PropertyGetter())
        {
            return getProperty.GetString("SerialNumber", "Win32_BIOS");
        }
    }

    /// <summary>
    /// Internal identifier for this compilation of this software element. 
    /// This property is inherited from CIM_SoftwareElement.
    /// </summary>
    public string BuildNumber()
    {
        using (PropertyGetter getProperty = new PropertyGetter())
        {
            return getProperty.GetString("BuildNumber", "Win32_BIOS");
        }
    }

    /// <summary>
    /// Name of the current BIOS language.
    /// </summary>
    public string CurrentLanguage()
    {
        using (PropertyGetter getProperty = new PropertyGetter())
        {
            return getProperty.GetString("CurrentLanguage", "Win32_BIOS");
        }
    }

    /// <summary>
    /// Manufacturer of this software element.
    /// </summary>
    public string Manufacturer()
    {
        using (PropertyGetter getProperty = new PropertyGetter())
        {
            return getProperty.GetString("Manufacturer", "Win32_BIOS");
        }
    }

    /// <summary>
    /// BIOS version as reported by SMBIOS.
    /// </summary>
    public string SMBIOSBIOSVersion()
    {
        using (PropertyGetter getProperty = new PropertyGetter())
        {
            return getProperty.GetString("SMBIOSBIOSVersion", "Win32_BIOS");
        }
    }

    /// <summary>
    /// Current status of the object. Various operational and nonoperational statuses can be defined.
    /// Operational statuses include: "OK", "Degraded", and "Pred Fail" (an element, such as a SMART-enabled 
    /// hard disk drive, may be functioning properly but predicting a failure in the near future). Nonoperational 
    /// statuses include: "Error", "Starting", "Stopping", and "Service". The latter, "Service", could apply during
    /// mirror-resilvering of a disk, reload of a user permissions list, or other administrative work. Not all such
    /// work is online, yet the managed element is neither "OK" nor in one of the other states.
    /// This property is inherited from CIM_ManagedSystemElement. 
    /// </summary>
    /// <returns></returns>
    public string Status()
    {
        using (PropertyGetter getProperty = new PropertyGetter())
        {
            return getProperty.GetString("Status", "Win32_BIOS");
        }
    }

    /// <summary>
    /// Number of languages available for installation on this system. Language may determine properties such as 
    /// the need for Unicode and bidirectional text.
    /// </summary>
    public ushort InstallableLanguages()
    {
        {
            using (PropertyGetter getProperty = new PropertyGetter())
            {
                return getProperty.GetUshort("InstallableLanguages", "Win32_BIOS");
            }
        }
    }

    /// <summary>
    /// Major SMBIOS version number. This property is NULL if SMBIOS is not found.
    /// </summary>
    /// <returns></returns>
    public ushort SMBIOSMajorVersion()
    {
        {
            using (PropertyGetter getProperty = new PropertyGetter())
            {
                return getProperty.GetUshort("SMBIOSMajorVersion", "Win32_BIOS");
            }
        }
    }

    /// <summary>
    /// Minor SMBIOS version number. This property is NULL if SMBIOS is not found.
    /// </summary>
    public ushort SMBIOSMinorVersion()
    {
        {
            using (PropertyGetter getProperty = new PropertyGetter())
            {
                return getProperty.GetUshort("SMBIOSMinorVersion", "Win32_BIOS");
            }
        }
    }

    /// <summary>
    /// If true, the SMBIOS is available on this computer system.
    /// </summary>
    public bool SMBIOSPresent()
    {
        {
            using (PropertyGetter getProperty = new PropertyGetter())
            {
                return getProperty.GetBool("SMBIOSPresent", "Win32_BIOS");
            }
        }
    }

    /// <summary>
    /// Release date of the Windows BIOS converted from UTC format to DateTime. 
    /// </summary>
    public DateTime ReleaseDate()
    {
        {
            using (PropertyGetter getProperty = new PropertyGetter())
            {
                return getProperty.GetDateTimeFromDmtf("ReleaseDate", "Win32_BIOS");
            }
        }
    }
}
#endregion

/// <summary>
/// Handles the actual WMI queries for the other classes in this file
/// </summary>
public class PropertyGetter : IDisposable
{

    public PropertyGetter() { }

    /// <summary>
    /// Converts kilobyte values to megabytes for readability.
    /// </summary>
    /// <param name="kiloBytes">Value to be converted</param>
    /// <returns>Kilobytes converted to megabytes as ulong</returns>
    private ulong KiloBytesToMegaBytes(ulong kiloBytes)
    {
        return kiloBytes / (ulong)1024;
    }

    /// <summary>
    /// Performs WMI queries on objects which return Int64 bit values
    /// </summary>
    /// <param name="propertyName">Property value to be returned</param>
    /// <param name="Win32Class">WMI class which contains desired property</param>
    /// <returns>The property value of the Int64 bit object queried for</returns>
    public ulong GetUlong(string propertyName, string Win32Class)
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM " + Win32Class);
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext())
            {
                return 0;
            }
            return KiloBytesToMegaBytes((ulong)enu.Current[propertyName]);
        }
    }

    public string GetString(string propertyName, string Win32Class)
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM " + Win32Class);
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext() || enu.Current[propertyName].ToString() == null)
            {
                return String.Empty;
            }
            return enu.Current[propertyName].ToString();
        }
    }

    public ushort GetUshort(string propertyName, string Win32Class)
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
           ("SELECT " + propertyName + " FROM " + Win32Class);
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext())
            {
                return (ushort)0;
            }
            return (ushort)enu.Current[propertyName];
        }
    }

    public bool GetBool(string propertyName, string Win32Class)
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM " + Win32Class);
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext())
            {
                return false;
            }
            return true;
        }
    }

    public DateTime GetDateTimeFromDmtf(string propertyName, string Win32Class)
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM " + Win32Class);
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext())
            {
                return DateTime.Today;
            }
            return ManagementDateTimeConverter.ToDateTime(enu.Current[propertyName].ToString());
        }
    }

    public void Dispose(){ }

}

Questions in this series

Class Seperation vs Polymorphism.

You're currently viewing the second question in the series.

Second iteration of DRY refactoring.

Genericizing PropertyValues

\$\endgroup\$
  • \$\begingroup\$ Hi! I have removed the extra closing brace at the end; you could add a bit more context to the question, describing what the code is doing - comments are part of your code and can be subject to review, so don't remove them unless it makes your post too long to be posted ;) - IIRC this is a follow-up to another question, you can consider adding a link to the previous one too. Thanks for posting! :) \$\endgroup\$ – Mathieu Guindon Mar 6 '14 at 13:33
  • \$\begingroup\$ I was thinking of adding the link. I'll do that now. \$\endgroup\$ – Gabriel W Mar 6 '14 at 13:39
  • \$\begingroup\$ Could you explain why PropertyGetter needs to be IDisposable? It doesn't seem to persist anything outside of its methods. \$\endgroup\$ – Ben Aaronson Mar 6 '14 at 13:41
  • \$\begingroup\$ That's something I was playing around with because VS was barking at me for all my using statements. How should this be handled instead? \$\endgroup\$ – Gabriel W Mar 6 '14 at 13:43
  • \$\begingroup\$ Don't use using statements. Instead just have a single PropertyGetter instance as a class-level field or property \$\endgroup\$ – Ben Aaronson Mar 6 '14 at 13:44
6
\$\begingroup\$

Your PropertyGetter.Dispose method does nothing and has nothing to do, so there's no point in PropertyGetter being disposable.

Instead its methods could be static, for example:

public static ulong GetUlong(string propertyName, string Win32Class) { ... }

You can call static methods without using a new PropertyGetter for example:

public ulong FreePhysicalMemory()
{
    return PropertyGetter.GetUlong("FreePhysicalMemory", "Win32_OperatingSystem");
}

By the way, you should theoretically call Dispose on the result of Get() as well as on the result of GetEnumerator(): because the result of Get() is a ManagementObjectCollection instance which is IDisposable, and the result of GetEnumerator() is ManagementObjectEnumerator which is also IDisposable (as well as IEnumerable).

So instead of this ...

using (var enu = moSearcher.Get().GetEnumerator())
{
    ... etc ...
}

... this instead ...

using (var collection = moSearcher.Get())
{
    using (var enu = collection.GetEnumerator())
    {
        ... etc ...
    }
}

However there are members of my group that have an aversion to static classes as they fear they lead to things staying in memory longer that a manually instantiated and disposed class. Can you help shed some light on the subject?

(The following example pseudo-code won't compile, because it uses a too-simplistic version of the SqlConnection API; but I hope it illustrates the point, i.e. object lifetimes.)

Static data exists forever (which may be bad):

static class Foo
{
    // this SqlConnection exists forever!
    static SqlConnection connection = new SqlConnection("sql connection string");

    public static string selectUserName(int userId)
    {
        return connection("select username from users where userid=" + userid.ToString());
    }
}

Local (non-static) data in a static method is eligible for garbage collection as soon as the static method returns (which may be harmless):

static class Foo
{    
    // No static data here!

    public static string selectUserName(int userId)
    {
        // this SqlConnection is temporary
        using (SqlConnection connection = new SqlConnection("sql connection string"))
        {
            return connection("select username from users where userid=" + userid.ToString());
        }
    }
}

The above class has no instance data so there's no reason to construct it: it can be a static class.

Alternatively it could be coded with instance data:

// implements IDisposable because it contains a data member which needs to be disposed
class Foo : IDisposable
{
    // this non-static SqlConnection exists for the same lifetime as
    // the Foo instance[s] which contain[s] it
    SqlConnection connection;

    public Foo()
    {
        SqlConnection connection = new SqlConnection("sql connection string");
    }

    // can't be static because it uses non-static 'connection' member
    public string selectUserName(int userId)
    {
        return connection("select username from users where userid=" + userid.ToString());
    }

    public void Dispose()
    {
        connection.Dispose();
    }
}

The disadvantage of this last one is:

  • You must instantiate a Foo because you call a non-static method
  • You should remember to dispose the Foo perhaps with a using statement

The advantage of this last one is:

  • You can construct a Foo and then call selectUserName several times, using the same Foo instance (therefore the same SqlConnection instance) each time (which may be good if SqlConnection is expensive to construct)
  • You can Dispose Foo (and therefore Dispose the SqlConnection instance) when you have finished with it (which may be better than a static SqlConnection instance which exists forever)

In summary the members of your group are semi-correct:

  • Static data exists forever (e.g. instance member data of a static object exist forever)
  • Local variables in a static method don't exist forever

The code you posted in the OP only uses local data (not instance data, and not static data); therefore it would be better as static methods in a static class.

\$\endgroup\$
  • \$\begingroup\$ Thanks Chris. Quick follow up. If I change the methods in PropertyGetter to static what is the impact on the program memory wise? Are they still disposed of after being called? To sum up how do I differentiate between managed and unmanaged resources? \$\endgroup\$ – Gabriel W Mar 6 '14 at 17:21
  • \$\begingroup\$ Usually your classes needn't be disposable. But when you use a framework class then you should check whether it is disposable: if (only if) it is then it should be explicitly disposed. Your class should be disposable if it contains, as an instance data member, an instance of a class which needs to be disposed: e.g. Google for "implement IDisposable". \$\endgroup\$ – ChrisW Mar 6 '14 at 17:38
  • \$\begingroup\$ As for static, you can use it when a class contains no instance data at all: therefore you never need to create an instance of the PropertyGetter class, never mind dispose it: see for example Static Classes and Static Class Members. \$\endgroup\$ – ChrisW Mar 6 '14 at 17:39
  • 1
    \$\begingroup\$ After reading that article and one on Instance Constuctors I don't see any reason not to use static classes for these queries. However there are members of my group that have an aversion to static classes as they fear they lead to things staying in memory longer that a manually instantiated and disposed class. Can you help shed some light on the subject? \$\endgroup\$ – Gabriel W Mar 7 '14 at 2:20
  • \$\begingroup\$ @GabrielW I updated my answer. \$\endgroup\$ – ChrisW Mar 7 '14 at 6:07
4
\$\begingroup\$

I assume you are using Visual Studio Ctrl+K, Ctrl+F to format your code, thus I will spare you from formatting... But here's my 5 cents after reading your code twice:


Naming

using(PropertyGetter getProperty = new PropertyGetter())

could really use a rename... getProperty looks so much like a standard getter method.
As you are always using the PropertyGetter only for one line, you might as well just name him: pg.

public String getString(string propertyName, string Win32Class)

and others on the other hand are in dire need for a refactor.

You nicely use camelCase until there, but suddenly you switch to PascalCasing. Be consistent. Also you already can see it's a String from the type it returns. writing that in the name is not helpful or clear in any way...

Drying this out:

public someType getType(string propertyName, string Win32Class) 

is often repeated and almost completely the same in each one. You might instead want to go for:

public T? getProperty<T>(string propertyName, Win32Class parentClass){
    ManagementObjectSearcher moSearcher = 
        new ManagementObjectSearcher("SELECT " + propertyName + " FROM " + class);
    using (var enu = moSearcher.Get().GetEnumerator())
    {
        if (!enu.MoveNext() || enu.Current[propertyName] == null)
        {
            return null;
        }
        return (T?)enu.Current[propertyName];
    }
}

This method returns a nullable generic type of the inferred type you give. You could also return the inferred type itself, but then you are not allowed to return null. That is your choice to make.

This assumes you have already extracted possible Win32Class values into an enum:

public enum Win32Class{
   Win32OperatingSystem, Win32ComputerSystem, [...]
}

You'd then have to introduce nullchecks in your calling methods, similar to this one:

public ulong FreeVirtualMemory()
{
    using (PropertyGetter pg= new PropertyGetter())
    {
        ulong? val = pg.getProperty<ulong>("FreeVirtualMemory", Win32Class.Win32_OperatingSystem);
        return val != null ? (ulong)val : (ulong)0;
    }
}

Alternatively you just return T and nullcheck in getProperty

\$\endgroup\$
  • \$\begingroup\$ Having 'string' in the name might be necessary, because you can't have overloaded method names which are overloaded only on the return type. \$\endgroup\$ – ChrisW Mar 6 '14 at 14:20
  • \$\begingroup\$ On unexpected null it would IMO be better to throw an exception than to return a bogus value such as 0. \$\endgroup\$ – ChrisW Mar 6 '14 at 14:21
  • \$\begingroup\$ @ChrisW null is not unexpected... but i didn't want to switch the expected type to return the "correct" bogus value... \$\endgroup\$ – Vogel612 Mar 6 '14 at 14:21
  • \$\begingroup\$ I really like your 'getProperty' method and I can see where that would cut down that class greatly. But isn't it better to go with strongly typed things rather than objects? \$\endgroup\$ – Gabriel W Mar 7 '14 at 4:30
  • \$\begingroup\$ @GabrielW getProperty should probably be private e.g. as shown in my previous answer. \$\endgroup\$ – ChrisW Mar 7 '14 at 5:32
2
\$\begingroup\$

There is no reason for your PropertyGetter to be IDisposable, or to be disposed of after every usage - simply put it as a member of your classes.

As an added bonus for this, you can declare the table name on the PropertyGetter instance, so you don't have to repeat it:

public class PropertyGetter
{
    private string win32Class;

    public PropertyGetter(string win32Class) { 
        this.win32Class = win32Class;
    }

    public ulong GetUlong(string propertyName)
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM " + win32Class);
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext())
            {
                return 0;
            }
            return KiloBytesToMegaBytes((ulong)enu.Current[propertyName]);
        }
    }

    // ... etc.

}

public class Win32OperatingSystem
{
    private PropertyGetter propertyGetter = new PropertyGetter("Win32_OperatingSystem");

    public Win32OperatingSystem() { }

    /// <summary>
    /// Number, in megabytes, of physical memory currently unused and available.
    /// </summary>
    public ulong FreePhysicalMemory()
    {
        return propertyGetter.GetUlong("FreePhysicalMemory");
    }
    // ...
}

Also, some of the properties which don't change (like processor architecture), you might want to cache your answers:

public class Win32Processor
{
    private PropertyGetter propertyGetter = new PropertyGetter("Win32_Processor");

    public Win32Processor() { }

    private ushort? _architecture;
    /// <summary>
    /// Processor architecture used by the platform.
    /// </summary>
    public ushort Architecture()
    {
        if (_architecture == null) {
           _architecture = propertyGetter.GetUshort("Architecture");
        }
        return _architecture;
    }
}
\$\endgroup\$

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.