Storing reader information in C#

Question

Hi,

I know what I asking might not make a lot of sense for C# experts but I'll explain what I want to do and then you can suggest me how to do it in a better way if you want ok?

I have a C# class called DatabaseManager that deals with different MySQL queries (ado.net NET connector, not linq or any kind of ActiveRecord-ish library).

I am doing something like

categories = db_manager.getCategories();

The list of categories is quite small (10 items) so I'd like to know what's the best way of accessing the retrieved information without a lot of additional code.

Right now I'm using a Struct to store the information but I'm sure there's a better way of doing this.

Here's my code:

    public struct Category
    {
        public string name;
    }
    internal ArrayList getCategories()
    {
        ArrayList categories = new ArrayList();

        MySqlDataReader reader;
        Category category_info;

        try
        {
            conn.Open();
            reader = category_query.ExecuteReader();
            while (reader.Read())
            {
                category_info = new Category();
                category_info.name = reader["name"].ToString();
                categories.Add(category_info);
            }
            reader.Close();
            conn.Close();
        }
        catch (MySqlException e)
        {
            Console.WriteLine("ERROR " + e.ToString());
        }

        return categories;
    }

Thanks in advance

Answer 1

The first thing I would do is to replace you use of ArrayList with List that will provide compile-time type checkig for your use of the category list (so you will not have to type cast it when using it in your code).

Answer 2

From your code I guess you are using .NET 1.1, becuase you are not using the power of generics.

1) Using a struct that only contains a string is an overkill. Just create an arraylist of strings (or with generics a List )

2) When an exception occurs in your try block, you leave your connection and reader open... Use this instead:

try 
{
   conn.open();
   //more code
}
catch (MySqlException e) { // code
}
finally {
   conn.close()
   if (reader != null)
      reader.close();
}

Answer 3

Example:

public IEnumerable<Category> GetCategories()
{
    using (var connection = new MySqlConnection("CONNECTION STRING"))
    using (var command = new MySqlCommand("SELECT name FROM categories", connection))
    {
        connection.Open();
        using (var reader = command.ExecuteReader())
        {
            while (reader.Read())
            {
                yield return new Category { name = reader.GetString(0) };
            }
        }
    }
}

Remarks:

1. Let ADO.NET connection pooling do the right work for you (avoid storing connections in static fields, etc...)
2. Always make sure to properly dispose unmanaged resources (using "using" in C#)
3. Always return the lowest interface in the hierarchy from your public methods (in this case IEnumerable<Category>).
4. Leave the callers handle exceptions and logging. These are crosscutting concerns and should not be mixed with your DB access code.

Answer 4

There's nothing wrong with returning them in an like this. However, a few things stand out:

• Your catch block logs the error but then returns either an empty array or a partially populated array. This probably isn't a good idea
• If an exception is thrown in the try block you won't close the connection or dispose of the reader. Consider the using() statement.
• You should use the generic types (List<>) instead of ArrayList.