ADO.NET transaction handling

Question

Please let me know your feedback around handling ADO.NET connection and transaction, especially when it comes the catch block. Any way I could improve?

public static bool FileBulkLoad(string filePath, string tableName, string fieldSeparator, int numberOfSourceColumns)
{
    using (var connection = new SqlConnection(GetConnectionString()))
    {
        connection.Open();
        using (var transaction = connection.BeginTransaction(IsolationLevel.ReadCommitted))
        {
            try
            {
                foreach (var fileData in Helpers.StreamFile(filePath, tableName, fieldSeparator, numberOfSourceColumns))
                {
                    // BulkInsert uses SqlBulkCopy to insert data.
                    Helpers.BulkInsert(fileData.Data, tableName, connection, transaction);
                }

                transaction.Commit();
                return true;
            }
            catch (Exception)
            {
                transaction.Rollback();
                return false;
            }
        }
    }
}

Answer 1

FileBulkLoad isn't an ideal name for a method - it's a noun, and nouns should be used for naming types - method names should be verbs, because they do something.

The fact that it's static is also worrying - less so than that static Helpers, but worrying nonetheless: you're coupling your application logic with a specific data access strategy, which means you have no way of writing a unit test for your application logic without actually hitting the database and, in this case, the file system.

Consider this alternative:

public interface IFilePersistenceService
{
    bool LoadFromDelimitedFile(string path, string separator, int columns);
}

Now you can implement this interface with a class that will use Helpers.BulkInsert with ADO.NET, or one that uses a UnitOfWork and Entity Framework, or NHibernate, or whatever: the point is that bulk insert is a mere implementation detail that the rest of your code doesn't need to care about - that's why having it in the method's name doesn't really bring anything to the table.

Your client code would be written against this IFilePersistenceService abstraction, and then you could inject a fake implementation of that interface into your application logic, and write tests that don't hit the file system or a database.

---

I said Helpers was worrisome; the name doesn't convey anything but "stuff that serves some purpose, somewhere" - after a while it becomes a dumping ground for any code that doesn't quite belong elsewhere, or for anything that you need to access from anywhere - and that's how spaghettification begins.

Consider killing that (static?) class, with fire.

---

The implementation itself isn't half bad: you are disposing of disposables, using using blocks - it doesn't get any better than that IMO.

The problem is at a higher level: you're returning true on success, false on failure - and once the client code knows the operation failed, it doesn't know anything about the failure, that was all swallowed by the called method.

I'd much rather return void and have code that either works or blows up.

The Boolean return value is more hurtful than anything, it's the client code that knows how to handle an exception (e.g. display a message to the user about a permission issue, or email tech support with exception details, whatever): it's the client code that should wrap this method call with a try/catch block. Or the method should log the exception details and then rethrow.

---

I would expect a "load" method to fetch data from a data source, and return that data to the application. Perhaps this would be more appropriate:

public interface IFilePersistenceService
{
    void ImportFile(string path, string separator, int columns);
}

Right? You're importing data into a database, from a file.

Answer 2

It looks much cleaner with the TransactionScope.

using (TransactionScope scope = new TransactionScope())
using (var connection = new SqlConnection(GetConnectionString()))
{
    connection.Open();
    try
    {
        foreach (var fileData in Helpers.StreamFile(filePath, tableName, fieldSeparator, numberOfSourceColumns))
        {
            // BulkInsert uses SqlBulkCopy to insert data.
            Helpers.BulkInsert(fileData.Data, tableName, connection, transaction);
        }

        scope.Complete();
        return true;
    }
    catch (Exception)
    {
        return false;
    }
}

You should also read this: Is it possible to use System.Transactions.TransactionScope with SqlBulkCopy?

The last parameter is unnecessary.

---

Exception handling

Are you sure you want to ignore the exception? I guess you log it somehow, do you?