views:

222

answers:

3

I'm working on a simple, internal ASP.NET app for a small company. I've designed the data access layer so it's database-agnostic.

I have the following:

  • IDataHelper - An interface requiring methods such as FillDataTable(), ExecuteNonQuery(), etc.
  • MySqlHelper - Implements IDataHelper for MySQL, the only database I support so far.
  • Static data access classes - Encapsulate data access methods for different parts of the app. They call methods on an IDataHelper to execute queries.

And finally, I have a static class that creates an IDataHelper for the data access classes to call. In the future it will create the appropriate helper based on the database specified in the config file. For now it's just hard-coded to create a MySqlHelper:

public static class DataHelperContainer
{
    private static IDataHelper dataHelper;

    public static IDataHelper DataHelper
    {
        get { return dataHelper; }
    }

    static DataHelperContainer()
    {
        string connectionString = ConfigurationManager
            .ConnectionStrings["myapp"].ConnectionString;

        // Support for other databases will be added later.
        dataHelper = new MySqlHelper(connectionString);
    }  
}

My questions are:

  1. What should I name this? "DataHelperContainer" doesn't seem right, since that implies some kind of list.
  2. Is this a good or bad design? If it's bad, what are its faults?
  3. Is this similar to any known design pattern? Can it be refactored to conform to one? It seems remotely like a factory, but I'm not sure.

Sorry about the long post and multiple questions. :)

Thanks!

+4  A: 

It looks like the Strategy pattern. With the strategy pattern would would be able to change the underlying functionality during the runtime of the program, and would be able to create new functionality without changing the basic flow of your data layer. [Functions are guarantied via the IHelper interface]

monksy
I see. So the IDataHelper would be like a strategy interface, MySQLHelper would be like concrete strategy class, and DataHelperContainer would be like a context class?
Andy West
Yes, it would be.
monksy
Nice! In that case I'll rename DataHelperContainer to something like DataContext. Thanks for the help.
Andy West
Note that DataContext is the name LINQ2SQL uses to represent databases - in case you want to avoid a name collision. It is a good name though.
Simon Buchan
@Simon: Thanks for the heads-up. I don't foresee using LINQ to SQL in this project so I should be safe. I will definitely keep it in mind for future projects, though.
Andy West
@Simon: Too bad the WPF and LINQ teams didn't compare notes. :)
Josh Einstein
@josh: :facepalm:
monksy
LOL, true. Oh, well. That's what namespaces are for.
Andy West
+1  A: 

For the data access classes it seems like it might be a Data Access Objects (DAO) pattern, but I'm not sure exactly how you are implementing that. Andy West is right you definitely have a Strategy pattern in there.

Brian T Hannan
Yes, that's true. Good observation.
Andy West
+2  A: 
  1. You could name it DataHelperFactory.

  2. It's a good pattern. You definitely don't want to leak things like ConfigurationManager.ConnectionStrings["myapp"].ConnectionString all over the place! A fault is that it's static which makes it difficult to test any code using it.

  3. This is most like the factory pattern or possibly service locator (not strategy).

Currently, your code will look like this:

public class MyClass
{
    public void DoSomething()
    {
        var helper = DataHelperFactory.Create();

        helper.ExecuteNonQuery("some sql");
    }
}

This is not easily testable because you'd have to modify your app.config in order to change what helper you get back in your test. Maybe you want to test what happens when helper.ExecuteNotQuery throws an exception.

Using dependency injection, your class would change to:

public class MyClass
{
    private IDataHelper helper;

    public MyClass(IDataHelper helper)
    {
        this.helper = helper;
    }

    public void DoSomething()
    {
        helper.ExecuteNonQuery("some sql");
    }
}

The trade-off here is that now you have to deal with supplying the IDataHelper dependency in the calling context. This is where IoC containers like Unity, Windsor, and StructureMap enter. This is more complex though and maybe not worth it in your situation.

Using a factory (even static) is great. It allows you to use other patterns like the decorator to add additional behavior. Consider a scenario where you want to sanitize your sql string and make sure nothing bad is sent to your db:

public class SanitizingDataHelper : IDataHelper
{
    private IDataHelper helper;

    public SanitizingDataHelper(IDataHelper helper)
    {
        this.helper = helper;
    }

    public void ExecuteNotQuery(string sql)
    {
        sql = EscapeHarmfulSql(sql);
        helper.ExecuteNonQuery(sql);
    }

    private string EscapeHarmfulSql(string sql)
    {
        ...
    }
}

Your factory can then do something like this:

public class DataHelperFactory
{
    public IDataHelper Create()
    {
        ...

        var helper = new MySqlDataHelper(connectionString);

        return new SanitizingDataHelper(helper);
    }
}
Michael Valenty
Factory is what I thought at first (as you can see in my post). I made it static because I'll only ever have one of these. What about it makes it hard to test? I feel like this is a code smell, but I don't know what to do about it.
Andy West
Thanks for the additional details. I'm learning a lot.
Andy West
Its not an factory pattern. A factory pattern would make it easy to change the helpers used by only specifiying the db via a string. See: http://www.dofactory.com/Patterns/PatternFactory.aspx for the factory pattern.
monksy
Yes, it is exactly the factory pattern. The factory pattern is a creational design pattern used in software development to encapsulate the processes involved in the creation of objects. http://en.wikipedia.org/wiki/Factory_pattern
Michael Valenty
Maybe the confusion here is that it's the factory pattern, not the factory method pattern. I do not have method to which I pass parameters that determine the type of object that's created. But from what I can tell, it's still a factory. Of course, I could be wrong - I'm still figuring this stuff out.
Andy West
This is not the first time strategy and factory patterns have been confused. See here: http://coding.derkeiler.com/Archive/General/comp.object/2005-06/msg00170.html
Andy West