0

My menu hierarchy looks like this:

Root
    ProductGroups
        Products
            ItemTypes
                Items

I avoided circular references so there are only references from a parent to its childs, e.g. a ProductGroup references its Products.

If I want to know to which Product an Item belongs to I have to iterate through the hierarchy starting from the Root. This results in 5 for-loops:

for (MenuElement rootChild : Root.getChildren())
 for(ProductGroup group : rootChild.getChildren())
  for(Product product: group.getChildren())
   for(ItemType itemType : product.getChildren())
    for(Item item : itemType.getChildren())
     if(item == searchedItem)
      return product;

Is this the best method or do you think that a recursive approach is better?

There shouldn't be a problem with performance: 10 ProductGroups, 10 Products, 6 ItemTypes, ~30 Items, so recursive approach wouldn't lead to a big overload.

One argument for recursion would be the shorter code. Are there more in this specific case?

10ff
  • 813
  • 4
  • 16
  • 31

2 Answers2

1

If you call this only somewhat frequently, I would recommend to either include a parent-pointer on each level, or in case you don't want to modify the code, create hashmaps for reverse-lookup instead of using either of your solutions.

Item->ItemType
ItemType->Product
Product->ProductGroup

You current solution always traverses ALL existing items, which is a bit overkill. If you do a reverse-lookup you will be done after 2 gets from a hashmap, and you code will be much simpler like this:

type = itemToTypeMap.get(item);
if (null == type); //treat error
product = typeToProductMap.get(type);
if (null == product); //treat error
return product;
midor
  • 5,487
  • 2
  • 23
  • 52
  • Yes a parent-pointer would simplify it, but that would be a cyclic reference. I think this is worse? But the HashMap is one good solution, thanks. – 10ff Mar 15 '15 at 13:08
  • I think it would be very helpful if you would explain what the problem with a "cyclic" reference is in your opinion. I would not describe a parent pointer as cyclic, but as bidirectional. – midor Mar 15 '15 at 19:34
1

I don't know how the recursive approach will work as it stands as you have different types at each level and we don't know enough about those types or any super types etc.

One piece of advice I can give is regarding coupling.

Your code violates the Law of Demeter because there is high coupling in this code to each of these internal structures. This makes it hard to refactor or extend later.

This code is more loosely coupled:

for (MenuElement rootChild : Root.getChildren()) {
   Product product = rootChild.findProductByItem(searchedItem);
   if (product != null) return product;
}

Then in MenuElement:

public Product findProductByItem(Item item) {
   for (ProductGroup group : getChildren()) {
     Product product = group.findProductByItem(item);
     if (product!=null) return product;
   }
   return null;
}

Then in ProductGroup:

public Product findProductByItem(Item item) {
   for (Product product: getChildren())
     if (product.matchesItem(item))
        return product;
   return null;
}

Then in Product:

public boolean matchesItem(Item item) {
   for (ItemType itemType : getChildren())
     if (itemType.matches(item))
       return true;
   return false;
}

Then in ItemType:

public boolean matchesItem(Item searchItem) {
     for (Item item : getChildren())
       if (item == searchItem) //do you really mean == BTW?
         return true;
   return false;
}

Now you can see repeating patterns that might be suitable for pulling up to super classes which could help you achieve the recursive code.

weston
  • 54,145
  • 21
  • 145
  • 203
  • Violation of the Law of Demeter? But the parents only know about their childs, not the other way around. Or do you mean because I use top-down-references and not bottom-up? – 10ff Mar 15 '15 at 13:14
  • I'm talking about the code you posted. It is coupled to all levels and all classes. – weston Mar 15 '15 at 14:29