2
\$\begingroup\$

I used to use static or singleton for my DAL class. However, I read some articles saying that singleton is evil and should be avoided. Therefore I try to rewrite my code like this:

public class CustomerBLL
{
    private CustomerHelper helper = new CustomerHelper();
    private CustomerDAL dal = new CustomerDAL();

    public void Method1(string param)
    {
        dal.Method1(param);
    }

    public void Method2(string param)
    {
        dal.Method2(param);
    }

    public void MethodA(int param)
    {
        helper.MethodA(param);
    }
}

How do I improve this code? It just doesn't look right.

\$\endgroup\$
6
\$\begingroup\$

I believe that some generalizations are to be made lightly. i.e. stating that Singletons are evil in general is not accurate, as there are some cases where Singleton is a proper (and maybe the only) solution. And yes I do believe that it is one of the overly abused patterns out there. Having gotten that out of the way, I move to answer your question.

From the code sample you submitted, it is very clear that your CustomerBLL has no business logic and merely acts as a Façade on top of the data access and other helper classes. Facades should be stateless by nature.

Having said that, I will offer two choices based on testability and decoupling requirements.

If you want to make you CustomerBLL testable and decouple it from the CustomerHelper and CustomerDAL concretions and substitute them with abstractions, then one good design enhancement would be to inject the CustomerHelper and CustomerDAL dependencies into the CustomerBLL class, which allows you to mock and stub the data access and helper classes and focus on testing other code modules that consume the CustomerBLL class. You acheive that through extraxcting interfaces of course.

Here is an example of how the code would look if we inject the dependencies through a constructor, note that your CustomerBLL Façade will be an instance class i.e. no static methods will be exposed.

public class CustomerBLL
{
    private readonly ICustomerHelper _helper;
    private readonly ICustomerDAL _dal;

    // default constructor initializes the concrete dependencies
    public CustomerBLL()
        :this(new CustomerHelper(), new CustomerDAL())
    {}

    // constructor to inject stubbed dependencies for testing purposes
    public CustomerBLL(ICustomerHelper customerHelper, ICustomerDAL customerDal)
    {
        _helper = customerHelper;
        _dal = customerDal;
    }

    public void Method1(string param)
    {
        _dal.Method1(param);
    }

    public void Method2(string param)
    {
        _dal.Method2(param);
    }

    public void MethodA(int param)
    {
        _helper.MethodA(param);
    }
}

public interface ICustomerDAL
{
    void Method1(string s);
    void Method2(string s);
}

public interface ICustomerHelper
{
    void MethodA(int i);
}

internal class CustomerDAL : ICustomerDAL
{
    public void Method1(string s)
    {
    }

    public void Method2(string s)
    {
    }
}

internal class CustomerHelper : ICustomerHelper
{
    public void MethodA(int i)
    {
    }
}

If on the other hand, testability is not a concern at all, then you can proceed by consuming the CustomerBLL as a Façade in a procedural way through static methods (although I much prefer the testability approach, but it’s not every one’s cup of tea).

Here you notice I removed the private fields that hold references to the Helper and DAL classes, because we want to promote that the CustomerBLL Façade is stateless.

public class CustomerFacade
{
    public static void Method1(string param)
    {
        new CustomerDAL().Method1(param);
    }

    public static void Method2(string param)
    {
        new CustomerDAL().Method2(param);
    }

    public static void MethodA(int param)
    {
        new CustomerHelper().MethodA(param);
    }
}

I have seen and used both approaches, in my humble opinion they both are valid depending on the usage scenario. In general I am in favor of testable design and decoupled modules and that’s why I proposed the first approach.

\$\endgroup\$
5
  • \$\begingroup\$ In the 2nd approach, does that mean CustomerFacade will create an instance for CustomerDAL/CustomerHelper everytime I call a method? \$\endgroup\$ – walterhuang Nov 23 '11 at 2:55
  • \$\begingroup\$ @WalterHuang, yes you instantiate a helper or a DAL class only when needed. This should be fine as long as your Helpers and DALs do not create costly resources in their constructors or keep references (state) to those resources without Disposing them. \$\endgroup\$ – Anas Karkoukli Nov 23 '11 at 5:17
  • \$\begingroup\$ @AnasKarkoukli Could I also make Helper and DAL static? Just like CustomerFacade, they act as containers of methods. Thank you :) \$\endgroup\$ – walterhuang Nov 24 '11 at 2:15
  • \$\begingroup\$ @WalterHuang Man you break my heart :-) can you at least consider the testability approach. Over use of static methods is a symptom of procedural programming and lack of object orientation. Nothing prevents you from using static methods across the board. I just encourage you to explore other ways. \$\endgroup\$ – Anas Karkoukli Nov 24 '11 at 4:22
  • \$\begingroup\$ @AnasKarkoukli I admit that I am a "staticaholic" :P OK, I will try the 1st approach :) thx \$\endgroup\$ – walterhuang Nov 24 '11 at 8:48

Not the answer you're looking for? Browse other questions tagged or ask your own question.