views:

2906

answers:

5

Here's my proposed (very simplified to illustrate the problem space) design for a C# console application. The database connections implement IDisposable, and this solution doesn't allow for using the database connection objects. Can someone propose a more correct structure for a console application? This is a problem I need to solve often.

class Program 
{
    SQLiteConnection sourceConnection;
    SQLiteConnection destinationConnection;

    static void Main(string[] args)
    {
        Program shell = new Program();

        // get connection strings from command line arguments
        string sourceConnectionString = shell.getConnectionString(args);
        string destinationConnectionString = shell.getConnectionString(args);

        // call non-static methods that use
        shell.setUpConnections(sourceConnectionString, destinationConnectionString);

        shell.doDatabaseWork();
    }

    private void setUpConnections(string sourceConnectionString, string destinationConnectionString)
    {
        sourceConnection = new SQLiteConnection(sourceConnectionString);
        destinationConnection = new SQLiteConnection(destinationConnectionString);
    }

    private void doDatabaseWork()
    {
        // use the connections here
    }
}

Edit:

Some people can't figure out why I'd want them as member variables. Here's my use case (a little psuedocoded) of what would go in doDatabaseWork:

foreach (Row sourceRow in DBResultSet)
{
  string sourceXml = sourceRow.Columns["MyColumnName"].Value;
  string destinationXML = transformUsingXSLT(sourceXml);
  writeToDestination(destinationXml);
}

See how I'd want to keep these connections open for the life of this loop?

+6  A: 

How about writing a class that implements IDisposable.

Inside your class constructor, you can instantiate your DB connections.

Then inside your IDisposable.Dispose Method, you write your tear down code for closing your DB connections.

Here is a code sample to demonstrate what I mean:

public class DBWrapper : IDisposable
{
    public SqlConnection Connection1 { get; set; }
    public SqlConnection Connection2 { get; set; }

    public DBWrapper()
    {
        Connection1 = new SqlConnection();
        Connection1.Open();
        Connection2 = new SqlConnection();
        Connection2.Open();
    }
    public void DoWork()
    {
        // Make your DB Calls here
    }

    public void Dispose()
    {
        if (Connection1 != null)
        {
            Connection1.Dispose();
        }
        if (Connection2 != null)
        {
            Connection2.Dispose();
        }
    }
}

And then, from within your main method of your Program class:

class Program
{
    static void Main(string[] args)
    {
        using (DBWrapper wrapper = new DBWrapper())
        {
            wrapper.DoWork();
        }
    }
}
Scott Ferguson
Right, but I have two database connections and they need to be able to grab resources from one, transform them, and write to the other one. I don't want to have to reconnect for each read and write. I just want to have a handle to both open, and then be able to perform operations on each as I need. Do I do sourceWrapper.DoWork(destinationWrapper)?
danieltalsky
Try opening 2 or more readers on that connection object and see why this wouldn't work out so well
Chad Grant
I just changed my code sample to be more explicit with 2 Connection objectsInside the DoWork method, you've got access to both SqlConnections, you can do whatever you need, without having to reconnect for each read and write.Basically, you're abstracting away the DB logic into the DBWrapper.DoWork() method, and away from your Program.Main method.
Scott Ferguson
So when you have 10 connections, you gonna add 10 connection member variables? The exercise/sample is contrived I know, not trying to give you a hard time
Chad Grant
@Deviant, the original poster had an example of a source connection, and a destination connection. I can't think of an example where I might need more than 2 connection objects, but if I did, I'd probably have one member variable containing a list of connection objects... but let's not distract further from pattern.How about if the DoWork method was called inside a tight loop that performed 100,000 iterations? Could you see the benefit of not opening and closing the connections inside that loop then?I have used this pattern before with OleDbConnection objects, and noticed a big perf gain. :)
Scott Ferguson
Depends, but I have no fear of Opening Connection objects but I would never create a list of them or make them a member variable ... because that creates a ton of work trying to dispose all that stuff and there would be no reason too, you're not REALLY opening a connection to the DB when you call Open(), connection pooling will return an already opened connection or open one for you if needed.
Chad Grant
Well, fair enough, Deviant, but honestly, I'm posting because I tried that approach first and saw performance degrade so badly over the life of the script that it became unusable. There's 300,000+ records and a total of about 1.8GB of data.
danieltalsky
+2  A: 

Scott's answer is one way to do it. You could also consider using try{} finally instead?

static void Main(string[] args)
{
    Program shell = new Program();

    // get connection strings from command line arguments
    string sourceConnectionString = shell.getConnectionString(args);
    string destinationConnectionString = shell.getConnectionString(args);

    // call non-static methods that use
    shell.setUpConnections(sourceConnectionString, destinationConnectionString);
    try
    {
      shell.doDatabaseWork();
    }
    finally
    {
      if(sourceConnection != null)
        sourceConnection.Dispose();
      if(destinationConnection != null)
        destinationConnection.Dispose();
    }
}
Matthew Flaschen
Why not just use the using keyword for sourceConnection and destinationConnection then?
Brian Rasmussen
Will that work? Answer and show a valid code sample, Brian?
danieltalsky
+2  A: 

Personally, I think you are over thinking this and the code samples in this thread are overly complex imho. I have no idea why people are implementing IDisposable on their Program class either since it's disposed when it exits.

I can't think of a single reason to not use or why you cannot use the using(){} statement.

You want to open a Connection and hold it? Why? All the real connections are behind the scenes in .net connection pooling, so new'ing Connection objects is not a big deal. Just open and close as you need them and connection pooling handles all that behind the scenes.

I edited my example to wrap it in a class so you can have your encapsulation as well.

class Program 
{
    static void Main(string[] args)
    {
        DBWorker worker = new DBWorker();
        worker.DoDatabaseWork();
    }
}

public class DBWorker 
{

    private void DoDatabaseWork()
    {
        using (SQLiteConnection sourceDB = new SQLiteConnection( GetConnectionString() ))
        {
            sourceDB.Open();
            using (SQLiteConnection destDB = new SQLiteConnection( GetConnectionString() ))
            {
                destDB.Open();
            }
        }
    }

}
Chad Grant
I want to be able to have separate helper methods that actually do the reads and writes so I can have the logic that translates the data from one to the other in it's own method. If I don't store the data as a member, then I don't have access to the connections when I need them.
danieltalsky
I added an edit to show why I'd struggle with that code.
danieltalsky
+1  A: 

I think that the best solution is to extract main logic from Program class. The Program class is some kind of starter for primary work. And providing wrappers for SqlConnections is not a good idea indeed, because they are managed resources already, wrapping them is redundant. Thus my solution looks like this:

class ProgramCore : IDisposable
{
    internal ProgramCore(string sourceConnectionString, string destinationConnectionString)
    {
        setUpConnections(sourceConnectionString, destinationConnectionString);
    }

    internal void Execute()
    {
        // do whatever you want
        doDatabaseWork();
        // do whatever you want
    }

    public void Dispose()
    {
        if (_sourceConnection != null)
            _sourceConnection.Dispose();
        if (_destinationConnection != null)
            _destinationConnection.Dispose();
    }

    private void setUpConnections(string sourceConnectionString, string destinationConnectionString)
    {
        _sourceConnection = new SQLiteConnection(sourceConnectionString);
        _destinationConnection = new SQLiteConnection(destinationConnectionString);
    }

    private void doDatabaseWork()
    {
        // use the connections here
    }

    private SQLiteConnection _sourceConnection;
    private SQLiteConnection _destinationConnection;
}

class Program
{
    static void Main(string[] args)
    {
        // get connection strings from command line arguments
        string sourceConnectionString = GetConnectionString(args);
        string destinationConnectionString = GetConnectionString(args);

        using (ProgramCore core = new ProgramCore(sourceConnectionString, destinationConnectionString))
        {
            core.Execute();
        }
    }

    static string GetConnectionString(string[] args)
    {
        // provide parsing here
    }
}
Dmitry Lobanov
Connections are taken from connections pool. Hence if you don't open and close them 1000 times a second, it will not affect your program's performance. But if you need to open and close connections very often, then you should use caching of data inside your program. But this concerns optimization, and you should carefully profile your program in order to optimize it. Remember what Donald Knuth said on optimization: "We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil".
Dmitry Lobanov
Fair enough, but my first strategy was to just open and close the db connections as I needed them. It slowed to a crawl during the life of the script. I didn't profile, but even just grouping the writes into groups of a 1000 per connection made performance manageable.
danieltalsky
Honestly, no one upvoted your strategy, but so far it's the closest to answering my question.
danieltalsky
A: 

Hmm, I see no one has mentioned doing it this way. You don't have to have the variables that are used in the using declared locally.


class Program 
{
    SQLiteConnection sourceConnection;
    SQLiteConnection destinationConnection;

    static void Main(string[] args)
    {
        Program shell = new Program();

        // get connection strings from command line arguments
        string sourceConnectionString = shell.getConnectionString(args);
        string destinationConnectionString = shell.getConnectionString(args);

        using (sourceConnection = new SQLiteConnection(sourceConnectionString))
        using (destinationConnection = new SQLiteConnection(destinationConnectionString))
        {
            shell.doDatabaseWork();
        }
    }

    private void doDatabaseWork()
    {
        // use the connections here
    }
}
Joel Lucsy