Class Seperation vs Polymorphism

Question

I'm interested in optimizing my code. Can someone help me understand the best way to trim this code down with DRY methodology?

So I've got these two classes here preforming WMI queries on two separate Win32 classes. My questions are:

1. Should I keep them separated in different classes based on the Win32 class for organizational purposes?
2. Should I use a parent partial class and split these into different files?
3. Should I use methods with optional parameters or overloads in order to reduce the number of classes here?
4. Why is the selected method best?

public class Win32OperatingSystem
{
    public ulong GetFreePhysicalMemory()
    {
        return GetProperty("FreePhysicalMemory");
    }

    public ulong GetTotalVirtualMemorySize()
    {
        return GetProperty("TotalVirtualMemorySize");
    }

    public ulong GetFreeVirtualMemory()
    {
        return GetProperty("FreeVirtualMemory");
    }

    private ulong GetProperty(string propertyName)
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM Win32_OperatingSystem");
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) return 0;
            return BytesToMegaBytes((ulong)enu.Current[propertyName]);
        }
    }

    private ulong BytesToMegaBytes(ulong bytes)
    {
        return bytes / (ulong)1024;
    }
}

public class Win32ComputerSystem
{
    public string GetName()
    {
        return GetProperty("Name");
    }

    public string GetManufacturer()
    {
        return GetProperty("Manufacturer");
    }

    public string GetModel()
    {
        return GetProperty("Model");
    }

    private string GetProperty(string propertyName)
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM Win32_ComputerSystem");
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) return "Unable to retrieve " + propertyName + " from Win32_ComputerSystem!";
            return enu.Current[propertyName].ToString();
        }
    }
}

Questions in this series

This is the first question in the series.

First iteration of DRY refactoring

Second iteration of DRY refactoring.

Genericizing PropertyValues

Answer 1

Should I keep them separated in different classes based on the Win32 class for organizational purposes?

Clean Code also suggest small classes. They have different responsibility, so I think they are fine.

(Clean Code by Robert C. Martin: Chapter 10: Classes, Classes Should Be Small!)

Should I use a parent partial class and split these into different files?

Please don't! It would be useful to remove the duplication of GetProperty but using inheritance for that is an overkill. There is a better solution: composition. Create a PropertyGetter class and move the getter logic into that:

public class PropertyGetter {

    public string GetStringProperty(string propertyName, string tableName)
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM " + tableName);
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) {
                return "Unable to retrieve " + propertyName + " from " + tableName;
            }
            return enu.Current[propertyName].ToString();
        }
    }

    private ulong GetUlongProperty(string propertyName, string tableName) {
        ...
    }
}

The use that class from Win32ComputerSystem and Win32OperatingSystem:

public class Win32ComputerSystem
{
    private PropertyGetter propertyGetter;

    public Win32ComputerSystem(PropertyGetter propertyGetter)
    {
        this.propertyGetter = propertyGetter; // TODO: check null
    }

    public string GetName()
    {
        return propertyGetter.GetStringProperty("Name", "Win32_ComputerSystem");
    }
    ...
}

See also: Effective Java, Second Edition, Item 16: Favor composition over inheritance

In the PropertyGetter class you could eliminate some duplication from the Get*Property methods if you extract out the duplicated logic to a private method(s).

---

Another note: returning 0 or an error message instead of the expected value seems a little bit dangerous.

if (!enu.MoveNext()) return 0;
...    
if (!enu.MoveNext()) return "Unable to retrieve " + propertyName + " from Win32_ComputerSystem!";

If that's an exceptional case an exception might be better. Clients of these classes might use these values as valid data and you just postpone the error which makes debugging harder. (The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)

Answer 2

How about leveraging Enum.ToString()?

Given these two enums:

public enum Win32OperatingSystem
{
    FreePhysicalMemory,
    TotalVirtualMemorySize,
    FreeVirtualMemory
}

public enum Win32ComputerSystem
{
    Name,
    Manufacturer,
    Model
}

Now you can have a simple method:

    public ulong SelectWin32Property(Win32OperatingSystem property)
    {
        var propertyName = property.ToString();
        var moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM Win32_OperatingSystem");
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) return 0;
            return BytesToMegaBytes((ulong)enu.Current[propertyName]);
        }
    }

If you wanted an overload that takes a Win32ComputerSystem parameter, you'd have to deal with the different return value:

    public string SelectWin32Property(Win32ComputerSystem property)
    {
        var propertyName = property.ToString();
        var moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM Win32_ComputerSystem");
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) return "Unable to retrieve " + propertyName + " from Win32_ComputerSystem!";
            return enu.Current[propertyName].ToString();
        }
    }

The easiest would probably be to box the numeric types into a string, so all overloads would return a string representation of their values. This might not be practical, but at least you don't need to write a new method nor to change your interface to support a new property.

Alternatively you could box results in an object, but that wouldn't be pretty, I'd much prefer dealing with a string than exposing object.

The ideal would be to avoid boxing the numeric return types altogether, so the methods have to live in separate classes - this is where generics come handy:

public abstract class Win32QueryBase<TResult, TProperty>
{
    public abstract TResult SelectWin32Property(TProperty property);
}

This can be implemented by this class:

public class Win32OperatingSystemQuery : Win32QueryBase<ulong, Win32OperatingSystem>
{
    public override ulong SelectWin32Property(Win32OperatingSystem property)
    {
        // ...
    }
}

And this class:

public class Win32ComputerSystemQuery : Win32QueryBase<string, Win32ComputerSystem>
{
    public override string SelectWin32Property(Win32ComputerSystem property)
    {
        // ...
    }
}

Answer 3

Should I keep them separated in different classes based on the Win32 class for organizational purposes?

Not necessarily.

The difference between 'computer' and 'operating system' seems to me non-obvious: for example, why is 'memory size' a property of 'operating system' not of 'computer'?

Speaking as a hypothetical user of your API, it isn't clear to me which class I'd want/expect to access for each property.

Furthermore, the implementation of each class is very similar, and has the same dependencies (they both use ManagementObjectSearcher in their implementation).

Therefore IMO you might as well combine them into a single class.

public class Win32System
{
    #region Get megabytes from Win32_OperatingSystem

    public ulong GetFreePhysicalMemory()
    {
        return GetOperatingSystemMegaBytes("FreePhysicalMemory");
    }

    public ulong GetTotalVirtualMemorySize()
    {
        return GetOperatingSystemMegaBytes("TotalVirtualMemorySize");
    }

    public ulong GetFreeVirtualMemory()
    {
        return GetOperatingSystemMegaBytes("FreeVirtualMemory");
    }

    private ulong GetOperatingSystemMegaBytes(string propertyName)
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM Win32_OperatingSystem");
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) return 0;
            return BytesToMegaBytes((ulong)enu.Current[propertyName]);
        }
    }

    private ulong BytesToMegaBytes(ulong bytes)
    {
        return bytes / (ulong)1024;
    }

    #endregion
    #region Get strings from Win32_ComputerSystem

    public string GetName()
    {
        return GetComputerString("Name");
    }

    public string GetManufacturer()
    {
        return GetComputerString("Manufacturer");
    }

    public string GetModel()
    {
        return GetComputerString("Model");
    }

    private string GetComputerString(string propertyName)
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM Win32_ComputerSystem");
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) return "Unable to retrieve " + propertyName + " from Win32_ComputerSystem!";
            return enu.Current[propertyName].ToString();
        }
    }

    #endregion
}

As a further exercise you could now refactor the GetOperatingSystemMegaBytes and GetComputerString methods, so that they share a common implementation/subroutine, e.g. as follows:

    private string GetComputerString(string propertyName)
    {
        object rc = GetManagementObject(propertyName, "Win32_ComputerSystem");
        return rc.ToString();
    }

    private ulong GetOperatingSystemMegaBytes(string propertyName)
    {
        object rc = GetManagementObject(propertyName, "Win32_OperatingSystem");
        return BytesToMegaBytes((ulong)rc);
    }

    private object GetManagementObject(string propertyName, string tableName)
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM " + tableName);
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext())
                throw new YourChoiceOfException();
            return enu.Current[propertyName];
        }
    }

Other comments:

• The public methods could be public properties instead (however see comments below this answer)
• There's something wrong with the BytesToMegaBytes method: it should be called KiloBytesToMegaBytes; or it should be dividing by (1024*1024).
• IMO the Get methods should throw an exception on failure, instead of returning nonsense values such as 0 or an error-message string.
• The whole class could be static, with its methods all static.

Answer 4

I would first move the property retrieval to a small generic class

public class PropertyReader<T>
{
    private string _win32Class;

    public PropertyReader(string win32Class)
    {
        _win32Class = win32Class;
    }

    public T GetProperty(string propertyName)
    {
        string query = "SELECT {0} from {1}";
        using (var moSearcher = new ManagementObjectSearcher(string.Format(query, propertyName, _win32Class)))
        {
            using (var collection = moSearcher.Get())
            {
                using (var enu = collection.GetEnumerator())
                {
                    if (!enu.MoveNext() || enu.Current[propertyName] == null)
                    {
                        return default(T);
                    }
                    return AdjustResult(enu.Current[propertyName]);
                }
            }
        }
    }

    protected virtual T AdjustResult(object obj)
    {
        return (T)obj;
    }
}

This class does all the job and returns a default value in case the property is unknown (whether that is a bad design or wanted can be changed later). Now, in order to return a string property, simply call

public GetStringProperty (string propertyName)
{
  return new PropertyReader<string>().GetProperty(propertyName);
}

The same applies to any datatype which does not require result conversion e.g. to megabytes or date time formatting.

public GetBoolProperty (string propertyName)
{
  return new PropertyReader<bool>().GetProperty(propertyName);
}

For the ulong and DateTime properties, create a derived class and overwrite the AdjustResult method:

public class UnsignedInt64PropertyReader : PropertyReader<ulong>
{
    public UnsignedInt64PropertyReader(string win32Class) : base(win32Class) {}

    private static ulong KiloBytesToMegaBytes(ulong kiloBytes)
    {
        return kiloBytes / (ulong)1024;
    }

    protected override ulong AdjustResult(object obj)
    {
        return KiloBytesToMegaBytes(base.AdjustResult(obj));
    }
}

public class DateTimePropertyReader : PropertyReader<DateTime> 
{
    public DateTimePropertyReader(string win32Class) : base(win32Class) {}

    protected override ulong Value(object obj)
    {
        return ManagementDateTimeConverter.ToDateTime(obj.ToString());
    }
}

This will remove almost all code redundancies as requested. Hope this helps.