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. enter image description here 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.

• Ben Aaronson: Could you explain what the classes represent conceptually?
• Kirsten: Hi Ben, sales quote, sales order and sales invoice are business terms. For example en.wikipedia.org/wiki/Sales_order They have some common information which is why I thought they should inherit from the same class. Their structure includes a header and a collection of lines, and the lines also have information in common.
• ferada: Pretty sure that BaseSalesHeader should inherit from BaseSalesLine, otherwise it wouldn't compile since the override doesn't match anything.
• Kirsten: Hi @ferada, I must have made a copy error. I have removed all properties from BaseSalesHeader now.
• Gert Arnold: Having a property of type BaseSalesHeader that requires SalesOrder values doesn't make sense. Only the SalesOrder property should be enough. This probably indicates some flaw in the inheritance scheme. Also, this will introduce mapping problems. And that's the problem with reviewing this, we don't see all the moving parts. The EF mappings, including the inheritance implementation, can't be omitted.

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.

• Kirsten: Thankyou Mattias You are right that CloneSalesObject is not just for testing purposes. But why is it a code smell to pass in the ObjectContext? I searched for ICollection vs List and discovered that List is sortable, which is what I want. ~stackoverflow.com/questions/10113244/…
• Kirsten: Thanks also for the points about the possibility of adding null to the lines, and not needing 2 generic parameters.
• Kirsten: I had to keep the CopyOfLines property as I need it in CloneSalesObject
• Mattias Åslund: @kirsten You require a lot of knowledge of how DbContext is implemented to call CloneSalesObject. It is not at all apparent how you get an ObjectContext to pass in.
• Mattias Åslund: @kirsten I'd prefer more business logic instead of CopyOfLines in the entities, that's all.

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

• Kirsten: I added an update at the bottom of my question to show a diagram of the database that gets created. I will study your solution further shortly.
• Kirsten: When I changed my business model to your suggestion and ran it , I got an error that Line and BaseSalesLine had no key defined. Thus I changed them to inherit from BasicBo. Then when I ran the application the BaseSalesHeaders and BaseSalesLines tables were created in my database. This is not what I want.
• Kirsten: Thanks for the comment to initialize the date in the constructor
• radarbob: Sorry for the misleading code, kirsten. When I said BasicBo was not needed I didn't mean the Id property was not needed, but to put it somewhere else. And I removed the [NotMapped] attributes because I was not focusing on Entity Framework, just the inheritance design of those few classes.
• Kirsten: Thanks Bob, If I put [NotMapped] on your BaseSalesHeader Lines property then I get no table for lines. I guess I didn't explain early enough that I wanted a SalesOrderLines table not a BaseSalesLines table.