Retrieving MAC addresses based on PCI interface connections and SQL queries

Question 1

I am currently using this C# method to retrieve MAC addresses from a user’s computer as a sort of unique identification. It retrieves the MAC address based on the fact that the physical card is connected to the PCI interface.

public static string returnMAC()
{
    ManagementObjectSearcher searcher = new ManagementObjectSearcher("Select MACAddress, PNPDeviceID FROM Win32_NetworkAdapter WHERE MACAddress IS NOT NULL AND PNPDEVICEID IS NOT NULL");
    ManagementObjectCollection mObject = searcher.Get();

    foreach (ManagementObject obj in mObject)
    {
        string pnp = obj["PNPDeviceID"].ToString();
        if (pnp.Contains("PCI\"))
        {
            string mac = obj["MACAddress"].ToString();
            mac = mac.Replace(":", string.Empty);
            return mac;
        }
    }
    return "Nothing happened...";
}

A method I used previously was pulled straight from MSDN, but due to some issues it inherently had (like pulling the wireless’ MAC instead of the LAN line), I switched it out for this one.

I have tested this method on Windows 7 and 8.1 systems, and it works fine in retrieving the physical MAC address. With that said – it uses SQL queries to search for strings, instead of a more typical method that uses System.Net.XXX – a more “built-in” and “native” method. Correct me if I am wrong on this.

Is there any way to make this better? I can’t think of what would make it better, but on the top of my head – LINQ queries? Native methods? I’m not sure.

Question 2

public static string returnMAC()

The C# coding conventions state that functions should be UpperCamelCase. Additionally that function name is not optimal, returnMAC, return it from where? More appropriate would be GetMAC or RetrieveMAC.

You should add XML documentation to this method.

ManagementObjectSearcher searcher = new ManagementObjectSearcher("Select MACAddress, PNPDeviceID FROM Win32_NetworkAdapter WHERE MACAddress IS NOT NULL AND PNPDEVICEID IS NOT NULL");

This could benefit from using a literal/verbatim string:

ManagementObjectSearcher searcher = new ManagementObjectSearcher(@"
SELECT
    MACAddress,
    PNPDeviceID
FROM
    Win32_NetworkAdapter
WHERE
    MACAddress IS NOT NULL AND
    PNPDEVICEID IS NOT NULL");

ManagementObjectCollection mObject = searcher.Get();

foreach (ManagementObject obj in mObject)

Both variable names are not great. You should avoid naming something obj if it is not an Object. Also you have the variable mObject which is a ManagementObjectCollection but can easily be confused to be a ManagementObject. Despite not being a great alternative either, collection and item might be better…though, only slightly.

ManagementObjectCollection collection = searcher.Get();

foreach (ManagementObject item in collection)

string pnp = obj["PNPDeviceID"].ToString();

Again, I would have rather named this variable pnpId or deviceId.

string pnp = obj["PNPDeviceID"].ToString();
if (pnp.Contains("PCI\"))
{
    string mac = obj["MACAddress"].ToString();
    mac = mac.Replace(":", string.Empty);
    return mac;
}

This whole block can be shortened without losing much readability:

    if (item["PNPDeviceID"].ToString().Contains("PCI\"))
    {
        return item["MACAddress"].ToString().Replace(":", string.Empty);
    }

Though, it seems a little bit messy at first sight, I think it’s within an acceptable level.

return "Nothing happened...";

A more appropriate return value would be "" or "No PCI device found.".

Of course, the most appropriate action would be to throw an exception, as that should not happen during normal operation.

Leave a Reply Cancel reply