I wouldn't say it's a messed-up design. I think it may be a little overly complex in some areas.
These part-whole hierarchies are always a bit of a tough nut. I've never been a fan of an explosion of leaf classes, which is kinda what you have here. I will readily concede that a use-case-driven design could lead to this, and there's no real good alternative. You're either checking against a ton of different types, or you're checking against some kind of type property. Which is better? OO design will say different types - but OO design will also say that you should include methods in the base class so that you can treat the subtypes the same.
If you look at the GoF Composite pattern - which is what most people would probably say you should be using - the issues with the pattern center around what you're dealing with. If you include a child collection at the base level, then you have to define behaviors for types that, by definition, will never have any children. You also potentially incur an object graph overhead, since said child collection will exist at every level, whether you need it or not.
(OK - enough of this theory crap. Let's get to some real advice, huh? ![Smile [:)]](/emoticons/emotion-1.gif)
)
I don't see a need for GroupSubItem, since all it seems to do is expose a property to the actual object you care about. Since the items are, by this definition, still editable within this context, GroupSubItem seems to be getting in the way.
One possible simplification I see is to take your collection-based line items and combine them into one class. Realizing that BundleLineItem and GroupLineItem aren't the same thing, when you boil it down, they aren't all that different. BundleLineItems contain non-editable (and, in some cases, non-viewable) children; GroupLineItems don't have that restriction. Presumably, pricing is basically the same - it's the sum of the individual parts. Especially if you get rid of the GroupSubItem, you might be able to combine both of those.
In the end, I think you're going to have to get to some kind of Composite hierarchy in order to protect yourself. You designed for one level of nesting, and now they want two. How long before they want three, four, etc.? Also, I think you may want to step back from the individual line-item use cases to do some consolidation. I see line items that look to have been created to fulfill mostly UI-related functions. I realize that putting that kind of logic in the UI isn't as helpful, but the alternative is what you have here.
The one crucial thing (crucial to my way of thinking, at least) is that order is important to you. Since you have line items that function on the previous one, or previous ones, in the list, the order of items must be preserved. That would include in the database. So my thoughts were centered around this. It's an odd implementation of a Composite pattern, but it might make your life easier - and you've already started at least some of it.
Your LineItems collection flattens your hierarchy. What if that was your one (and only) collection? Since order is important, you'd need to introduce two concepts: depth and sequence. Sequence is contained by depth, and depth is relatively self-explanatory. Then you only have one collection - the collection of LineItems off your quote. You'd want to build routines that allow a lineitem to find their next sibling, previous sibling, parent, etc. But then your grouping/bundling could become easier, and you automatically support any level of nesting you need without generating some sort of complex object graph. Grouping items merely requires a bit of re-ordering and some depth/sequence updates. It removes the "what do I do with child collections in leaf objects" question. You can still store parent-object references - they just point to an object higher up in the list. That way, parent-level properties can easily be retrieved (for something like the bundle children quantity multiplication).
I realize this probably ends up looking a lot like your database layout, and I'm not advocating a data-driven design. You can still have your different line items. But you'd also want to push as much functionality into the base LineItem class as you can. That way, your flattened collection can treat line items the same.
I also realize that, if you push your functionality into the base LineItem, you could still do a standard object graph. Using recursive routines, that's still not too hard to process. But, from your description, it feels like the flattened hierarchy is the more dominant form. Add in the need to "promote" child objects to the parent level for purposes of combining/rolling up items, and I think it's an idea at least worth investigating.
I also realize this is probably a fairly radical departure from your current design. If it were me, I'd blame it on the new use case. ![Wink [;)]](/emoticons/emotion-5.gif)
tmg4340:
I wouldn't say it's a messed-up design. I think it may be a little overly complex in some areas.
These part-whole hierarchies are always a bit of a tough nut. I've never been a fan of an explosion of leaf classes, which is kinda what you have here. I will readily concede that a use-case-driven design could lead to this, and there's no real good alternative. You're either checking against a ton of different types, or you're checking against some kind of type property. Which is better? OO design will say different types - but OO design will also say that you should include methods in the base class so that you can treat the subtypes the same.
Well, all those classes exist because they do have different responsiblities. Discounts aren't like note line items, etc.
If you look at the GoF Composite pattern - which is what most people would probably say you should be using - the issues with the pattern center around what you're dealing with. If you include a child collection at the base level, then you have to define behaviors for types that, by definition, will never have any children. You also potentially incur an object graph overhead, since said child collection will exist at every level, whether you need it or not.
I orginally did think about that pattern, but didn't go there for the reasons you specify.
I don't see a need for GroupSubItem, since all it seems to do is expose a property to the actual object you care about. Since the items are, by this definition, still editable within this context, GroupSubItem seems to be getting in the way.
Hmm, an interesting concept. I'll have to think about this. The GroupSubItem does a bit more than expose the Item property. For example, a SimpleLineItem overrides IsGroupable, IsUngroupable and IsGrouped to return true, false and false. While GroupSubItem delegates some thing, like Description, to the decorated instance, it doesn't delegate those three properties and always returns a fixed value for each of the Isxxx properties above. But this still may be a good option.
One possible simplification I see is to take your collection-based line items and combine them into one class. Realizing that BundleLineItem and GroupLineItem aren't the same thing, when you boil it down, they aren't all that different. BundleLineItems contain non-editable (and, in some cases, non-viewable) children; GroupLineItems don't have that restriction. Presumably, pricing is basically the same - it's the sum of the individual parts. Especially if you get rid of the GroupSubItem, you might be able to combine both of those.
Yes, they are similar, but the container itself acts a bit differently. For example, only the quantity of the bundle item is changable, and the unit price by certain users. For the group, everything is changable; the part number, description, etc. Taxable is forced blank (also unlike the bundle). The group item will also remove itself once it has no children. But perhaps this is something I can again work towards.
Also, bundle pricing may be the sum of the parts, or it may be fixed. That option is set by the product administrator when working with the bundle, and of course they can change to either method at any time.
In the end, I think you're going to have to get to some kind of Composite hierarchy in order to protect yourself. You designed for one level of nesting, and now they want two. How long before they want three, four, etc.? Also, I think you may want to step back from the individual line-item use cases to do some consolidation. I see line items that look to have been created to fulfill mostly UI-related functions. I realize that putting that kind of logic in the UI isn't as helpful, but the alternative is what you have here.
Well, I'm pretty certain that they will never ask for more than two levels of grouping. The purpose of the group item is to provide a way to "hide" what the customer is actually getting. Either to get around some IT policy at the customer site or because the customer just wants one line item on the invoice, for whatever reason. The bundle's purpose isn't necessarly to hide the items to make up the bundle, its a standard "we'll sell a complete system instead of you buying all the individual parts." Kind of like a home theater system. You could be each piece individually, but they'll sell you a working system. The result ends up being the same; the customer only sees the zero level items. Internally, we only show our parts department the "deepest" items.
The one crucial thing (crucial to my way of thinking, at least) is that order is important to you. Since you have line items that function on the previous one, or previous ones, in the list, the order of items must be preserved. That would include in the database. So my thoughts were centered around this. It's an odd implementation of a Composite pattern, but it might make your life easier - and you've already started at least some of it.
Yes, the ordering piece is in place currently. The items in a List stay where you place them, and you can ask a LineItem to move up or down. When saving, a position is deteremined.
Your LineItems collection flattens your hierarchy. What if that was your one (and only) collection? Since order is important, you'd need to introduce two concepts: depth and sequence. Sequence is contained by depth, and depth is relatively self-explanatory. Then you only have one collection - the collection of LineItems off your quote. You'd want to build routines that allow a lineitem to find their next sibling, previous sibling, parent, etc. But then your grouping/bundling could become easier, and you automatically support any level of nesting you need without generating some sort of complex object graph. Grouping items merely requires a bit of re-ordering and some depth/sequence updates. It removes the "what do I do with child collections in leaf objects" question. You can still store parent-object references - they just point to an object higher up in the list. That way, parent-level properties can easily be retrieved (for something like the bundle children quantity multiplication).
Yes, I have thought of this, and Rocky has also suggested it. My problem has been to reliably keep the items in order, or how to determine if an item can be moved. For example, bundle sub items cannot be re-ordered, they must appear in the order the product administrator set. If the bundle item needs to move up, so do all its children. Maybe this is simplier than I'm imagining though.
I realize this probably ends up looking a lot like your database layout, and I'm not advocating a data-driven design. You can still have your different line items. But you'd also want to push as much functionality into the base LineItem class as you can. That way, your flattened collection can treat line items the same.
No, not at all. Its confusing, because at the same time items ARE flat, but also exhibit behaviors which also suggest a hierarchy.
Yes, I agree that flat is more dominant and that I may have a lot of work to do... that's why I posted here. Since it's just me doing everything for this application (database to deployment and support), I need some place to bounce thoughts around.. and this forum has been a great place. Part of my dilemma is that I want to get this done rather soon, but I also don't want a bad design. I'm while I'm almost certain they'll never ask to group anything else (rather, ask to have one group of items under another group), I'm not 100% sure.I also realize that, if you push your functionality into the base LineItem, you could still do a standard object graph. Using recursive routines, that's still not too hard to process. But, from your description, it feels like the flattened hierarchy is the more dominant form. Add in the need to "promote" child objects to the parent level for purposes of combining/rolling up items, and I think it's an idea at least worth investigating.
I also realize this is probably a fairly radical departure from your current design. If it were me, I'd blame it on the new use case.
Yes, I knew that might be a possiblity when I ask for help. I certainly have a lot to think about.
In the interest of "getting it done" and the likelihood that I won't be asked to allow for groups of groups (just groups of products or product bundles), I think I may try for eliminating GroupSubItem and making items which can be grouped a bit smarter about when they are grouped (i.e., if their container is actually a line item or some other container).
Of course, if there are any other suggestions, I'd still love to hear them!
Thanksajj3085:
These part-whole hierarchies are always a bit of a tough nut. I've never been a fan of an explosion of leaf classes, which is kinda what you have here. I will readily concede that a use-case-driven design could lead to this, and there's no real good alternative. You're either checking against a ton of different types, or you're checking against some kind of type property. Which is better? OO design will say different types - but OO design will also say that you should include methods in the base class so that you can treat the subtypes the same.Well, all those classes exist because they do have different responsiblities. Discounts aren't like note line items, etc.
Sure - I'm not saying all the LineItem classes are the same. But they all do the same basic things, so perhaps your base class could turn into something more like a Template class. I don't know the full extent of your app, but if you could do something like that, you may be able to not care about the specific subtypes more. That makes the code a bit cleaner.
Yes, they are similar, but the container itself acts a bit differently. For example, only the quantity of the bundle item is changable, and the unit price by certain users. For the group, everything is changable; the part number, description, etc. Taxable is forced blank (also unlike the bundle). The group item will also remove itself once it has no children. But perhaps this is something I can again work towards. Also, bundle pricing may be the sum of the parts, or it may be fixed. That option is set by the product administrator when working with the bundle, and of course they can change to either method at any time. ajj3085: tmg4340:One possible simplification I see is to take your collection-based line items and combine them into one class. Realizing that BundleLineItem and GroupLineItem aren't the same thing, when you boil it down, they aren't all that different. BundleLineItems contain non-editable (and, in some cases, non-viewable) children; GroupLineItems don't have that restriction. Presumably, pricing is basically the same - it's the sum of the individual parts. Especially if you get rid of the GroupSubItem, you might be able to combine both of those.
I know what this is going to sound like, but if you took advantage of the fact that the line items know who their parent is, you could rely on the parent type to manage some of that. I know that sounds like you're pushing some of the responsibility of the LineItem to another class, but since the operations are dependent on the parent anyway, there's some coupling built in.
At least that's what I tell myself to feel better about it.
Yes, I have thought of this, and Rocky has also suggested it. My problem has been to reliably keep the items in order, or how to determine if an item can be moved. For example, bundle sub items cannot be re-ordered, they must appear in the order the product administrator set. If the bundle item needs to move up, so do all its children. Maybe this is simplier than I'm imagining though. ajj3085: tmg4340:Your LineItems collection flattens your hierarchy. What if that was your one (and only) collection? Since order is important, you'd need to introduce two concepts: depth and sequence. Sequence is contained by depth, and depth is relatively self-explanatory. Then you only have one collection - the collection of LineItems off your quote. You'd want to build routines that allow a lineitem to find their next sibling, previous sibling, parent, etc. But then your grouping/bundling could become easier, and you automatically support any level of nesting you need without generating some sort of complex object graph. Grouping items merely requires a bit of re-ordering and some depth/sequence updates. It removes the "what do I do with child collections in leaf objects" question. You can still store parent-object references - they just point to an object higher up in the list. That way, parent-level properties can easily be retrieved (for something like the bundle children quantity multiplication).
If you introduce depth and sequence, it's not so bad. Since the child items point to their parent, and sequence is encapsulated by parent item, moving a parent automatically moves the children - they still point to their parent, and their ordering doesn't change. Moving is pretty simple too - just change the sequence numbers of the moving items.
Maybe an example. Say you have something like this:
Parent ID Child ID Depth Sequence
--------- -------- ----- --------
1 <NULL> 1 1
1 2 2 1
1 3 2 2
1 4 2 3
5 <NULL> 1 2
5 6 2 1
5 7 2 2
If you need to move 5 above 1, then you just change their Sequence values. Nothing else changes, and the children stay where they should be.
As for determining what can or can't be moved, I'd fall back on "operations based on parent type" again, since the type of parent determines what can be done to its child(ren).
Yes, I agree that flat is more dominant and that I may have a lot of work to do... that's why I posted here. Since it's just me doing everything for this application (database to deployment and support), I need some place to bounce thoughts around.. and this forum has been a great place. Part of my dilemma is that I want to get this done rather soon, but I also don't want a bad design. I'm while I'm almost certain they'll never ask to group anything else (rather, ask to have one group of items under another group), I'm not 100% sure. ajj3085: tmg4340:I also realize that, if you push your functionality into the base LineItem, you could still do a standard object graph. Using recursive routines, that's still not too hard to process. But, from your description, it feels like the flattened hierarchy is the more dominant form. Add in the need to "promote" child objects to the parent level for purposes of combining/rolling up items, and I think it's an idea at least worth investigating.
Wherever you can get the help... hopefully I am helping!
I work under a similar type of situation, and here's how I look at it. I would love to have the time to develop The Perfect Design, and I often get my brain in a twist trying to get there. But in the end, if the design handles the needs of the app, then it's a good design. Everyone can see the holes they coded, given enough time. It's hard to plan for all the possible future cases, and in most cases you don't need to. You already know this. If what you have can be modified to accept the new conditions without a ton of work, and you're comfortable with the changes, do it and move on. You seem like a smart-enough developer that you're not going to code yourself into a corner that requires you to blow up the whole thing. Just because I think the design might be overly complicated certainly doesn't mean it is - I know nothing about the app or your customers. The only thing I'll say is that, if it were me, I wouldn't code myself into the "they aren't going to ask for more nesting levels" situation. But I only say that because I've been burned on that more than once. I know that makes the problem harder to fix than what's in front of you, and if they really never do ask for it, then you wasted your time.
Sure - I'm not saying all the LineItem classes are the same. But they all do the same basic things, so perhaps your base class could turn into something more like a Template class. I don't know the full extent of your app, but if you could do something like that, you may be able to not care about the specific subtypes more. That makes the code a bit cleaner.
I know what this is going to sound like, but if you took advantage of the fact that the line items know who their parent is, you could rely on the parent type to manage some of that. I know that sounds like you're pushing some of the responsibility of the LineItem to another class, but since the operations are dependent on the parent anyway, there's some coupling built in.
I think that's kind of the approach I'm taking. I don't want BundleLineItem to depend on GroupLineItem in any way, but I'm putting code in BundleLineItem to be able to recognize if it is a zero depth item or not. I'm also doing the same for SimpleLineItem (which, thinking about it, I should probably rename to ProductLineItem). I'm trying to use the interfaces I have to do this. I have a bunch.. ILineItemContainer (a collection class which holds line items), IContainerLineItem (a line item which containers other line items), IGroupableLineItem, IGroupedSubItem, ILineItemInternal. Then there's ILineItem, which is ALL the UI sees. Maybe it is time to see if I can't simplify a bit.
If you introduce depth and sequence, it's not so bad. Since the child items point to their parent, and sequence is encapsulated by parent item, moving a parent automatically moves the children - they still point to their parent, and their ordering doesn't change. Moving is pretty simple too - just change the sequence numbers of the moving items.Maybe an example. Say you have something like this:
Parent ID Child ID Depth Sequence
--------- -------- ----- --------
1 <NULL> 1 1
1 2 2 1
1 3 2 2
1 4 2 3
5 <NULL> 1 2
5 6 2 1
5 7 2 2If you need to move 5 above 1, then you just change their Sequence values. Nothing else changes, and the children stay where they should be.
That (except for depth) is what I first tried. It got complex, line items needed to know other line item's sequence numbers, etc. I realized that List kept the order, so I could remove that complexity and just move things around in the list. I could probably keep numbers in sync though.. my problem was exposing a BusinessListBase class that enforced this order. In my case, ordering is not just a UI feature, it's a business feature. But maybe I wasn't going about it the right way.
As for determining what can or can't be moved, I'd fall back on "operations based on parent type" again, since the type of parent determines what can be done to its child(ren).
Well, right now the only items that can't be moved are those contained in a BundleLineItem. Technically, items within a Group could be moved by the user... the current coding doesn't allow this, but it's not actually a business requirement, I think it was just a shortcut I took... or something I didn't think about. Indeed, this may be useful to users, because if they want a certain order within the group they need to ungroup the whole thing, re-order, regroup.
Wherever you can get the help... hopefully I am helping!
You have been very helpful. Going back and forth like this has given me some ideas.
I work under a similar type of situation, and here's how I look at it. I would love to have the time to develop The Perfect Design, and I often get my brain in a twist trying to get there. But in the end, if the design handles the needs of the app, then it's a good design. Everyone can see the holes they coded, given enough time. It's hard to plan for all the possible future cases, and in most cases you don't need to. You already know this. If what you have can be modified to accept the new conditions without a ton of work, and you're comfortable with the changes, do it and move on.
Well, that's where I'm at right now. This one feature is harder to implement in the current design than I would have liked. I guess that's just how things go sometimes. I don't mind doing alot of work again now, I just would like to have a design so that later changes don't require so much. But I guess that's not always possible, since we can't see the future.
You seem like a smart-enough developer that you're not going to code yourself into a corner that requires you to blow up the whole thing. Just because I think the design might be overly complicated certainly doesn't mean it is - I know nothing about the app or your customers. The only thing I'll say is that, if it were me, I wouldn't code myself into the "they aren't going to ask for more nesting levels" situation. But I only say that because I've been burned on that more than once. I know that makes the problem harder to fix than what's in front of you, and if they really never do ask for it, then you wasted your time.
Well, I might be at a point now where my current design needs to be blown up. I don't want to code myself into a corner either, but at the same time I try to stick to the YAGNI principle, so that I can get things done in a reasonable amount of time. Although my employer has been great about saying "take as much time as you need to do it," I still like to get things out the door sooner rather than later. Especially since much of what I'm replacing is a huge mess, and they've been very happy with what I've been able to deliver thus far.
I think I'll stay the current route, and follow your suggestion to kill GroupSubItem. It seems to be going much better so far, although there are some kinks to work out still... mostly that I haven't finished coding this concept yet. If the time comes that I need more nesting, my current design shouldn't prevent me from moving to a full Composite pattern.. it won't be any harder than it is now, and it really wouldn't be that hard... just more difficult than killing GroupSubItem.
Thanks very much for your help, it has been useful!
Andy
Well - the first thing that comes to mind is a static list of allowable child types for your GroupLineItem class. This isn't something I see you being able to put in the LineItemNode base class, since the check is really type-specific, and you can't have virtual or abstract static members. Plus, the check isn't really useful for the other list types. BundleLineItem objects only accept BundleComponentLineItem children, but that's completely enforced in the UI. And LineItems objects accept all types.
Anyway, you add a static constructor for your GroupLineItem class that populates this list. Finally, you put a "CanContainType" method that returns True or False based on a simple Contains() check. Then, in your context-menu code, you call "GroupLineItem.CanContainType(Type)" for each selected item, and enable/disable your menu item appropriately. And maybe you also have an overload that takes a list of items and does the looping for you - depends on how often you have to check multiple types.
It's not polymorphic, but since the grouping functionality is specific to the GroupLineItem type, that's not so bad. At least it isn't to me. It also "feels" better - knowing whether a specific line item can be grouped feels like GroupLineItem functionality to me.
- Scott
Copyright (c) Marimer LLC