3
\$\begingroup\$

From the outset, let me state that I'm working with some older technology. I'd rather use Entity Framework, but I have to use what the client is using if I want to get paid.

I am converting some old VB6/VBA code to C#. The VB code relies on RecordSet, which allow you to browse results of a query and also randomly make changes to the current row.

To duplicate this functionality in C#, I need to make use of SqlDataAdapter, DataSet/DataTable and SqlCommandBuilder classes. Since every single database object seems to implement IDisposable, the whole thing becomes a bit unwieldy and detracts from my main logic.

So I've tried to provide this same functionality in a single class that implements IDisposable. I would love some feedback on this class. My main concern is if I'm leaving anything out that could leave something not getting cleaned up, but would also love any ideas on how I might make the code better.

RecordSet Class

public class RecordSet : IDisposable
{
    protected SqlConnection Connection;
    protected bool OwnsConnection;
    protected SqlCommand Command;
    protected SqlDataAdapter Adapter;
    protected SqlCommandBuilder CommandBuilder;
    protected DataTable DataTable;

    protected int RowIndex;

    public RecordSet(string connectionString)
    {
        if (connectionString == null)
            throw new NullReferenceException("connectionString");
        Connection = new SqlConnection(connectionString);
        OwnsConnection = true;
    }

    public RecordSet(SqlConnection connection)
    {
        if (connection == null)
            throw new NullReferenceException("connection");
        Connection = connection;
        OwnsConnection = false;
    }

    ~RecordSet()
    {
        Debug.Assert(Connection == null);
        Debug.Assert(Command == null);
        Debug.Assert(Adapter == null);
        Debug.Assert(CommandBuilder == null);
        Debug.Assert(DataTable == null);
    }

    public void Open(string query, params SqlParameter[] parameters)
    {
        if (Disposed)
            throw new NotSupportedException("Cannot reuse RecordSet.");

        Command = new SqlCommand(query, Connection);
        if (parameters != null)
        {
            foreach (SqlParameter parameter in parameters)
                Command.Parameters.Add(parameter);
        }
        Adapter = new SqlDataAdapter(Command);
        CommandBuilder = new SqlCommandBuilder(Adapter);
        DataTable = new DataTable();

        Adapter.Fill(DataTable);
        RowIndex = -1;
    }

    public bool Read()
    {
        if ((RowIndex + 1) < DataTable.Rows.Count)
        {
            RowIndex++;
            return true;
        }
        return false;
    }

    public object this[string field]
    {
        get
        {
            return DataTable.Rows[RowIndex][field];
        }
        set
        {
            DataTable.Rows[RowIndex][field] = value;
        }
    }

    public void Update()
    {
        Adapter.Update(DataTable);
    }

    #region IDisposable

    protected bool Disposed = false;

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected void Dispose(bool disposing)
    {
        if (Disposed)
            return;

        if (disposing)
        {
            if (OwnsConnection && Connection != null)
                Connection.Dispose();
            if (Command != null)
                Command.Dispose();
            if (Adapter != null)
                Adapter.Dispose();
            if (CommandBuilder != null)
                CommandBuilder.Dispose();
            if (DataTable != null)
                DataTable.Dispose();
            Connection = null;
            Command = null;
            Adapter = null;
            CommandBuilder = null;
            DataTable = null;
        }
        Disposed = true;
    }

    #endregion
}

Example Usage 1

string query = "SELECT * FROM TestTable WHERE Id = @FirstValue OR Id = @SecondValue";

using (RecordSet recordset = new RecordSet(connectionString))
{
    recordset.Open(query, new SqlParameter("@FirstValue", 2), new SqlParameter("@SecondValue", 3));
    while (recordset.Read())
    {
        recordset["Value3"] = 123;
    }

    recordset.Update();
}

Example Usage 2

string query = "SELECT * FROM TestTable WHERE Id = @FirstValue OR Id = @SecondValue";

using (SqlConnection connection = new SqlConnection(connectionString))
using (RecordSet recordset = new RecordSet(connection))
{
    recordset.Open(query, new SqlParameter("@FirstValue", 4), new SqlParameter("@SecondValue", 5));
    while (recordset.Read())
    {
        recordset["Value3"] = 456;
    }

    recordset.Update();
}

I'm using SQL Server. Thanks for any tips.

\$\endgroup\$
13
  • \$\begingroup\$ I like the idea of transposing an ADODB recordset to C#... I'll try to take a moment to review your code later. So.. you know both VBA and C# huh... if you're ever looking for an open-source project to contribute to, I know one that could use your help! \$\endgroup\$ Mar 19 '18 at 16:06
  • \$\begingroup\$ @MathieuGuindon: Well, I used to know VB6 pretty good. (Used to write alternating columns for Visual Basic Programmer's Journal.) But that was a long time ago and I've forgotten some things. Don't have much extra time these days but will take a look at the GitHub project. \$\endgroup\$ Mar 19 '18 at 16:18
  • \$\begingroup\$ @Heslacher: Sorry, I've continued to work on the code and found a couple of mistakes. I've stopped editing the question now. \$\endgroup\$ Mar 19 '18 at 16:26
  • 1
    \$\begingroup\$ @JonathanWood the problem with "updates" is just that it won't be clear for late answerers which version should be reviewed. See the In addition paragraph of the posted link. \$\endgroup\$
    – Heslacher
    Mar 19 '18 at 17:08
  • 1
    \$\begingroup\$ If you disagree with the scope of this site, feel free to post a question on Meta. That's where policy is discussed and changed (if required). \$\endgroup\$
    – Mast
    Mar 19 '18 at 17:31
1
\$\begingroup\$

In the constructors

public RecordSet(string connectionString)
{
    if (connectionString == null)
        throw new NullReferenceException("connectionString");
    Connection = new SqlConnection(connectionString);
    OwnsConnection = true;
}

public RecordSet(SqlConnection connection)
{
    if (connection == null)
        throw new NullReferenceException("connection");
    Connection = connection;
    OwnsConnection = false;
}

I would not throw a NullReferenceException but an ArgumentNullException. From the docs

A NullReferenceException exception is thrown when you try to access a member on a type whose value is null.

versus

An ArgumentNullException exception is thrown when a method is invoked and at least one of the passed arguments is null but should never be null.

What makes me wonder is what should happen if connectionString is empty or whitespace.


This looks strange to me as a reader of the code

public void Open(string query, params SqlParameter[] parameters)
{
    if (Disposed)
        throw new NotSupportedException("Cannot reuse RecordSet.");  

If one doesn't dispose the RecordSet it looks ok to use a different query and reuse the object. If you want to state that the object is disposed you should throw an ObjectDisposedException but you should throw it as well from every public method/property.


CommandBuilder isn't really used so you could just remove it.


Classlevel variables should be named using camelCase casing. PascalCase casing should be used for namespaces, classes, properties and methods.


I would like to encourage you to always use braces {} although they might be optional. Its a little bit more to type but the code looks more structured and is IMO easier to read. In addition it helps to prevent hidden and therfor hard to find bugs.

If you want to stick to not using them, its your decision, at least you are consistent in your style.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks for the feedback. Looks like most things you found were minor (which is encouraging). I'll consider the overhead of checking Disposed in every method. I know I've seen classes that can't be re-used. Not sure the best way to approach that. I'm also concerned about what happens if they call Open more than once. As far as SqlCommandBuilder, it performs an important task with the adapter, and it also implements IDisposable. So although I'm pretty sure it's harmless to not dispose it, I decided to for completeness. \$\endgroup\$ Mar 19 '18 at 16:37
  • \$\begingroup\$ @JonathanWood And just what task does CommandBuilder perform? \$\endgroup\$
    – paparazzo
    Mar 20 '18 at 14:43
  • \$\begingroup\$ @paparazzo creating the update commands used by the adapter based on the select query \$\endgroup\$
    – Heslacher
    Mar 20 '18 at 14:44
  • \$\begingroup\$ @paparazzo: It automatically builds the InsertCommand, UpdateCommand, and/or DeleteCommand for the adapter. \$\endgroup\$ Mar 20 '18 at 16:25

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.