Sales order domain model with Entity Framework

Question

I am wondering whether the following inheritance pattern is a good idea. I noticed that Sales Orders, Sales Quotes and Sales Invoices all follow a similar Master/ Detail pattern ( i.e a Header with Lines ) so I wanted to be able to inherit from base classes that encapsulated their common properties.

I will start with my test to explain it.

[TestClass]
    public class UnitTest1
    {
        [TestMethod]
        public void TestCreateSalesOrderFromQuote()
        {
            using (var ctx = new CrmDbContext())
            {
                var customer = new Customer {Name = "test"};     
                ctx.Customers.Add(customer);

                var quote = new SalesQuote {Customer = customer, CreatedOn = DateTime.Now};
                quote.Lines.Add(new SalesQuoteLine
                {
                    Description = "Box of widgets",
                    Price = (decimal) 100.50
                });
                ctx.SalesQuotes.Add(quote);

                var oc = ((IObjectContextAdapter) ctx).ObjectContext;
                var order = Functions.CloneSalesObject<SalesOrder, SalesOrderLine>(oc, quote) as SalesOrder;
                Assert.AreEqual(order.Customer.Id, quote.Customer.Id);
                Assert.AreEqual(order.Lines[0].Description, quote.Lines[0].Description);
            }
        }
    }

The function used in the test

public static BaseSalesHeader CloneSalesObject<T1, T2>( ObjectContext oc, BaseSalesHeader baseSale) where T1 : BaseSalesHeader where T2 : BaseSalesLine
        {
            var obj = oc.CreateObject<T1>();
            obj.CreatedOn = DateTime.Now;
            obj.Customer = baseSale.Customer;

            foreach (var line in baseSale.CopyOfLines)
            {
                var newLine = oc.CreateObject<T2>();
                // clone properties needed from BaseSalesLine
                newLine.Price = line.Price;
                newLine.Description = line.Description;

                obj.AddLine(newLine);
            }
            return obj  ;
        }

Next the context

public class CrmDbContext : DbContext
    {

        public CrmDbContext()
            : base("name=ConnectionString")
        {
        }

        public DbSet<ModuleInfo> ModulesInfo { get; set; }
        public DbSet<SalesOrder> SalesOrders { get; set; }
        public DbSet<SalesQuote> SalesQuotes { get; set; }
        public DbSet<Customer> Customers { get; set; }

        protected override void OnModelCreating(DbModelBuilder modelBuilder)
        {
            base.OnModelCreating(modelBuilder);
            modelBuilder.Conventions.Remove<OneToManyCascadeDeleteConvention>();
            modelBuilder.Entity<SalesOrder>().HasRequired(c => c.Customer);
            modelBuilder.Entity<SalesQuote>().HasRequired(c => c.Customer);
            modelBuilder.Entity<SalesOrder>().HasMany(p => p.Lines).WithRequired(t => t.SalesOrder).WillCascadeOnDelete(true);
            modelBuilder.Entity<SalesQuote>().HasMany(p => p.Lines).WithRequired(t => t.SalesQuote).WillCascadeOnDelete(true);
        }
    } 

and now the business classes

public abstract class BaseSalesHeader : BasicBo
    {
        public DateTime CreatedOn { get; set; }
        public virtual Customer Customer { get; set; }
        public string CustomerReference { get; set; }
        [Notmapped]  // needed to avoid a BaseSalesHeader being created
        public abstract List<BaseSalesLine> CopyOfLines { get; }
        public abstract void AddLine(BaseSalesLine line);
    }
public abstract class BaseSalesLine : BasicBo
    {
        [NotMapped] // needed to avoid a BaseSalesLine table being created
        public abstract BaseSalesHeader SalesHeader { get; set; }
        public string Description { get; set; }
        public decimal Price { get; set; }
    }
public abstract class BasicBo
    {
        public virtual int Id { get; set; }
    }
public class Customer : BasicBo
    {
        public string Name { get; set; }
    }
public class SalesOrder : BaseSalesHeader  
    {
        public SalesOrder()
        {
            Lines = new List<SalesOrderLine>();
        }
        public virtual List<SalesOrderLine> Lines { get; set; }

        public override List<BaseSalesLine> CopyOfLines => Lines.Cast<BaseSalesLine>().ToList();
        public override void AddLine(BaseSalesLine line)
        {
            Lines.Add(line as SalesOrderLine);
        }
    }
public class SalesOrderLine : BaseSalesLine
    {
        public virtual SalesOrder SalesOrder { get; set; }

        public override BaseSalesHeader SalesHeader
        {
            get { return SalesOrder; }
            set { SalesOrder = (SalesOrder) value; }
        }
    }
public class SalesQuote : BaseSalesHeader 
    {
        public SalesQuote()
        {
            Lines = new List<SalesQuoteLine>();
        }

        public List<SalesQuoteLine> Lines { get; set; }

        public override List<BaseSalesLine> CopyOfLines => Lines.Cast<BaseSalesLine>().ToList();
        public override void AddLine(BaseSalesLine line)
        {
            Lines.Add(line as SalesQuoteLine);
        }
    }
public class SalesQuoteLine : BaseSalesLine
    {
        public virtual SalesQuote SalesQuote { get; set; }
        public override BaseSalesHeader SalesHeader
        {
            get { return SalesQuote; }
            set { SalesQuote = (SalesQuote)value; }
        }
    }

My test passes, but is my idea sound? [Update1] Here is a database diagram of the database I get. The reason I have the [NotMapped] attribute is to prevent Entity Framework from creating the base tables. I want TPT not TPH

Thus the purpose of the base classes are for code re-use.

Answer 1

If CloneSalesObject() is foremost a helper function for testing purposes (which I don't think it is), the entire polymorphism is a bad fit because it only helps to shorten testing code.

If on the other hand CloneSalesObject() is part of the business logic, it is a code smell that you need to pass in ((IObjectContextAdapter) ctx).ObjectContext.

It is also strange that you need to specify both SalesOrder and SalesOrderLine as generics to the function, as SalesOrder can only have SalesOrderLine.

It is pretty restrictive that derivatives of BaseSalesHeaders only exposes copies of its lines. That prevents more advanced business logic to do all sorts of manipulations in the future.

AddLine() is another method that shouldn't be needed, but as it is implemented it is perfectly ok to add null to the array. If you try to add an object of the wrong type that is also ok, but null will be added.

All implementations of entities for DbContext I've seen before expose child collections as virtual ICollection<T> and parents as virtual NameOfParentClass.

I think my implementation of an entity would rather look like

public abstract class BaseSalesHeader : BasicBo
{
    public DateTime CreatedOn { get; set; }
    public string CustomerReference { get; set; }

    public virtual Customer Customer { get; set; }
}

public class SalesOrder : BaseSalesHeader
{
    public SalesOrder()
    {
        Lines = new HashSet<SalesOrderLine>();
    }

    public virtual ICollection<SalesOrderLine> Lines { get; set; }
}

... and then I'd write more verbose cloning code in the business logic.

Answer 2

I am wondering whether the following inheritance pattern is a good idea

• Most of the abstract and virtual property declarations are not necessary. Subclasses of the bases are effectively only renaming the base class; the extensions do not add anything to, for example, make SalesQuote anything more than what the base is already. Pushing implementation to the subclasses by declaring base stuff as abstract is just illusion. There is no usable, practical, extended, difference between the bases and the subclasses.

• A base class should declare all it's constituent base parts. There are base-class references in sub-classes. Therefore these references can easily go back into the base class instead.

• "A SalesQuote is a BaseSalesHeader" does not make sense for me. Ditto for SalesOrder

  • BaseSalesHeader sounds like the header of a report to me. SalesQuote sounds like a "sales quote for a customer" and so a SalesQuote would contain a BaseSalesHeader; not "sales quote is a sales header". However, if it reflects your "business domain semantics" then ignore this comment!
  • This needs a different notional base class maybe? Prospect ?

• BasicBo

  • With only a single member this class seems to have no reason to exist.
  • It really does not help define the sales model domain. Id is merely a technical implementation detail.
  • The name is inconsistant. All the other bases are "BaseXXXX"

• BaseSalesHeader.CopyOfLines

  • "Lines" is declared in a sub-class. Assuming CopyOfLines is a copy of Lines then Lines should be declared in the base as well. As it is, it's like the base class is inheriting from the sub-class; that's just totally backwards.
  • Without base implementation a sub-class can make this anything it wants.
  • Not needed. Just have Lines return a copy of itself.

• SalesOrderLine : BaseSalesLine

  • The structure of a "Line" is is not actually extended in the subclass so there is no point in the subclass.
  • If Line was it's own class then this becomes really clear. See below.

---

Break out a Line class

public class Line {
    // setting should be done via a constructor.
    public string Description {get; set;}
    public decimal Price {get; set;}
}

then:

public class BaseSalesLine {
    // setting properties should be done via constructor
    public BaseSalesHeader SalesHeader { get; set; }
    public Line SalesLine {get; set;}

    // the only point of this class is to add in a BaseSalesHeader.
    // but is it needed? BaseSalesHeader clients have a that BaseSalesHeader, 
    // which in turn has the list of Lines.
}

public class SalesOrderLine {
    // Everything in this class just "renames" things already in the base class
    // BaseSalesHeader has the functionality.
    // There is no added value in this class. Remove it.
}

public class BaseSalesHeader  {
    public List<Line> SalesLines {get; set; }

    // This property's class already contains a reference to a
    // BaseSalesHeader. But THIS is a BaseSalesHeader.
    // The AddLine method gets confusing (see below)
    public List<BaseSalesLine> SalesLines {get; set;}
}

public class SalesOrder : BaseSalesHeader {

    // delete this property. The base contains a List<Line> which needs 
    // no subclassing. It is a fundamental "sales domain" data structure.
    public List<SalesOrderLine> Lines {get; set;}
}

Base classes need some implementation

Put implementation in base classes. That's what abstract classes are for.

BaseSalesHeader

public abstract class BaseSalesHeader : BasicBo {
    private List<BaseSalesLine> lines {get; set;}  // or should this be List<Line> ?
    public List<BaseSalesLine> Lines {get { return this.lines;} }

    // The parameter object has reference to BaseSalesHeader(s)
    // But we're in the BaseSalesHeader class here. Sounds like a circular
    // reference situation. I'm confused.
    public void AddLine(BaseSalesLine line){
        if(line == null)  return;

        Lines.Add(line);
    }

    // this is better set in a constructor. After all that is when
    // this thing is created.
    public DateTime CreatedOn {get; set {this.createdOn = DateTime.Now;}}
}

Now BaseSalesHeader subclass implementation is superfluous

public class SalesOrder : BaseSalesHeader  
{
    public SalesOrder()
    {
        // The base class takes are of this.
        Lines = new List<SalesOrderLine>();
    }

    // declared in the base class
    public  List<SalesOrderLine> Lines { get; set; }

    // implemented in the base class. This override is redundant.
    //public override List<BaseSalesLine> CopyOfLines => Lines.Cast<BaseSalesLine>();
    public override void AddLine(BaseSalesLine line)
    {
        Lines.Add(line as SalesOrderLine);
    }
}

---

New Inheritance Picture

public class SalesOrder : BaseSalesHeader {
    // add SalesOrder unique stuff here.

    // we've inherited a sales lines list and 
    // the method to add more lines.
}


// There are no BaseSalesLine sub classes
// All that functionality is encapsulated in BaseSalesHeader