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.

This should be the last iteration for this code.

Question 1

Does this approach make sense or am I heading down the wrong path?

Question 2

The only thing I can see to make this better is to genericize the GetType methods at the end so that I could eliminate the goofy if statement in the GetProperties method. Any suggestions?

What I've done between the last question and now

I've used a private method GetProperty as suggested in the last question in order to minimize the repeated code. Here's that method

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

Step 1

I decided to edit the PropertyValue class along these lines to handle the dictionary

public PropertyValue(Dictionary<string, string> propertyList, string wmiClassName).

Where the first string in the dictionary is the type the property will return and second in the dictionary is the propertyName as a string so that it is easy to work with. I'd like to genericize the type part so I could use a list or array instead of a dictionary. Really just to eliminate one of the string variables.

Step 2

I updated the calls to the WMIClasses

public class Win32Processor
{

    public Win32Processor() { }

    readonly PropertyValue propertyValue = new PropertyValue("Win32_Processor");

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

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

To...

public class Win32BaseBoard
{

public object GetValues()
    {
    Dictionary<string, string> propertyList = new Dictionary<string, string>(){

        /// <summary>
        /// 
        /// </summary>
        {"ushort", "AddressWidth"},

        /// <summary>
        /// 
        /// </summary>
        {"ushort", "Architecture"},};

        // ...

        PropertyValue propertyValue = new PropertyValue(propertyList, "Win32_BaseBoard");
        return propertyValue.GetProperties();
    }
}

Then I went about refactoring my PropertyValue class.

After implementing all of the above here's the full code as it stands.

#region WMI Classes

/// <summary>
/// The Win32_BaseBoard WMI class represents a baseboard, which is also known as a motherboard or system board.
/// </summary>
public class Win32BaseBoard
{
    public Win32BaseBoard() { }

    public object GetValues()
    {
        Dictionary<string, string> propertyList = new Dictionary<string, string>(){

            {"ushort", "AddressWidth"},

            {"ushort", "Architecture"},

            {"ushort", "Availability"},

            {"ushort", "CpuStatus"},

            {"uint", "CurrentClockSpeed"},

            {"ushort", "DataWidth"},

            {"string", "Description"},


            {"string", "DeviceID"},

            {"uint", "ExtClock"},


            {"ushort", "Family"},


            {"uint", "L2CacheSize"},


            {"uint", "L2CacheSpeed"},


            {"ushort", "Level"},

            {"ushort", "LoadPercentage"},


            {"string", "Manufacturer"},


            {"uint", "MaxClockSpeed"},


            {"string", "Name"},


            {"uint", "NumberOfCores"},

            {"string", "PNPDeviceID"},

            {"string", "ProcessorId"},

            {"ushort", "ProcessorType"},

            {"ushort", "Revision"},

            {"string", "Role"},

            {"string", "SocketDesignation"},

            {"ushort", "StatusInfo"},

            {"string", "Stepping"},

            {"string", "UniqueId"},

            {"ushort", "UpgradeMethod"},

            {"string", "Version"},

            {"uint", "VoltageCaps"},
        };
        PropertyValue propertyValue = new PropertyValue(propertyList, "Win32_BaseBoard");
        return propertyValue.GetProperties();
    }
}

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

    public object GetValues()
    {
        Dictionary<string, string> propertyList = new Dictionary<string, string>(){

            {"string", "BankLabel"},

            {"string", "BuildNumber"},

            {"ulong", "Capacity"},

            {"string", "CurrentLanguage"},

            {"ushort", "DataWidth"},

            {"string", "Description"},

            {"ushort", "FormFactor"},

            {"bool", "HotSwappable"},

            {"string", "InstallableLanguages"},

            {"string", "Manufacturer"},

            {"ushort", "MemoryType"},

            {"string", "Name"},

            {"string", "PartNumber"},

            {"uint", "PositionInRow"},

            {"datetime", "ReleaseDate"},

            {"string", "SerialNumber"},

            {"string", "SMBIOSMajorVersion"},

            {"string", "SMBIOSMinorVersion"},

            {"bool", "SMBIOSPresent"},

            {"string", "SMBIOSBIOSVersion"},

            {"uint", "Speed"},

            {"string", "Status"},

            {"string", "Tag"},

            {"ushort", "TypeDetail"},

            {"string", "Version"}
        };
        PropertyValue propertyValue = new PropertyValue(propertyList, "Win32_BIOS");
        return propertyValue.GetProperties();
    }


}

/// <summary>
/// The Win32_DiskDrive WMI class represents a physical disk drive as seen by a computer running the Windows 
/// operating system. Any interface to a Windows physical disk drive is a descendent (or member) of this class. 
/// The features of the disk drive seen through this object correspond to the logical and management 
/// characteristics of the drive. In some cases, this may not reflect the actual physical characteristics 
/// of the device. Any object based on another logical device would not be a member of this class.
/// For security reasons, a user connecting from a remote computer must have the SC_MANAGER_CONNECT privilege 
/// enabled to be able to enumerate this class.
/// </summary>
public class Win32DiskDrive
{
    public Win32DiskDrive() { }

    public object GetValues()
    {
        Dictionary<string, string> propertyList = new Dictionary<string, string>(){

            /// <summary>
            /// 
            /// </summary>
            {"string", "Description"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "Manufacturer"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "Model"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "Name"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "OtherIdentifyingInfo"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "PartNumber"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "Product"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "RequirementsDescription"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "SerialNumber"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "SKU"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "SlotLayout"},

            /// <summary>
            /// 
            /// </summary>
            {"bool", "SpecialRequirements"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "Tag"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "Version"}
        };
        PropertyValue propertyValue = new PropertyValue(propertyList, "Win32_DiskDrive");
        return propertyValue.GetProperties();
    }
}

/// <summary>
/// The Win32_DiskPartition WMI class represents the capabilities and management capacity of a partitioned area 
/// of a physical disk on a computer system running Windows. Example: Disk #0, Partition #1.
/// </summary>
public class Win32DiskPartion
{
    public Win32DiskPartion() { }

    public object GetValues()
    {
        Dictionary<string, string> propertyList = new Dictionary<string, string>(){

            /// <summary>
            /// 
            /// </summary>
            {"string", "Name"},

            /// <summary>
            /// 
            /// </summary>
            {"bool", "PrimaryBIOS"}
        };
        PropertyValue propertyValue = new PropertyValue(propertyList, "Win32_DiskPartion");
        return propertyValue.GetProperties();
    }
}

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

    public Win32ComputerSystem() { }

    public object GetValues()
    {
        Dictionary<string, string> propertyList = new Dictionary<string, string>(){
            /// <summary>
            /// Key of a CIM_System instance in an enterprise environment.
            /// This property is inherited from CIM_ManagedSystemElement.
            /// </summary>   
            {"string", "Name"},

            /// <summary>
            /// Name of a computer manufacturer.
            /// </summary>  
            {"string", "Manufacturer"},

            /// <summary>
            /// Product name that a manufacturer gives to a computer. This property must have a value.
            /// </summary>
            {"string", "Model"}
    };
        PropertyValue propertyValue = new PropertyValue(propertyList, "Win32_ComputerSystem");
        return propertyValue.GetProperties();
    }
}

/// <summary>
/// The Win32_LogicalDisk WMI class represents a data source that resolves to an actual local storage device 
/// on a computer system running Windows.
/// </summary>
public class Win32LogicalDisk
{
    public Win32LogicalDisk() { }

    public object GetValues()
    {
        Dictionary<string, string> propertyList = new Dictionary<string, string>(){

            /// <summary>
            /// 
            /// </summary>
            {"uint", "BytesPerSector"},

            /// <summary>
            /// 
            /// </summary>
            {"ushort", "Capabilities"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "Caption"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "DeviceID"},

            /// <summary>
            /// 
            /// </summary>
            {"suint", "Index"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "InterfaceType"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "Manufacturer"},

            /// <summary>
            /// 
            /// </summary>
            {"bool", "MediaLoaded"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "MediaType"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "Model"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "Name"},

            /// <summary>
            /// 
            /// </summary>
            {"uint", "Partitions"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "PNPDeviceID"},

            /// <summary>
            /// 
            /// </summary>
            {"uint", "SCSIBus"},

            /// <summary>
            /// 
            /// </summary>
            {"ushort", "SCSILogicalUnit"},

            /// <summary>
            /// 
            /// </summary>
            {"ushort", "SCSIPort"},

            /// <summary>
            /// 
            /// </summary>
            {"ushort", "SCSITargetId"},

            /// <summary>
            /// 
            /// </summary>
            {"uint", "SectorsPerTrack"},

            /// <summary>
            /// 
            /// </summary>
            {"uint", "Signature"},

            /// <summary>
            /// 
            /// </summary>
            {"ulong", "Size"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "Status"},

            /// <summary>
            /// 
            /// </summary>
            {"ulong", "TotalCylinders"},

            /// <summary>
            /// 
            /// </summary>
            {"uint", "TotalHeads"},

            /// <summary>
            /// 
            /// </summary>
            {"ulong", "TotalSectors"},

            /// <summary>
            /// 
            /// </summary>
            {"ulong", "TotalTracks"},

            /// <summary>
            /// 
            /// </summary>
            {"uint", "TracksPerCylinder"}
        };
        PropertyValue propertyValue = new PropertyValue(propertyList, "Win32_LogicalDisk");
        return propertyValue.GetProperties();
    }
}

/// <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 object GetValues()
    {
        Dictionary<string, string> propertyList = new Dictionary<string, string>(){
            /// <summary>
            /// Number, in megabytes, of physical memory currently unused and available.
            /// </summary>    
            {"ulong", "FreePhysicalMemory"},
            /// <summary>
            /// Number, in megabytes, of virtual memory currently unused and available.
            /// </summary>    
            {"ulong", "FreeVirtualMemory"},
            /// <summary>
            /// Number, in megabytes, of virtual memory.
            /// </summary>
            {"ulong", "TotalVirtualMemory"}
        };
        PropertyValue propertyValue = new PropertyValue(propertyList, "Win32_OperatingSystem");
        return propertyValue.GetProperties();
    }
}

/// <summary>
/// The Win32_PhysicalMemory WMI class represents a physical memory device located on a computer system and 
/// available to the operating system.
/// </summary>
public class Win32PhysicalMemory
{
    public Win32PhysicalMemory() { }

    public object GetValues()
    {
        Dictionary<string, string> propertyList = new Dictionary<string, string>(){

            /// <summary>
            /// 
            /// </summary> 
            {"ulong", "BlockSize"},

            /// <summary>
            /// 
            /// </summary>
            {"bool", "Bootable"},

            /// <summary>
            /// 
            /// </summary>
            {"bool", "BootPartition"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "Description"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "DeviceID"},

            /// <summary>
            /// 
            /// </summary>
            {"ushort", "DiskIndex"},

            /// <summary>
            /// 
            /// </summary>
            {"ushort", "Index"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "Name"},

            /// <summary>
            /// 
            /// </summary>
            {"ulong", "NumberOfBlocks"},

            /// <summary>
            /// 
            /// </summary>
            {"bool", "PrimaryPartition"},

            /// <summary>
            /// 
            /// </summary>
            {"ulong", "Size"},

            /// <summary>
            /// 
            /// </summary>
            {"ulong", "StartingOffset"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "Type"}
        };

        PropertyValue propertyValue = new PropertyValue(propertyList, "Win32_PhysicalMemory");
        return propertyValue.GetProperties();
    }
}

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

    public object GetValues()
    {
        Dictionary<string, string> propertyList = new Dictionary<string, string>(){
            /// <summary>
            /// Key of a CIM_System instance in an enterprise environment.
            /// This property is inherited from CIM_ManagedSystemElement.
            /// </summary>   
            {"ushort", "Architecture"},

            /// <summary>
            /// Name of a computer manufacturer.
            /// </summary>  
            {"string", "Description"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "DeviceID"},

            /// <summary>
            /// 
            /// </summary>
            {"uint", "DriveType"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "FileSystem"},

            /// <summary>
            /// 
            /// </summary>
            {"uint", "MediaType"},

            /// <summary>
            /// 
            /// </summary>
            {"string", "Name"},

            /// <summary>
            /// 
            /// </summary>
            {"bool", "VolumeDirty"}
        };
        PropertyValue propertyValue = new PropertyValue(propertyList, "Win32_Processor");
        return propertyValue.GetProperties();
    }
}

#endregion

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

    public PropertyValue() { }

    public PropertyValue(Dictionary<string, string> propertyList, string wmiClassName)
    {
        this.PropertyList = propertyList;
        this.WmiClassName = wmiClassName;
    }

    public string WmiClassName { get; set; }
    public Dictionary<string, string> PropertyList { get; set; }

    /// <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; }

    public object GetProperties()
    {
        foreach (var pair in PropertyList)
        {
            if (pair.Key.ToLower() == "datetime")
            { return GetDateTimeFromDmtf(pair.Value); }
            if (pair.Key.ToLower() == "string")
            { return GetString(pair.Value); }
            if (pair.Key.ToLower() == "ulong")
            { return GetUnsignedInt64(pair.Value); }
            if (pair.Key.ToLower() == "bool")
            { return GetBool(pair.Value); }
            if (pair.Key.ToLower() == "ushort")
            { return GetUnignedInt16(pair.Value); }
            if (pair.Key.ToLower() == "uint")
            { return GetUnsignedInt32(pair.Value); }
        }
        return null;
    }

    private uint GetUnsignedInt32(string propertyName)
    {
        var property = FindProperty(propertyName);
        return property == null ? (uint)(0) : (uint)(property);
    }

    private object FindProperty(string propertyName)
    {
        using (ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM " + WmiClassName))
        using (var collection = moSearcher.Get())
        using (var enu = collection.GetEnumerator())
            return (!enu.MoveNext() || enu.Current[propertyName] == null) ? null : enu.Current[propertyName];

    }

    private DateTime GetDateTimeFromDmtf(string propertyName)
    {
        var property = FindProperty(propertyName);
        return property == null ? DateTime.Today : ManagementDateTimeConverter.ToDateTime(property.ToString());
    }

    private string GetString(string propertyName)
    {
        var property = FindProperty(propertyName);
        return property == null ? String.Empty : property.ToString();
    }

    private ulong GetUnsignedInt64(string propertyName)
    {
        var property = FindProperty(propertyName);
        return property == null ? (ulong)(0) : KiloBytesToMegaBytes((ulong)(property));
    }

    private bool GetBool(string propertyName)
    {
        var property = FindProperty(propertyName);
        return property == null ? false : true;
    }

    private ushort GetUnignedInt16(string propertyName)
    {
        var property = FindProperty(propertyName);
        return property == null ? (ushort)(0) : (ushort)(property);
    }
}
#endregion

Questions in this series

Class Seperation vs Polymorphism.

First iteration of DRY refactoring.

Second iteration of DRY refactoring.

You are currently viewing the fourth question in the series.

share|improve this question

1 Answer 1

First:

        if (pair.Key.ToLower() == "datetime")
        { return GetDateTimeFromDmtf(pair.Value); }
        if (pair.Key.ToLower() == "string")
        { return GetString(pair.Value); }
        if (pair.Key.ToLower() == "ulong")
        { return GetUnsignedInt64(pair.Value); }
        if (pair.Key.ToLower() == "bool")
        { return GetBool(pair.Value); }
        if (pair.Key.ToLower() == "ushort")
        { return GetUnignedInt16(pair.Value); }
        if (pair.Key.ToLower() == "uint")
        { return GetUnsignedInt32(pair.Value); }

Screaming to be a switch statement.

Second, these guys in every GetValues method:

Dictionary<string, string> propertyList = new Dictionary<string, string>(){

    /// <summary>
    /// 
    /// </summary>
    {"ushort", "AddressWidth"},

    /// <summary>
    /// 
    /// </summary>
    {"ushort", "Architecture"},};

a) are immutable once created, so it makes better sense to make them class-level variables. So they're initialized only once. b) should then be readonly. c) should be declared as IDictionary<string, string> so that you're developing against an interface rather than an implementation. This will need to be changed throughout method signatures, etc.

Third: GetUnignedInt16 really should be spelled correctly. GetUnsignedInt16.

Fourth: GetProperties() in the PropertyValue class does not "get properties". It gets a single property. It takes the Dictionary, but only returns the first property value (this is due to return being in the foreach loop. My guess is maybe you mean to add to a list or array?). Seems like a misnaming and a bug at the same time. Fix the code before the review.

Those are initial thoughts.

share|improve this answer
    
If GetProperties() is actually supposed to return all properties, simply writing it with yield return will do the trick. –  Bobson Mar 10 at 17:43
    
And with the required IEnumerable<object> return type. yield return methods must have a collection return type. –  Jesse C. Slicer Mar 10 at 17:53
    
A minor issue: You should use .ToUpperInvariant. It will probably never matter, but ToUpper is "better" than ToLower -- I don't have a link to the actual reason, but I believe it is really only an issue for some Unicode characters. And similarly using the Invariant culture mostly just avoids Code Analysis warnings, but they are there for a (most unlikely) reason. –  Mark Hurd Mar 31 at 2:35

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.