10

Domain Model I am working on has root aggregate and child entities. Something like the following code:

class Order
{
   IList<OrderLine> Lines {get;set;}
}

class OrderLine
{
}

Now I want my Order to control lines. Something like that:

class Order
{
   OrderLine[] Lines {get;}

   void AddLine(OrderLine line);
}

At this time we are using the following pattern:

class Order
{
   private IList<OrderLine> lines = new List<OrderLine>();
   OrderLine[] Lines {get {return this.lines;}}

   void AddLine(OrderLine line)
   {
      this.orders.Add(line);
   {
}

NHibernate is mapped directly to the lines field.

Now questions...

  • What do you practice in such situations?
  • Does anyone use methods: public IEnumerable GetLines()
  • What are you using as return type for property? May be ReadOnlyCollection or IEnumerable;
  • May be this is not the best place to ask? Suggest please.

Update: Seems IEnumerable wins, however solution still is not perfect...

Mike Chaliy
  • 25,801
  • 18
  • 67
  • 105

5 Answers5

14

I do it like this:

public class Order
{
      private ISet<OrderLine> _orderLines = new HashedSet<OrderLine>();

      public ReadOnlyCollection<OrderLine> OrderLines
      {
          get { return new List<OrderLine>(_orderLines).AsReadOnly(); }
      }

      public void AddOrderLine( OrderLine ol )
      {
          ...
      }
}

Then, offcourse, in the mapping, NHibernate is told to use the _orderLines field:

<set name="OrderLine" access="field.camelcase-underscore" ... >
...
</set>
Frederik Gheysels
  • 56,135
  • 11
  • 101
  • 154
  • With your code following code will fail: order.OrderLines != order.OrderLines; I am not sure if this is bad or not... Anyway thank you. – Mike Chaliy Mar 14 '09 at 22:19
  • That doesn't matter imho, since you shouldn't compare it that way :) – Frederik Gheysels Mar 15 '09 at 08:39
  • Good solution. This helped me out a lot. Thanks! – CalebHC Jun 10 '09 at 20:22
  • Why would you choose to return new instead of caching the readonlycollection in a lazy private field? – smartcaveman Apr 07 '12 at 01:44
  • If you're returning new, I think you'd be better off doing `return new ReadOnlyCollection(_orderLines)` as others suggested. This is because `.AsReadOnly()` returns an object that still has `Add()` (just throwing exceptions when called), while `ReadOnlyCollection` (from System.Collections.ObjectModel) does not, and thus makes sure noone even thinks about using your read only collection incorrectly. – Dav Sep 11 '12 at 09:33
  • Not sure if I'm doing something wrong, but in order to use the `ReadOnlyCollection` with an `ISet` I had to instantiate it like so: `return new ReadOnlyCollection(new List(_orderLines))`. Otherwise I get an error casting from `ISet` to `IList`. – Josh Anderson Jan 04 '13 at 14:48
9

The pattern I use is:

class Order
{
   private List<OrderLine> lines = new List<OrderLine>();

   IEnumerable<OrderLine> Lines { get { return this.lines; } }

   void AddLine(OrderLine line)
   {
       this.orders.Add(line);
   }
}

If you're using NET 3.5 you get all the search functionality you could want for IEnumerable using LINQ, and you hide your collection implementation.

The problem with returning OrderLine[] is that your collection can be modified externally eg: 

Order.Lines[0] = new OrderLine().
gcores
  • 12,376
  • 2
  • 49
  • 45
  • Regarding "Order.Lines[0] = new OrderLine();" it cannot. Each time you access to the Lines new copy of the array issued (I do not like this though...). IEnumerable has lack of list functionality, I mean Count, access by index. Of course, LINQ covers this. Thank you! – Mike Chaliy Mar 14 '09 at 22:27
  • Here, you can still cast the Lines property to a List and modify the collection in that way ... – Frederik Gheysels Mar 15 '09 at 08:39
  • 2
    Yes, but you have to do the casting. It's not so much about protection but about showing what you're allowed to do to the collection. You can return this.lines.AsReadonly() if you want that extra security. – gcores Mar 15 '09 at 11:27
  • I agree with gcores, the key thing is to convey what you are aloud to do via the public interface. If you want to hack around by casting you can do it, but even if you copy it someone could use reflection to get at the underlying list. – Caleb Vear Feb 09 '11 at 02:18
  • @gcores If I do this then I get an error "Could not find a setter for property XXX". Do I need to configure the mapping to use the back field instead of the property? – Rezler Jul 31 '11 at 12:10
  • 1
    @Rezler Yes, you have to do that. – gcores Jul 31 '11 at 17:32
  • @gcores Just one more thing - will the above solution work ok with Linq to NH queries? In the past I've attempted to achieve read only lists through using ReadOnlyCollection, but it didn't play well with Linq to NH. Thanks. – Rezler Aug 01 '11 at 09:22
  • Ideally the Order aggregate should be responsible for creating its OrderLine. As written, you are violating the aggregate's boundary by implying that an external object creates the Order's weak entities. Best pratices would entail passing the ctor args of the OrderLine entity to the AddLine method and instantiating it within that method prior to adding it to the underlying list. (I try to limit my exposure of child entities outside the containing aggregates to interfaces when possible) – smartcaveman Apr 07 '12 at 01:38
3

I expose collections as ReadOnlyCollection and use AddX and RemoveX methods for maintaining the collections. We just switched to 3.5 and I'm thinking about exposing IEnumerable instead. In most cases with NHibernate the child has a reference to the parent so exposing Add and Remove methods allows you to maintain that relationship:

    public void AddPlayer(Player player)
    {
        player.Company = this;
        this._Players.Add(player);
    }

    public void RemovePlayer(Player player)
    {
        player.Company = null;
        this._Players.Remove(player);
    }
Jamie Ide
  • 48,427
  • 16
  • 81
  • 117
  • Yeh, seems IEnumerable beats ReadOnlyCollection... ReadOnlyCollection make API confusing. It still has Add method that will throw exception in runtime... – Mike Chaliy Mar 14 '09 at 22:29
  • 1
    Mike -- I think you're mistaken on this. Returning a List.AsReadOnly() does expose an Add method but ReadOnlyCollection does not. ReadOnlyCollection is in System.Collections.ObjectModel so it's often overlooked. – Jamie Ide Mar 16 '09 at 11:33
  • Funny, yes thats true.. I overlooked it. Thank you very much. – Mike Chaliy May 21 '10 at 14:54
1

If I'm exposing a list which shouldn't be modified, then I use IEnumerable and yield. I find it cumbersome trying to use ReadOnlyCollections in conjunction with NHiberante.

With this approach, you still have the private lines field which gets mapped and populated via NHibernate; however, public access to the collection is performed through iterators. You cannot add to or remove from the underlying list with this Lines property.

For example:

public IEnumerable<OrderLine> Lines {
    get {
        foreach (OrderLine aline in lines) {
            yield return aline;
        }
    }
}
Steve Scheffler
  • 3,706
  • 2
  • 21
  • 22
  • Why not to just "return lines"? You are protecting from casting? Solution with yield means that Count() will require foreach and will load all backed ites (in case of lazy load). – Mike Chaliy Mar 14 '09 at 22:16
  • The IEnumerable combined with the yield allows you to walk the collection without being able to add/remove items. – Steve Scheffler Mar 14 '09 at 22:57
  • Well, IEnumerable all by itself would also prevent you from adding/removing items - so a `public IEnumerable Lines { get { return lines; }}` should suffice. – Dav Sep 11 '12 at 09:30
0

I have spent several days looking for a best approach for readonly lists in NHibernate. This discussion helped me a lot to form the one that fits our project.

There is an approach I started to use:

  1. Backing fields are used to store collections
  2. IEnumerable< T> is used to expose collections to force clients use AddLine() and RemoveLine() methods.
  3. ReadOnlyCollection type is used in addition to IEnumerable.

Code: 

public class Order
{
    private readonly IList<OrderLine> lines = new List<OrderLine>();

    public virtual IEnumerable<OrderLine> Lines
    {
        get
        {
            return new ReadOnlyCollection<OrderLine>(lines);
        }
    }

    public void AddLine(OrderLine line)
    {
        if (!lines.Contains(line))
        {
            this.lines.Add(line);
            line.Order = this;
        }
    }

    public void RemoveLine(OrderLine line)
    {
        if (lines.Contains(line))
        {
            this.lines.Remove(line);
            line.Order = null;
        }
    }
}

public class OrderLine
{
    public Order Order { get; set; }
}
Ilya Palkin
  • 14,687
  • 2
  • 23
  • 36