Is this tidy?
CodeGuru Home VC++ / MFC / C++ .NET / C# Visual Basic VB Forums Developer.com
Results 1 to 6 of 6

Thread: Is this tidy?

  1. #1
    Join Date
    Dec 2009
    Posts
    596

    Is this tidy?

    Hello everyone. I have a reader that I'm not sure if it's being properly closed after it's used. I create a reader in a method call in an object instance , return it to some code in a button click event and close it in the event code. Was the reader passed in the return statement by value or by reference? If by reference, I beleive all is well. If not then I wonder if I'm not programming this like best practice style.

    This is the button click event:
    Code:
            private void btnListBox_Click(object sender, EventArgs e)
            {
                DataAccess dc = new DataAccess();
                SqlDataReader reader = dc.AllWallers();
                
                try
                {
                       
                        listBox1.Items.Clear();
                        while (reader.Read())
                        {
                            listBox1.Items.Add(reader[0] + "\t" + reader[1] + "\t" + reader[2]);
                        }
                        reader.Close();
                }
                catch
                {
    
                }
            }
    and this is the code in the object's method:
    Code:
    namespace AccountingCsharp
    {
        class DataAccess
        {
            public SqlDataReader AllWallers()
            {
                SqlConnection myConn = new SqlConnection("Data Source=(local);Initial Catalog=ToDeleteDB;" + "Integrated Security=true");
                string queryString = "SELECT ID, Handle, State from dbo.LeatherWallersTbl;";
                
                SqlCommand command = new SqlCommand(queryString, myConn);
                myConn.Open();
    
                SqlDataReader reader = command.ExecuteReader();
                return reader;
            }
        }
    }

  2. #2
    Join Date
    Jan 2010
    Posts
    1,099

    Re: Is this tidy?

    Yes, since SqlDataReader is a class (and not a struct), it is, in essence, returned by reference.
    In C#, classes are also known as reference types, because they are essentially managed pointers under the hood. Structs, on the other hand, are called value types, and they have copy-by-value semantics (this is one way C# differs from C++, there's no such a distinction in the type system there).
    You can find out if a type is a class or a struct by hovering your mouse over the type name, and reading the popup. For more details and info on methods and properties, just google something like: msdn TypeNameHere.

    So, your data reader variable in the event handler refers to the same object that was created in the AllWallers() method.

    A few remarks: When using types that implement IDisposable iterface (and this one does), you generally want to call the Dispose() method when you are done with them, to free unmanaged resources as soon as possible.

    The good thing here is that the Dispose() method also calls Close(). There are two ways to invoke Dispose(): explicitly, or via a using block; the result is the same.

    Code:
    // use some disposable object
    Foo f = new Foo();
    f.BlahBlahBlah();
    
    f.Dispose();
    Or:
    Code:
    using (Foo f = new Foo())
    {
         f.BlahBlahBlah();
    }
    // f.Dispose() called automatically
    Now, even though Dispose() calls Close(), it's not wrong to call Close() before Dispose(), and if you are unsure what Dispose() does for a particular type, or if documentation doesn't mention it, or if you want for your code to clearly state what it does - you should do it.

    Also, given the structure of your event handler and the way it uses try-catch, you might want to call reader.Close() on a different line, or use the try-catch-finally block instead (although, the finally block is used to safely release resources obtained in the try block, but you obtain your reader before the code enters the try block, so you don't have to use it).
    The problem is, if there is an exception, the normal execution flow of the program stops, and the code jumps right to the catch block, so reader.Close() might never be called.

    Here are some alternatives:

    Code:
            private void btnListBox_Click(object sender, EventArgs e)
            {
                DataAccess dc = new DataAccess();
                SqlDataReader reader = dc.AllWallers();
                
                try
                {                   
                        listBox1.Items.Clear();
                        while (reader.Read())
                        {
                            listBox1.Items.Add(reader[0] + "\t" + reader[1] + "\t" + reader[2]);
                        }
                }
                catch
                {
                        // you might want to process the error in some way here, rather than just ignore it
                }
    
                reader.Close();
                reader.Dispose();
            }

    Or with the using block:

    Code:
            private void btnListBox_Click(object sender, EventArgs e)
            {
                DataAccess dc = new DataAccess();
    
                using (SqlDataReader reader = dc.AllWallers())
                {
                
                    try
                    {                   
                            listBox1.Items.Clear();
                            while (reader.Read())
                            {
                                listBox1.Items.Add(reader[0] + "\t" + reader[1] + "\t" + reader[2]);
                            }
                    }
                    catch
                    {
                            // you might want to process the error in some way here, rather than just ignore it
                    }
    
                    reader.Close();
    
                }  // end of using block, Dispose() called automatically
            }
    Note: if you wanted to call Dispose() on an object created in a try block, you wouldn't be able to utilize the using statement because the object would have to be released in the finally block - a different block; so you would have to call Dispose() explicitely. Also, you would have to check if the object was created at all (check if the variable is not null), and maybe do some cleanup by making sure other things that use or depend on the object that threw the exception aren't left in an invalid state.
    Last edited by TheGreatCthulhu; July 26th, 2013 at 10:53 PM.

  3. #3
    Join Date
    Dec 2009
    Posts
    596

    Re: Is this tidy?

    Thank you very much for your easy to undersand explanation and for taking the time to show me the alternate ways of coding this. I think for now I'll code it both ways so that I can really get it. Coding with the Using statement might have me forget what I'm gaining because of the shortcut. When I say code it both ways I mean I'll do a listbox with your first example and do another listbox with the other example on my learning project. Thanks again.

  4. #4
    Arjay's Avatar
    Arjay is offline Moderator / MS MVP Power Poster
    Join Date
    Aug 2004
    Posts
    11,307

    Re: Is this tidy?

    Quote Originally Posted by viperbyte View Post
    Thank you very much for your easy to undersand explanation and for taking the time to show me the alternate ways of coding this. I think for now I'll code it both ways so that I can really get it. Coding with the Using statement might have me forget what I'm gaining because of the shortcut. When I say code it both ways I mean I'll do a listbox with your first example and do another listbox with the other example on my learning project. Thanks again.
    Imo, you should always attempt to code with using blocks on any .Net object. Of course, not every object will support IDisposable so you'll get an intellisense error and compiler error for the objects that don't support it. For those that do support it, the using block guarantees that the object will be cleaned up properly. Please don't get into the habit of calling .Close() and/or .Dispose() inside a using block - because it's redundant and only clutters up the code. I find that programmers tend to program by rote or patterns and those that start out coding without the using block, tend to leave in .Close() and .Dispose() statements even after they start using using blocks. I feel it's better to start with using blocks rather than the other way around (because the other way around may lead to resource leaks if you forget to call .Close() or .Dispose().

    Btw, you may wonder about resource leaks in .Net when there is automatic garbage collection. How can that be? Well, garbage collection doesn't act immediately, so you can run out of resources on some items before the gc kicks in. An example of this would be database related objects. For SqlConnection, each connection is part of a pool of db connections. If you are accessing the db frequently and don't close the connection, you won't return that connection to the pool and you might get an out of connections error. A programmer might be tempted to keep a connection open in this case (to alleviate the error), but usually a better approach is to wrap the connection in a using block, so it gets closed immediately after use and let the db provider do it's job of handling the pooled connections under the covers.

    Similarly, folks sometimes wonder why they get "file in use" exceptions when doing multiple operations with files. A common mistake is to use one of the file class that implements IDisposable, but use it without a using block or explicit close or dispose calls. User may get subsequent "file in use" exceptions if they try to access the file later. This is because the gc hasn't kicked in yet so the original file object is keeping the file opened and preventing new objects from accessing it. Using the using block for file operations eliminates this problem.
    Last edited by Arjay; July 28th, 2013 at 06:01 PM.

  5. #5
    Join Date
    Dec 2009
    Posts
    596

    Re: Is this tidy?

    Thanks Arjay.

  6. #6
    Join Date
    Jan 2010
    Posts
    1,099

    Re: Is this tidy?

    Well, OK, I too feel that calling Dispose() inside a using block is redundant, but as far as calling Close() is concerned, although it is technically redundant as well, I disagree that it causes clutter. I feel rather that it helps stating the intent of the code. Maybe it's just me, but I don't like the obscurity of it. (On the other hand, I don't have a problem with obscurity introduced by things like high level APIs, but there's an important difference - there methods come with descriptive names, while here you have a generic language construct.)

    The implementations (well, behaviors) of the Close() and Dispose() methods are somewhat convoluted themselves, even without the using block. IIRC, the design is that Dispose(void) calls Close() which then calls the protected Dispose(bool). On the other hand, the docs for SqlDataReader say that both overloads of Dispose() call Close()...

    The problem is that in the documentation for a particular class, these facts are often stated in some obscure place, so that people who are unsure what's supposed to be happening when the calls are made can have a hard time finding out. Because of this, better-safe-than-sorry seams like a reasonable strategy to me.
    In any case, this is ultimately a matter of preference (or company standards).

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  


Windows Mobile Development Center


Click Here to Expand Forum to Full Width

This is a CodeGuru survey question.


Featured


HTML5 Development Center