Code Reuse Verses Database Access Duration

Question

The Problem

I have some code which I've coded in such a way to enable high maintainability and code re-usability. I am concerned about a particular piece of code and I would like a professional opinion on if this code will crumble under high stress.

The Codez ##

public abstract class PlexxisDataTransferObjects : PlexxisDatabaseRow
    {
        //static methods
        public static List<PlexxisDatabaseRow> GetAll();

        //Constructors
        public PlexxisDataTransferObjects(){ }

        //Methods
        public abstract bool Insert(OracleConnection Conn);
        public abstract bool Update(OracleConnection Conn);
        public abstract bool Delete(OracleConnection Conn);

        public bool Insert()
        {
            using (var Conn = new OracleConnection(ConnectionString))
            {
                Conn.Open();
                return Insert(Conn);
            }
        }
        public bool Update()
        {
            using (var Conn = new OracleConnection(ConnectionString))
            {
                Conn.Open();
                return Update(Conn);
            }
        }
        public  bool Delete()
        {
            using (var Conn = new OracleConnection(ConnectionString))
            {
                Conn.Open();
                return Delete(Conn);
            }
        }
    }


    //Data Transfer Objects
    public sealed class Apps : PlexxisDataTransferObjects
    {
        //Static Methods
        public override static List<PlexxisDatabaseRow> GetAll()
        {
            List<PlexxisDatabaseRow> collection = new List<PlexxisDatabaseRow>();
            using (var Conn = new OracleConnection(ConnectionString))
            {
                using (var Command = new OracleCommand("select * from APPS", Conn))
                {
                    Conn.Open();
                    using (var reader = Command.ExecuteReader(CommandBehavior.CloseConnection))
                        while (reader.Read())
                            collection.Add(new Apps(reader));
                }
            }
            return collection;
        }

        //Fields
        public int AppId;
        public string AuthKey;
        public string Title;
        public string Description;
        public bool isClientCustomApp;

        //Constructors
        public Apps() : base () { }
        public Apps(OracleDataReader reader) : base ()
        {
            if (reader["APP_ID"] != DBNull.Value)
                this.AppId = Convert.ToInt32(reader["APP_ID"]);

            if (reader["AUTH_KEY"] != DBNull.Value)
                this.AuthKey = Convert.ToString(reader["AUTH_KEY"]);

            if (reader["TITLE"] != DBNull.Value)
                this.Title = Convert.ToString(reader["TITLE"]);

            if (reader["DESCRIPTION"] != DBNull.Value)
                this.Description = Convert.ToString(reader["DESCRIPTION"]);

            if (reader["IS_CLIENT_CUSTOM_APP"] != DBNull.Value)
                this.isClientCustomApp = Convert.ToBoolean(reader["IS_CLIENT_CUSTOM_APP"]);
        }

        //Methods
        public override bool Insert(OracleConnection Conn)
        {

            string sql = string.Empty;
            sql += "INSERT INTO APPS (APP_ID, AUTH_KEY, TITLE, DESCRIPTION, IS_CLIENT_CUSTOM_APP)";
            sql += "VALUES(:appid, :authkey, :title, :description, :iscust)";

            using (var Command = new OracleCommand(sql, Conn))
            {
                AppId = GetId();
                Command.Parameters.Add(":appid", OracleDbType.Int32).Value = AppId;
                Command.Parameters.Add(":authkey", OracleDbType.Varchar2).Value = AuthKey;
                Command.Parameters.Add(":title", OracleDbType.Varchar2).Value = Title;
                Command.Parameters.Add(":description", OracleDbType.Varchar2).Value = Description;
                Command.Parameters.Add(":iscust", OracleDbType.Int32).Value = Convert.ToInt32(isClientCustomApp);

                return Convert.ToBoolean(Command.ExecuteNonQuery());
            }
        }
        public override bool Update(OracleConnection Conn)
        {
            string sql = string.Empty;
            sql += "UPDATE APPS SET ";
            sql += "AUTH_KEY = :authkey, TITLE = :title, DESCRIPTION = :description, IS_CLIENT_CUSTOM_APP = :iscust ";
            sql += "WHERE APP_ID = :appid";

            using (var Command = new OracleCommand(sql, Conn))
            {
                Command.Parameters.Add(":authkey", OracleDbType.Varchar2).Value = AuthKey;
                Command.Parameters.Add(":title", OracleDbType.Varchar2).Value = Title;
                Command.Parameters.Add(":description", OracleDbType.Varchar2).Value = Description;
                Command.Parameters.Add(":iscust", OracleDbType.Int32).Value = Convert.ToInt32(isClientCustomApp);
                Command.Parameters.Add(":appid", OracleDbType.Int32).Value = AppId;

                return Convert.ToBoolean(Command.ExecuteNonQuery());
            }
        }
        public override bool Delete(OracleConnection Conn)
        {
            string sql = string.Empty;
            sql += "DELETE FROM APPS ";
            sql += "WHERE APP_ID = :appid";

            using (var Command = new OracleCommand(sql, Conn))
            {
                Command.Parameters.Add(":appid", OracleDbType.Int32).Value = AppId;
                return Convert.ToBoolean(Command.ExecuteNonQuery());
            }
        }
    }

What Am I looking at?

What concerns me the most is the Insert, Update and Delete method in the abstract class calling the Insert, Update and Delete in the concrete class.

I've done it this way so that I could enable transactions if necessary by opening a connection and explicitly starting a transaction, send the transaction and still have the objects do what they need to do; furthermore, if I had to explicitly rewrite the 3 methods for 40 or so classes, it could become quite cumbersome.

However, I'm concerned that by opening the connection early that I might be holding up the database. I don't know how much input data that might be being updated at any given time. In this situation I have two major thoughts, I can either make the insert, update and delete abstract in the abstract class and implement them while explicitly opening the connection immediately before the Command.ExecuteNonQuery() or I can leave it how it is now.

What would I like from you?

First and foremost, your opinion on the situation. Secondly pointing out any pitfalls behind the logic or any bad coding that you happen to spot would also be very helpful.

Answer 1

I think this might be worthwhile to investigate the unit-of-work pattern.

To me it looks like you're already using the active-record pattern, however I find it problematic (from a separation of concerns and dependency perspective) that your class definitions are hard-coded to be dependent on oracle, which means the code that uses your DTOs must also be dependent on oracle. I'm not suggesting this is a problem in case you want to switch your database, I am saying that it is best practice to have a very decoupled system, both for understanding and unit-testing.

Database agnostic code

class Application
{
    public int ID { get; set; }
    public string AuthKey { get; set; }
    // and so on
}

interface IApplicationRepository
{
    IEnumerable<Application> GetAll();
    void Update(Application app);
    void Delete(Application app);
    void Insert(Application app);
}

interface IUnitOfWork : IDisposable
{
    IApplicationRepository Applications { get; }
    void Commit();
}

Consuming code

void SaveButton_Click(object sender, EventArgs e)
{
    // this could be resolved by dependency injection, this would know about Oracle
    using (var uow = UnitOfWorkFactory.Create()) 
    {
        uow.Applications.Insert(new Application { AuthKey = "1234" });

        // you may have other repo that have work done in the same transaction / connection

        uow.Commit();
    }
}

If you look at all the code that is written above, there is no mention of Oracle, or even of a concept of connections or transactions. You have this abstraction called UnitOfWork which behind the scenes manages state for your application. The repository works with plain classes. This type of code is easy to mock and write tests for. This is huge for maintainability.

Database specific code

class OracleApplicationRepository : IApplicationRepository
{
    public readonly OracleDbConnection _dbConnection;

    public OracleApplicationRepository(OracleDbConnection dbConnection)
    {
        _dbConnection = dbConnection;
    }

    IEnumerable<Application> GetAll()
    {
        // up to you, the viewer
        throw new NotImplementedException();
    }

    void Update(Application app)
    {
        // up to the viewer
    }

    void Delete(Application app)
    {
        // up to the viewer
    }

    void Insert(Application app)
    {       
        using (var command = _dbConnection.CreateCommand())
        {
            // or whatever the syntax is
            command.Parameters["AuthKey"] = app.AuthKey;

            command.ExecuteNonQuery();
        }
    }
}

class OracleUnitOfWork : IUnitOfWork
{
    private readonly OracleDbConnection _dbConnection;

    public OracleUnitOfWork(string connectionString)
    {
        _dbConnection = new OracleDbConnection(connectionString);
    }

    public IApplicationRepository Applications 
    {
        get
        {
            // this could be lazy loaded instead of making new instances all over the place
            return new OracleApplicationRepository(_dbConnection); 
        }
    }

    public Dispose()
    {
        // close the connection and any transactions
        _dbConnection.Dispose();
    }
}

Answer 2

Opening and closing a database connection for each CRUD operation will hurt if you are looping over a large set of data. Also, you should use try,catch,and finally. You can't guarantee the database will be up and it will cause an exception to be thrown.