5
\$\begingroup\$

The idea here is to be able to find a USB serial port device connected during runtime, thus not knowing its port number, and use it in the application to retrieve information from the device.

string comportInfo = string.Empty;

using (var entitySearcher = new ManagementObjectSearcher("root\\CIMV2", "SELECT * FROM Win32_PnPEntity WHERE Caption LIKE '%" + SerialPortToFind + "%'"))
{
    using (var serialPortSearcher = new ManagementObjectSearcher("root\\WMI", "SELECT * FROM MSSerial_PortName"))
    {
        //testBase.SamplerWithCancel is like a for loop with exception control, a time between interations and amount of iterations to be made. It expects a true or false value to determine whether a desired condition is met. Failing to return a true value in the specified iterations throws an exception.
        testBase.SamplerWithCancel(() =>
            {
            var portList = serialPortSearcher.Get().Cast<ManagementBaseObject>().ToList();
            var matchingEntities = entitySearcher.Get().Cast<ManagementBaseObject>().First();

            if (portList.Count != 0 && matchingEntities != null)
            {
                foreach (ManagementBaseObject port in portList)
                {
                    if (port["InstanceName"].ToString().Contains(matchingEntities["DeviceID"].ToString()))
                    {
                        comportInfo = port["PortName"].ToString();
                    }
                }
                return true;
            }
            else
                return false;
        }, "Serial port not found", 3, 1500, 500, false, false);
    }
}

The code works well, but I want to know where I can improve it to make it more resilient and less error prone.

I feel like using LINQ would be way more appropiate than what I did.

\$\endgroup\$
3
\$\begingroup\$

The code works well, but I want to know where I can improve it to make it more resilient and less error prone.

  • A very good start would be to always use braces {} although they might be optional.

  • By stacking the using blocks you can save some horizontal spacing.

  • By reverting the condition of if (portList.Count != 0 && matchingEntities != null) you could return early and the else would be (like now) redundant and could be removed which will save some spacing too.

  • The naming of the variables could need some imporvements too, e.g the plural matchingEntities doesn't match the result of the call to First().

  • Storing the result of matchingEntities["DeviceID"].ToString() into a variable will speed up things.

Implementing the mentioned points will lead to

string comportInfo = string.Empty;

using (var entitySearcher = new ManagementObjectSearcher("root\\CIMV2", "SELECT * FROM Win32_PnPEntity WHERE Caption LIKE '%" + SerialPortToFind + "%'"))    
using (var serialPortSearcher = new ManagementObjectSearcher("root\\WMI", "SELECT * FROM MSSerial_PortName"))
{
    //testBase.SamplerWithCancel is like a for loop with exception control, a time between interations and amount of iterations to be made. It expects a true or false value to determine whether a desired condition is met. Failing to return a true value in the specified iterations throws an exception.
    testBase.SamplerWithCancel(() =>
        {
        var portList = serialPortSearcher.Get().Cast<ManagementBaseObject>().ToList();
        var matchingEntity = entitySearcher.Get().Cast<ManagementBaseObject>().First();

        if (portList.Count == 0 || matchingEntity == null)
        {
            return false;
        }

        string entity = matchingEntity["DeviceID"].ToString();

        foreach (ManagementBaseObject port in portList)
        {
            if (port["InstanceName"].ToString().Contains(entity))
            {
                comportInfo = port["PortName"].ToString();
            }
        }

        return true;

    }, "Serial port not found", 3, 1500, 500, false, false);
}
\$\endgroup\$
  • \$\begingroup\$ Thanks! Very constructive ideas right here in both clarity and readability. \$\endgroup\$ – Oscar Guillamon Nov 20 '15 at 8:13
0
\$\begingroup\$

I developed an iteration using LINQ instead of the foreach loop while also implementing the improvements Heslacher suggested, the code now looks like this:

string comportInfo = string.Empty;

using (var entitySearcher = new ManagementObjectSearcher("root\\CIMV2", "SELECT * FROM Win32_PnPEntity WHERE Caption LIKE '%" + SerialPortToFind + "%'"))
using (var serialPortSearcher = new ManagementObjectSearcher("root\\WMI", "SELECT * FROM MSSerial_PortName"))
{
    testBase.SamplerWithCancel(() =>
        {
            var portList = serialPortSearcher.Get().Cast<ManagementBaseObject>().ToList();
            var matchingEntity = entitySearcher.Get().Cast<ManagementBaseObject>().First();

            if (portList.Count == 0 || matchingEntity == null)
            {
                return false;
            }

            var entity = matchingEntity["DeviceID"].ToString();

            comportInfo = portList.Select(x => x)
                .Where<ManagementBaseObject>(x => x["InstanceName"].ToString().Contains(entity))
                .First()["PortName"].ToString();

            return true;
        }, "Device not found", 3, 1500, 500, false, false);
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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