views:

87

answers:

3

I'm writing a small piece of code that would determine which serial ports on a computer are free for connection. This is done by looping through the different serial ports and calling the Open() method. If an exception occurs, this indicates that the port is unavailable.

However visual studios is telling me that I'm not disposing of the object properly, or disposing it too many times if I place the dispose method within the finally block. What is the best way of disposing the serial port object, and is it wise to create a new serial port object in the for or leave it how it is?

The commented section with the question marks is the bits that I'm unsure about.

    public static void QueryOpenPorts(out string[] portNames, out bool[] isOpen)
    {
         // serial port object used to query
        SerialPort serialPort = new SerialPort();
        // get valid ports on current computer
        portNames = SerialPort.GetPortNames();
        // number of valid ports
        int count = portNames.Length;
        // initialise isOpen array
        isOpen = new bool[count];

        // iterate through portNames and check Open()
        for (int i = 0; i < count; i++)
        {
            // set port name
            serialPort.PortName = portNames[i];
            // attempt to open port
            try
            {
                serialPort.Open();
                // port available
                isOpen[i] = true;
            }
            catch (Exception ex)
            {
                // serial port exception
                if (ex is InvalidOperationException || ex is UnauthorizedAccessException || ex is IOException)
                {
                    // port unavailable
                    isOpen[i] = false;
                }
            }
            finally
            {

                //    // close serial port if opened successfully ????????????
                //    if (serialPort.IsOpen)
                //    {
                //        serialPort.Close();
                //    }

            }
        }
        // release object ?????????
        // serialPort.Dispose(); 
    } 
A: 

I would declare the SerialPort inside the try (inside the for loop) and use a using() statement on it. You want a different SerialPort instance for each port you are accessing.

Hightechrider
A: 

You shouldn't need to declare the serialport inside the loop, but you do need to reinstantiate it there (ie new SerialPort())

There's another example here:

http://www.gamedev.net/community/forums/topic.asp?topic_id=525884

SteveCav
You don't *need* to, but you *should* be minimizing the lexical scope and putting it in a using() statement for this particular example.
Hightechrider
+1  A: 

You can use a using block for this instead.

using (SerialPort serialPort = new SerialPort(portNames[i]))
{
    try
    {
        serialPort.Open();
        isOpen[i] = true;
        // You could call serialPort.Close() here if you want. It's really not needed, though, since the using block will dispose for you (which in turn will close)
    }
    // This is a better way to handle the exceptions.
    // You don't need to set isOpen[i] = false, since it defaults to that value
    catch (InvalidOperationException) { }
    catch (UnauthorizedAccessException) { }
    catch (IOException) { }
}

Note that you don't need to call Close(), since Dispose() will do this for you.

Jon B
By using the 'using' statement, you don't have to explicitly call dispose/close?
yeastbeast
@yeastbeast - the using statement automatically calls `Dispose()` for you, even if there is an exception. It's essentially no different from using a try/finally - it's just easier to use/read. The `Dispose()` function for SerialPort will call `Close()` for you. You can still cause `Close()` yourself if you like (it's unnecessary but harmless).
Jon B
Here's a link for using statements: http://msdn.microsoft.com/en-us/library/yh598w02(VS.80).aspx
Jon B