5
\$\begingroup\$

I'm having trouble testing some code. It's laid out in such a way that the business logic relies on the persistence layer.

Some classes require that an object be saved to a database. Some classes re-read the object they were working to get the "original" version of it. All of this tends to happen in what's been labeled the business logic layer.

This a WCF application where the business logic layer manipulates objects from the DAL

Code sample below.

public class WCFService : IWCFService
{

    public OrderHeader GetOrder(int id)
    {
        var om = new OrderManager();
        return om.Read(id);
    }

}
[DataContract]
public class OrderHeader
{
    [DataMember]
    public int id { get; set; }

    public OrderHeader Read(int id)
    {
        // ADO.NET calls 
        return new OrderHeader();
    }

    public void CreateUpdate()
    {
        // ADO.NET calls
        this.id = GetSqlOutputParam();
    }

    private int GetSqlOutputParam() 
    { 
        // get the identity supplied by the executed stored proc 
    }
}
public class OrderManager
{

    public OrderHeader Read(int id)
    {
        OrderHeader oh = new OrderHeader();
        return oh.Read(id);
    }

    public bool SaveOrder(OrderHeader order)
    {
        // do lots of complex stuff
        order.CreateUpdate(); // update here because some other class is going to want to do a fresh read
        // instantiate other "manager" classes
        order = order.Read(order.id); // make sure we get the latest record from the database in case other manager classes changed my order 
        // do more complex stuff
        order.CreateUpdate();

        return true;

    }
}

Firstly and unfortunately, there isn't an immediate plan to replace the entity specific ADO.NET with an ORM.

I would like to start by making the code a little more testable by removing all persistence calls from the business logic layer, or separating the actual logic away from these calls while keeping them in the same layer. Instead of update() and read() littered throughout those manager classes, the object would be passed around as much as needed.

To give some context, the code suffers from a lot of persistence related bugs. I feel like removing persistence from the equation would make matters easier to deal with.

Edit: I miscommunicated the intent of the GetSqlOutputParam() method. It's not just returning a const, it is pulling it from an executed SqlCommand.

\$\endgroup\$
  • \$\begingroup\$ The ADO.NET calls could very well be worth reviewing as well, feel free to edit them in! :) \$\endgroup\$ – Mathieu Guindon May 8 '14 at 23:28
3
\$\begingroup\$

Firstly and unfortunately, there isn't an immediate plan to replace the entity specific ADO.NET with an ORM.

That's fine.

This is more questionable:

private int GetSqlOutputParam() { return 1; }

It would be much clearer to declare this as a descriptively named constant near the top of the class (being class-level scoped):

private const int defaultEntityId = 1;

But that's far from being what's making that code hard to test.

I would like to start by making the code a little more testable by removing all persistence calls from the business logic layer, or separating the actual logic away from these calls while keeping them in the same layer. Instead of update() and read() littered throughout those manager classes, the object would be passed around as much as needed.

A class such as an OrderHeader exists to hold data. Nothing less, but also and more importantly nothing more. It's a Plain Old CLR Object, a POCO. You hit the nail on the head, that's exactly what you need to do. Move the ADO.NET code into its own class.

As much as I dislike this pattern when it's used on top of Entity Framework, if I had to refactor some ADO.NET code into its own class, I'd probably write something like a repository:

DI and repository pattern

This blog post (click the diagram) explains how dependency inversion enables testing.

(I am not the author of the blog this link points to, it's pure coincidence that this guy's name is Mat. I'm not this Mat's Mug.)

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for the response! The repository pattern seems to be a good possibility to consider. I have to say I miscommunicated the intent of that GetSqlOutputParam() method. It's not just returning 1 const, it is pulling it from an executed stored procedure \$\endgroup\$ – xerosis May 9 '14 at 9:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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