Next Instance of DRY Refactoring

Question

I'd like some additional assistance on refactoring. This is my latest update for this iteration.

#region WMI Classes
/// <summary>
/// The Win32_OperatingSystem WMI class represents a Windows-based OS installed on a computer. Any OS that can be 
/// installed on a computer that can run a Windows-based OS is a descendent or member of this class. 
/// Win32_OperatingSystem is a singleton class. To get the single instance, use "@" for the key.
/// </summary>
public class Win32OperatingSystem
{

    public Win32OperatingSystem() { }

    PropertyGetter pg = new PropertyGetter("Win32_OperatingSystem");

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

    /// <summary>
    /// Number, in megabytes, of virtual memory currently unused and available.
    /// </summary>
    public ulong FreeVirtualMemory()
    {
        return pg.GetUnsignedInt64("FreeVirtualMemory");
    }

    /// <summary>
    /// Number, in megabytes, of virtual memory.
    /// </summary>
    public ulong TotalVirtualMemorySize()
    {
        return pg.GetUnsignedInt64("TotalVirtualMemorySize");
    }
}

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

    public Win32ComputerSystem() { }

    PropertyGetter pg = new PropertyGetter("Win32_ComputerSystem");

    /// <summary>
    /// Key of a CIM_System instance in an enterprise environment.
    /// This property is inherited from CIM_ManagedSystemElement.
    /// </summary>
    public string Name()
    {
        return pg.GetString("Name");
    }

    /// <summary>
    /// Name of a computer manufacturer.
    /// </summary>
    public string Manufacturer()
    {
        return pg.GetString("Manufacturer");
    }

    /// <summary>
    /// Product name that a manufacturer gives to a computer. This property must have a value.
    /// </summary>
    public string Model()
    {
        return pg.GetString("Model");
    }
}

/// <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() { }

    PropertyGetter pg = new PropertyGetter("Win32_Processor");

    /// <summary>
    /// Processor architecture used by the platform.
    /// </summary>
    public ushort Architecture()
    {
        return pg.GetUnignedInt16("Architecture");
    }

    /// <summary>
    /// Description of the object. This property is inherited from CIM_ManagedSystemElement.
    /// </summary>
    public string Description()
    {
        return pg.GetString("Description");
    }
}

/// <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
{
    PropertyGetter pg = new PropertyGetter("Win32_BIOS");

    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()
    {
        return pg.GetString("Version");
    }

    /// <summary>
    /// Assigned serial number of the software element. This property is inherited from CIM_SoftwareElement.
    /// </summary>
    public string SerialNumber()
    {
        return pg.GetString("SerialNumber");
    }


    /// <summary>
    /// Internal identifier for this compilation of this software element. 
    /// This property is inherited from CIM_SoftwareElement.
    /// </summary>
    public string BuildNumber()
    {
        return pg.GetString("BuildNumber");
    }

    /// <summary>
    /// Name of the current BIOS language.
    /// </summary>
    public string CurrentLanguage()
    {
        return pg.GetString("CurrentLanguage");
    }

    /// <summary>
    /// Manufacturer of this software element.
    /// </summary>
    public string Manufacturer()
    {
        return pg.GetString("Manufacturer");
    }

    /// <summary>
    /// BIOS version as reported by SMBIOS.
    /// </summary>
    public string SMBIOSBIOSVersion()
    {
        return pg.GetString("SMBIOSBIOSVersion");
    }

    /// <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()
    {
        return pg.GetString("Status");
    }

    /// <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()
    {
        return pg.GetUnignedInt16("InstallableLanguages");

    }

    /// <summary>
    /// Major SMBIOS version number. This property is NULL if SMBIOS is not found.
    /// </summary>
    /// <returns></returns>
    public ushort SMBIOSMajorVersion()
    {
        return pg.GetUnignedInt16("SMBIOSMajorVersion");
    }

    /// <summary>
    /// Minor SMBIOS version number. This property is NULL if SMBIOS is not found.
    /// </summary>
    public ushort SMBIOSMinorVersion()
    {
        return pg.GetUnignedInt16("SMBIOSMinorVersion");
    }

    /// <summary>
    /// If true, the SMBIOS is available on this computer system.
    /// </summary>
    public bool SMBIOSPresent()
    {
        return pg.GetBool("SMBIOSPresent");
    }

    /// <summary>
    /// Release date of the Windows BIOS converted from UTC format to DateTime. 
    /// </summary>
    public DateTime ReleaseDate()
    {
        return pg.GetDateTimeFromDmtf("ReleaseDate");
    }

}
#endregion

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

    public PropertyGetter() { }

    private string _win32Class;

    /// <summary>
    /// 
    /// </summary>
    /// <param name="win32Class"></param>
    public PropertyGetter(string win32Class)
    {
        this._win32Class = win32Class;
    }

    /// <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 static 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 GetUnsignedInt64(string propertyName)
    {
        using (ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM " + _win32Class))
        {
            using (var collection = moSearcher.Get())
            {
                using (var enu = collection.GetEnumerator())
                {
                    if (!enu.MoveNext() || enu.Current[propertyName] == null)
                    {
                        return (ulong)0;
                    }
                    return KiloBytesToMegaBytes((ulong)enu.Current[propertyName]);
                }
            }
        }
    }

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

    public ushort GetUnignedInt16(string propertyName)
    {
        using (ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM " + _win32Class))
        {
            using (var collection = moSearcher.Get())
            {
                using (var enu = collection.GetEnumerator())
                {
                    if (!enu.MoveNext() || enu.Current[propertyName] == null)
                    {
                        return (ushort)0;
                    }
                    return (ushort)enu.Current[propertyName];
                }
            }
        }
    }

    public bool GetBool(string propertyName)
    {
        using (ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM " + _win32Class))
        {
            using (var collection = moSearcher.Get())
            {
                using (var enu = collection.GetEnumerator())
                {
                    if (!enu.MoveNext() || enu.Current[propertyName] == null)
                    {
                        return false;
                    }
                    return true;
                }
            }
        }
    }

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

Questions in this series

Class Seperation vs Polymorphism.

First iteration of DRY refactoring.

You're currently viewing the third question in the series

Genericizing PropertyValues

Answer 1

The first thing that I thought about was how could we get rid of the duplication in the PropertyGetter class public methods.

So I first went about creating a common method to do just this

private T TypeConvert<T>(
            string propertyName, 
            Func<object, T> convertIfNotNull,
            T defaultIfNullOrEmpty) where T: class
{
    using (ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
    ("SELECT " + propertyName + " FROM " + _win32Class))
    {
        using (var collection = moSearcher.Get(propertyName))
        {
            using (var enu = collection.GetEnumerator())
            {
                return !enu.MoveNext() || enu.Current[propertyName] == null
                        ? defaultIfNullOrEmpty
                        : convertIfNotNull(enu.Current[propertyName]);
            }
        }
    }
} 

This led me to have public methods such as

public string GetString(string propertyName)
{
    return TypeConvert<string>(
        propertyName,
        convertIfNotNull: value => value.ToString(),
        defaultIfNullOrEmpty: String.Empty);    
}

public ulong GetUnsignedInt64(string propertyName)
{
    return TypeConvert<ulong>(
        propertyName,
        convertIfNotNull: value => KiloBytesToMegaBytes((ulong)value),
        defaultIfNullOrEmpty: (ulong)0);    
}

public DateTime GetDateTimeFromDmtf(string propertyName)
{
    return TypeConvert<DateTime>(
        propertyName,
        convertIfNotNull: value => ManagementDateTimeConverter.ToDateTime(value.ToString()),
        defaultIfNullOrEmpty: DateTime.Today);              
}

public bool GetBool(string propertyName)
{
    return TypeConvert<bool>(
        propertyName,
        convertIfNotNull: value => true,
        defaultIfNullOrEmpty: false);           
}

public ushort GetUnignedInt16(string propertyName)
{
    return TypeConvert<ushort>(
        propertyName,
        convertIfNotNull: value => (ushort)value,
        defaultIfNullOrEmpty: 0);   
}

Then I thought. It would be nice to be able to unit test this class as it seems a good candidate for just this. And considering it might be a well used class a unit test might be beneficial.

However looking at this I came across a couple of pitfalls.

1. Use of ManagementObjectSearcher.
2. Use of ManagementDateTimeConverter

Ideally we would remove these however I'm not sure exactly the best way to approach this might be. I'm thinking the ManagementObjectSearcher would be the likely first candidate.

Perhaps injecting via a constructor. Something like:

public interface IObjectSearcher : IDisposable
{
    IObjectCollection Get(string propertyName); 

}

public interface IObjectSearchFactory
{
    IObjectSearcher Create();
}

So the TypeConvert method now becomes:

using (IObjectSearcher moSearcher = _searchFactory.Create())
{
    // ....
}

However I'm interested to see other thoughts on potentially removing these dependencies.

Answer 2

1. What is the point of empty public constructors? If there's no logic in them, remove them.

2. The PropertyGetter class member in multiple classes is never assigned to after it is initialized. Mark them readonly. Same goes for _win32Class in the PropertyGetter class itself.

3. There are a number of methods which follow a particular pattern in the PropertyGetter class. I am a fan of brevity and fewer lines of code, so I made it look like this (for example):

   public DateTime GetDateTimeFromDmtf(string propertyName)
   {
       using (var searcher = new ManagementObjectSearcher("SELECT " + propertyName + " FROM " + this._win32Class))
       using (var collection = searcher.Get())
       using (var enu = collection.GetEnumerator())
       {
           return !enu.MoveNext() || (enu.Current[propertyName] == null)
               ? DateTime.Today
               : ManagementDateTimeConverter.ToDateTime(enu.Current[propertyName].ToString());
       }
   }
   

4. Since all these things have the word "property" in them, possibly make them properties instead of methods? Example:

   /// <summary>
   /// Manufacturer of this software element.
   /// </summary>
   public string Manufacturer()
   {
       return this.pg.GetString("Manufacturer");
   }
   

becomes:

    /// <summary>
    /// Manufacturer of this software element.
    /// </summary>
    public string Manufacturer
    {
        get
        {
            return this.pg.GetString("Manufacturer");
        }
    }

Answer 3

At this point, the most obvious candidate for refactoring seems to be your public methods on PropertyGetter, as they contain a lot of repeated logic. Note that for these examples I'm going to continue using the using statements from your posted coded, but from your last question I remember you were using these unnecessarily. You should only use using in this situation if the classes you're instantiating for moSearcher, collection and enu implements IDisposable. Otherwise all it does is make your code harder to read.

The simple refactoring would probably be as follows. Move the common logic to a private method, like:

private object GetProperty(string propertyName)
{
    using (var moSearcher = new ManagementObjectSearcher
        ("SELECT " + propertyName + " FROM " + _win32Class))
    {
        using (var collection = moSearcher.Get())
        {
            using (var enu = collection.GetEnumerator())
            {
                if (!enu.MoveNext() || enu.Current[propertyName] == null)
                    return null;
                return enu.Current[propertyName];
            }
        }
    }
}

Just for reference, without the using statements (if they do turn out to be unnecessary), this could be:

private object GetProperty(string propertyName)
{
    var moSearcher = new ManagementObjectSearcher
        ("SELECT " + propertyName + " FROM " + _win32Class));
    var collection = moSearcher.Get()
    var enu = collection.GetEnumerator())

    if (!enu.MoveNext() || enu.Current[propertyName] == null)
        return null;
    return enu.Current[propertyName];
}

Then your public methods can look like:

public DateTime GetDateTimeFromDmtf(string propertyName)
{
    var property = GetProperty(propertyName)
    if(property==null)
        return DateTime.Today;
    return ManagementDateTimeConverter.ToDateTime(property.ToString());
}

The public methods will still all follow that common logic (get the property, if it's null return a default, otherwise convert to the desired format and return), but it's far more tolerable. The actual meat of the logic has been moved to its own method, which is exactly what you want for DRY.

Now, there's possible scope for further cutting down on the amount of code you have here using generics, but it depends a bit on taste whether you actually like this. The idea here would be to write a single public method:

public TProp Get<TProp>(string propertyName);

You may not like that name, feel free to change it, I'll keep using it for now. This would be fine if, for example, for null properties you returned default(T) and for non-null you returned (T)property. Unfortunately, you seem to have different ways of handling null and non-null results for your different types. This means you'd need to write something like:

public TProp Get<TProp>(string propertyName)
{
    var property = GetProperty(propertyName)
    if(property==null)
        return DefaultValue<T>();
    return Convert<T>(property);
}

Here, DefaultValue<T>() and Convert<T>(object property) would be methods that handled the conversion. Unfortunately this would mean a big ugly if or switch statement checking the type of T, which while not having unnecessary repetition, is hardly very nice either.

There is a way you could avoid this, and that's to have something like a 'PropertyConverter' class which handles the null and non-null conversion. You'd I could go into more detail on this, but at this point it seems to be adding a lot of complexity and really getting very little value out of it. My suggested solution would be to stick with the version in the first two chunks of code I posted.