Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

share|improve this question
add comment

4 Answers 4

up vote 4 down vote accepted

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.)

share|improve this answer
    
I'm not quite sure what you are getting at having the PropertyGetter take an instance of a PropertyGetter?? –  dreza Mar 4 at 20:40
    
@dreza: Thanks, fixed. –  palacsint Mar 4 at 20:45
add comment

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)
    {
        // ...
    }
}
share|improve this answer
1  
I'm not sure if your abstract class is very useful, but I love your enum. (I always love enums!) –  Simon André Forsberg Mar 4 at 20:27
    
Please bear with me here as I'm new to generics. Let me see if I understand. –  Gabriel W Mar 4 at 21:08
    
The ManagementObject.Item property (inevitably used in the implementation) is already returning a (boxed) Object; so IMO your SelectWin32Property method could return an object i.e. return enu.Current[propertyName], and let the caller (or a wrapper method like the GetComputerString and/or GetOperatingSystemMegabytes methods in my answer) do the casting of that object. –  ChrisW Mar 4 at 21:36
    
@ChrisW interesting... can you tell I don't do WMI very often? :) - if it's all boxed-up already, then I'd still try to expose the appropriate type to client code, so as to not expose object in my API... but that could be a debatable concern. –  Mat's Mug Mar 4 at 21:38
2  
I too would avoid exposing object in my public API: I'd expose it in my private SelectWin32Property method. E.g. you could have several enum types (one for properties which return strings, another for properties which return longs), have a corresponding public method for each enum type, call the private SelectWin32Property method from the public method, and cast from object inside the implementation of the public method. –  ChrisW Mar 4 at 21:43
show 2 more comments

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.
share|improve this answer
    
Good answer. I was going to also recommend properties, but found that they'd really be methods in disguise; I don't like a property getter that makes a WMI query. –  Mat's Mug Mar 4 at 21:12
    
Can you explain (or link to an article which explains) why one shouldn't use a property to e.g. wrap a WMI query? –  ChrisW Mar 4 at 21:20
1  
Sure, see this SO answer, first point is "The getters should be simple and thus unlikely to throw exceptions. Note that this implies no network (or database) access. Either might fail, and therefore would throw an exception." –  Mat's Mug Mar 4 at 21:21
    
I don't like the fact that you have to use the same conversion per data type. This is redundant and should be avoided. –  alzaimar Mar 11 at 7:41
    
@alzaimar I don't understand your comment. Do you mean the fact that GetOperatingSystemMegaBytes is used in the implementation of two different properties i.e. GetFreePhysicalMemory and GetTotalVirtualMemorySize? And what is the solution you prefer? –  ChrisW Mar 11 at 9:31
show 1 more comment

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.