Good Coding Practices

Question

I am working on my 2nd project with Entity Framework and I would like to read some opinions about if the code below its good coding practice of not.

My proposed architecture is this:

So, the website calls the business logic, business rules are evaluated there. THen it calls the DALFacade, its just that its invisible for the upper layers whether if we are accessing db2 or sql server.

The EF DAL, its the one in charge of talking with database, CODE FIRST Approach. The entities class library is the project with the poco classes. Utilities its self explanatory, I might put there code like reading properties from active directory.

SO here is my code, I simplified it as much as I could

OnePage:

protected void BtnSubmitRequestClick(object sender, EventArgs e)
    {
        var ecoBonusWorkflow = new EcoBonusWorkflow
                                   {
                                       IsOnHold = true
                                   };

        BusinessLogic.EcoBonusRequestSave(ecoBonusWorkflow);
    }

BUsiness Logic:

public static class BusinessLogic
    {
        public static void EcoBonusRequestSave(EcoBonusWorkflow ecoBonusWorkflow)
        {
            if (true)
            {
                DalFacade.EcoBonusRequestSave(ecoBonusWorkflow);
            }
        }
    }

DALFacade:

 public static class DalFacade
    {
        private  static readonly UnitOfWork UnitOfWork = new UnitOfWork();

        public static void EcoBonusRequestSave(EcoBonusWorkflow ecoBonusWorkFlow)
        {
            UnitOfWork.EcoBonusWorkflowRepository.InsertEcoBonusWorkflow(ecoBonusWorkFlow);
        }
    }

Then in the EF Dal class library I use the traditionary EF 4.1 Code First Approach. I used also Repository Pattern and Unit of Work.

Any observation is welcomed

Answer 1

The inclusion of 'utilities' seems very strange to me. Everything else is decently well named with a pretty specific setup of what exactly it should do, then you have this vaguely named block of 'utilities'.

This isn't any particular coding standard, but I would avoid incorporating 'utilities' into your top level architecture as much as you possibly can. Every project has some folder like that named 'helpers' or 'utilities' and it ends up being a dump for a large amount of miscellaneous code that doesn't seem like it immediately fits anywhere else. This is exactly the way technical debt and spaghetti code begins.

It will probably end up happening no matter what you present unless you are the only developer on the project, but nevertheless I'd avoid legitimizing it.

Also, the inclusion of static classes in your quick-and-dirty alarms me. Aside from the fact that static code hurts testability, the Entity framework is not thread-safe. It is up to you to use it in a thread-safe way, so if you have any multithreading or co-processing going on at all your DALFacade is going to randomly throw a ton of errors and be a gigantic headache.

I also don't see any allowance for dependency injection in your design, which again goes to how testable the code is.

EDIT: OK, these were not concessions for Stack, so it's time to introduce the idea of dependency injection.

Basically, any time you make something directly depend on something else, such as when you invoke a static class, you are tying these two things together. One part is no longer replaceable without also modifying or replacing the other part. This is A Bad Thing, because it makes refactoring and changes a very large problem. What you want to do instead is to 'loosely couple' these dependencies so you can change parts out without breaking everything. In C#/.NET, you mostly do this with interfaces.

So instead of having "DALFacade" and passing it around directly, you would instead have something to the effect of:

interface IDalFacade
{
    int DoAThing();

    bool DoAnotherThing();
}

class DalFacade : IDalFacade
{
    public int DoAThing()
    {
        return 0;
    }

    public bool DoAnotherThing()
    {
        return false;
    }
}

You may then use it in your business logic like so:

class BusinessLogic : IBusinessLogic
{
    public IDalFacade DalFacade { get; private set; }

    public BusinessLogic() : this(new DalFacade())
    {}

    public BusinessLogic(IDalFacade dalFacade)
    {
        this.DalFacade = dalFacade;
    }

    public void LogicTheThings()
    {
        this.DalFacade.DoAThing();
    }
}

It now no longer matters if DalFacade gets completely trashed or if all of a sudden you need two of them for different things. Or even, truly, if a DalFacade is even passed in. As long as it fulfills the IDalFacade contract, it's perfectly fine. You can still call it with no parameters (it just assumes a specific implementation), but even this is not ideal. Ideally you would want to use a dependency injection library, such as Ninject or StructureMap, which would allow you to make any specific changes even more easily.

This is also useful when attempting to unit test your code, since, as I said, you don't even need an actual DalFacade to do things. You can simply mock out something that fills the IDalFacade contract (using something like RhinoMocks), and you can then test all your code totally separately from anything else in your project.