views:

796

answers:

8

It feels like there must be some semi-simple solution to this, but I just can't figure it out.

Edit: The previous example showed the infinite loop more clearly, but this gives a bit more context. Check out the pre-edit for a quick overview of the problem.

The following 2 classes represent the View-Models of the Model View View-Model (MVVM) pattern.

/// <summary>
/// A UI-friendly wrapper for a Recipe
/// </summary>
public class RecipeViewModel : ViewModelBase
{
    /// <summary>
    /// Gets the wrapped Recipe
    /// </summary>
    public Recipe RecipeModel { get; private set; }

    private ObservableCollection<CategoryViewModel> categories = new ObservableCollection<CategoryViewModel>();

    /// <summary>
    /// Creates a new UI-friendly wrapper for a Recipe
    /// </summary>
    /// <param name="recipe">The Recipe to be wrapped</param>
    public RecipeViewModel(Recipe recipe)
    {
        this.RecipeModel = recipe;
        ((INotifyCollectionChanged)RecipeModel.Categories).CollectionChanged += BaseRecipeCategoriesCollectionChanged;

        foreach (var cat in RecipeModel.Categories)
        {
            var catVM = new CategoryViewModel(cat); //Causes infinite loop
            categories.AddIfNewAndNotNull(catVM);
        }
    }

    void BaseRecipeCategoriesCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
    {
        switch (e.Action)
        {
            case NotifyCollectionChangedAction.Add:
                categories.Add(new CategoryViewModel(e.NewItems[0] as Category));
                break;
            case NotifyCollectionChangedAction.Remove:
                categories.Remove(new CategoryViewModel(e.OldItems[0] as Category));
                break;
            default:
                throw new NotImplementedException();
        }
    }

    //Some Properties and other non-related things

    public ReadOnlyObservableCollection<CategoryViewModel> Categories 
    {
        get { return new ReadOnlyObservableCollection<CategoryViewModel>(categories); }
    }

    public void AddCategory(CategoryViewModel category)
    {
        RecipeModel.AddCategory(category.CategoryModel);
    }

    public void RemoveCategory(CategoryViewModel category)
    {
        RecipeModel.RemoveCategory(category.CategoryModel);
    }

    public override bool Equals(object obj)
    {
        var comparedRecipe = obj as RecipeViewModel;
        if (comparedRecipe == null)
        { return false; }
        return RecipeModel == comparedRecipe.RecipeModel;
    }

    public override int GetHashCode()
    {
        return RecipeModel.GetHashCode();
    }
}

.

/// <summary>
/// A UI-friendly wrapper for a Category
/// </summary>
public class CategoryViewModel : ViewModelBase
{
    /// <summary>
    /// Gets the wrapped Category
    /// </summary>
    public Category CategoryModel { get; private set; }

    private CategoryViewModel parent;
    private ObservableCollection<RecipeViewModel> recipes = new ObservableCollection<RecipeViewModel>();

    /// <summary>
    /// Creates a new UI-friendly wrapper for a Category
    /// </summary>
    /// <param name="category"></param>
    public CategoryViewModel(Category category)
    {
        this.CategoryModel = category;
        (category.DirectRecipes as INotifyCollectionChanged).CollectionChanged += baseCategoryDirectRecipesCollectionChanged;

        foreach (var item in category.DirectRecipes)
        {
            var recipeVM = new RecipeViewModel(item); //Causes infinite loop
            recipes.AddIfNewAndNotNull(recipeVM);
        }
    }

    /// <summary>
    /// Adds a recipe to this category
    /// </summary>
    /// <param name="recipe"></param>
    public void AddRecipe(RecipeViewModel recipe)
    {
        CategoryModel.AddRecipe(recipe.RecipeModel);
    }

    /// <summary>
    /// Removes a recipe from this category
    /// </summary>
    /// <param name="recipe"></param>
    public void RemoveRecipe(RecipeViewModel recipe)
    {
        CategoryModel.RemoveRecipe(recipe.RecipeModel);
    }

    /// <summary>
    /// A read-only collection of this category's recipes
    /// </summary>
    public ReadOnlyObservableCollection<RecipeViewModel> Recipes
    {
        get { return new ReadOnlyObservableCollection<RecipeViewModel>(recipes); }
    }


    private void baseCategoryDirectRecipesCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
    {
        switch (e.Action)
        {
            case NotifyCollectionChangedAction.Add:
                var recipeVM = new RecipeViewModel((Recipe)e.NewItems[0], this);
                recipes.AddIfNewAndNotNull(recipeVM);
                break;
            case NotifyCollectionChangedAction.Remove:
                recipes.Remove(new RecipeViewModel((Recipe)e.OldItems[0]));
                break;
            default:
                throw new NotImplementedException();
        }
    }

    /// <summary>
    /// Compares whether this object wraps the same Category as the parameter
    /// </summary>
    /// <param name="obj">The object to compare equality with</param>
    /// <returns>True if they wrap the same Category</returns>
    public override bool Equals(object obj)
    {
        var comparedCat = obj as CategoryViewModel;
        if(comparedCat == null)
        {return false;}
        return CategoryModel == comparedCat.CategoryModel;
    }

    /// <summary>
    /// Gets the hashcode of the wrapped Categry
    /// </summary>
    /// <returns>The hashcode</returns>
    public override int GetHashCode()
    {
        return CategoryModel.GetHashCode();
    }
}

I won't bother showing the Models (Recipe and Category) unless requested, but they basically take care of the business logic (for instance adding a recipe to a category will also add the other end of the link, i.e. if a category contains a recipe, then the recipe is also contained in that category) and basically dictate how things go. The ViewModels provide a nice interface for WPF databinding. That's the reason for the wrapper classes

Since the infinite loop is in the constructor and it's trying to create new objects, I can't just set a boolean flag to prevent this because neither object ever gets finished being constructed.

What I'm thinking is having (either as a singleton or passed in to the constructor or both) a Dictionary<Recipe, RecipeViewModel> and Dictionary<Category, CategoryViewModel> that will lazy-load the view models, but not create a new one if one already exists, but I haven't gotten around to trying to see if it'll work since it's getting late and I'm kinda tired of dealing with this for the past 6 hours or so.

No guarantee the code here will compile since I took a bunch of stuff out that was unrelated to the problem at hand.

+1  A: 

options:

  1. implement a membership test, e.g. check bar-is-member-of-foo before adding
  2. move the many-to-many relationship to its own class

the latter is preferred, i think - it's more relationally sound

of course, with a foo-bar example we really don't know what the goal is, so your mileage may vary

EDIT: given the code in the original question, #1 will not work because the infinite recursion happens before anything is ever added to any list.

There are several problems with this approach/question, probably because it has been abstracted to the point of near silliness - good for illustrating the coding problem, no so good for explaining the original intent/goal:

  1. the wrapper classes don't actually wrap anything or add any useful behavior; this makes it difficult to see why they're needed
  2. with the given structure, you cannot initialize the lists in the constructor at all because each wrapper list immediately creates a new instance of the other wrapper list
  3. even if you separate initialization from construction, you still have a cyclic dependency with hidden membership (i.e. the wrappers reference each other but hide the foo/bar elements from a contains check; which doesn't really matter because the code never gets to add anything to any list anyway!)
  4. a direct relational approach would work, but requires searching mechanisms and assumes that wrappers would be created as-needed instead of in advance, e.g. an array with search functions or a pair of dictionaries (e.g. Dictionary>, Dictionary>) would work for the mapping but might not fit your object model

Conclusion

I don't think the structure as described will work. Not with DI, not with a factory, not at all - because the wrappers reference each other while hiding the sublists.

This structure hints at unstated incorrect assumptions, but with no context we can't ferret out what they might be.

Please restate the problem in the original context with real-world objects and desired goal/intent.

Or at least state what structure you think your sample code should produce. ;-)

Addendum

Thanks for the clarification, this makes the situation more comprehensible.

I have not worked with WPF databinding - but I have skimmed this MSDN article - so the following may or may not be helpful and/or correct:

  • I think the categories and recipes collections in the view-model classes are redundant
    • you already have the M:M information in the underlying category object, so why duplicate it in the view-model
    • it looks like your collection-changed handlers will also cause infinite recursion
    • the collection-changed handlers do not appear to update the underlying M:M information for the wrapped recipe/category
  • I think the purpose of the view-model is to expose the underlying model data, not to indivudally wrap each of its components.
    • This seems redundant and a violation of encapsulation
    • It is also the source of your infinite-recursion problem
    • Naively, I would expect the ObservableCollection properties to merely return the underlying model's collections...

The structure you have is an "inverted index" representation of a many-to-many relationship, which is quite common for optimized lookups and dependency management. It reduces to a pair of one-to-many relationships. Look at the GamesViewModel example in the MSDN article - note that the Games property is just

ObservableCollection<Game>

and not

ObservableCollection<GameWrapper>
Steven A. Lowe
I tried every variation I could come up for #1 but no dice. The problem is the infinite loop happens before anything is actually added. I could replace "MyBarWrappers.Add(new BarWrapper(bar));" with just "new BarWrapper(bar);" and the same for the other one and the stackoverflow would still occur.
Davy8
#2 seems like it'd probably work, but doesn't feel very Object-oriented and feels more like relational DB style. Conceptually I can't see why this is so difficult, all I want is to mirror the relationships in the wrapper objects that I have in the business objects.
Davy8
Actually, unless I'm doing it wrong, which is highly possible, #2 doesn't seem to work either. I created basically a tuple class FooBarWrapperPair and instead of having FooWrapper contain a list of BarWrappers, I have FooWrapper containing a list of FooBarWrapperPairs. But the problem is I still need to construct a BarWrapper to put into the pair. I might be misunderstanding your meaning though.
Davy8
@[Davy8]: see edits; note that with no context this appears to be a very strange thing to do - the wrapper classes serve no apparent purpose
Steven A. Lowe
@[Davy8]: edits completed, sorry for the delay. I think you've coded yourself into a corner, and need to back up and rethink the domain and goals.
Steven A. Lowe
Edited with real example (minus unrelated details) and more context
Davy8
@[Davy8]: see addendum
Steven A. Lowe
In this particular case I could just have an ObservableCollection of the model directly, but that depends on the fact that my model implements IPropertyChanged and ICollectionChanged, and thus is bindable. If the model were something that didn't implement those and I didn't have the source for it, that wouldn't work because while the ObserableCollection will give notice of the collection changing, each of the items within the collection need to give WPF friendly notification that they themselves changed.
Davy8
I tried to download the code sample for link you gave to take a look at what the Game class looked like, since it wasn't shown in the article, but unfortunately none of the projects would load in Visual C# Express. I did look at some of the CS files directly and it looks like Game (well the partial class for Game, looks like the other half is an Entities Framework generated class) does implement INotifyPropertyChanged. What it looks like now though is that the View depends on the Model, when if I understand MVVM properly, the View should depend only on ViewModel which depends on Model
Davy8
Also, my RecipeViewModel has slightly different properties for binding, where in the Model I have a TimeSpan property, in the ViewModel it's exposed as a separate Minutes and Hours field for binding to text boxes.
Davy8
@[Davy8]: then i think you'll need a dynamic factory and a lookup; sorry i can't be more helpful, I haven't bothered with WTF et al yet. You might try upvoting some other people's answers and see if they will kick in some extra effort. ;-)
Steven A. Lowe
+1  A: 

I would recommend you get rid of the mutual dependency, for example through the Dependency Inversion principle, http://en.wikipedia.org/wiki/Dependency_inversion_principle -- have at least one of the two sides Foo and Bar (or their wrappers) depend on an abstract interface which the other side implements, rather than having two concrete classes directly depend on each other, which can easily produce circular-dependency and mutual-recursion nightmares like the one you're observing. Also, there are alternative ways to implement many-to-many relationships that may be worth considering (and may be easier to subject to inversion of dependency through the introduction of suitable interfaces).

Alex Martelli
In particular, Google Guice makes short work of things like this. See http://code.google.com/p/google-guice/
Dave W. Smith
I'm reading the PDF by Robert Martin linked from wiki right now, about halfway through and while I agree that it's a good practice, I don't see how how having a an IFoo and IBar interfaces would help in this particular scenario. I might be missing something though.
Davy8
Unfortunately, as cool as DI is, it will not help with the given structure.
Steven A. Lowe
A: 

I am going to say Factory Pattern. That way, you can construct each in turn, then add them to each other, then return them all hidden from prying eyes by the factory.

JP Alioto
A: 

So, Foo and Bar are the Models. Foo is a list of Bars and Bar is a list of Foos. If I'm reading that correctly you have two objects which are nothing but containers of each other. A is the set of all Bs and B is the set of all As? Isn't that circular by its very nature? It's infinite recursion by its very definition. Does the real world case include more behavior? Perhaps that's why people are having a difficult time explaining a solution.

My only thought is that if this is truly on purpose then to use static classes or to use a static variable to record that the classes had been created once and only once.

KnownIssues
It's not that A is the set of all B's and B is the set of all A's. A more concrete example instead of Foo and Bar is Books and Libraries. A book can be in many libraries and a library can contain many books. A book (or more realistically a book's entry in a computer catalog of some sort) has a list of all the libraries that carry it, and a library has a list of all the books it contains.
Davy8
+1  A: 

This reminds me of the way serialization prevents infinite loops when objects contain other objects. It maps the hash code of each object to its byte array so when an object contains a reference to another object it: a) doesn't serialize the same object twice, and b) doesn't serialize itself into an infinite loop.

You have essentially the same issue. The solution could be just as simple as using some kind of map instead of list collection. If what you're getting at is a many-to-many then you just create a map of lists.

dviljoen
+1  A: 

Man, my answer is not as cool as those DI ones. But...

In simplest terms, I think you must create your wrappers before you start relating them. Traverse your entire list of Foos, creating FooWrappers. Then traverse Bars and create BarWrappers. Then read the source Foos, adding appropriate BarWrapper references to the MyBarWrappers in the associated FooWrapper, and vice versa for Bars.

If you insist on both creating a wrapper for a Foo instance and immediately creating relationships to each of it's Bar instances, then you must "break" the cycle by marking which Foo you are working on, i.e. Foo_1, and let each of the BarWrapper instances know NOT to create yet another FooWrapper_1 instance inside it's MyFooWrappers collection. After all, you are, in fact, already creating the FooWrapper_1 higher up (or further down, as it were) the call stack.

Bottom line: as a matter of code sanity, the wrapper constructors should not be creating more wrappers. At the very most - it should only know/find that a single unique wrapper exists somewhere else for each Foo and Bar, and MAYBE create the wrapper ONLY if it doesn't find it elsewhere.

Jeff Meatball Yang
Yeah, I was thinking about having a dictionary somewhere with the Foo's as keys and FooWrappers as values. Too tired to try it tonight though.
Davy8
+2  A: 

Back to your original question (and the code). If what you want is to have a many-2-many relation that is automatically synchronized, then read on. The best place to look for a complex code that handles these cases is the source code of any ORM framework and it is very common problem for this domain of tools. I would look at the source code of nHibernate (https://nhibernate.svn.sourceforge.net/svnroot/nhibernate/trunk/nhibernate/) to see how it implements collections that handle both 1-N and M-N relations.

Simple something you can try is to create your own little collection class that just takes care of that. Below I removed your original wrapper classes and added a BiList collection, which is initialized with the object (the owner of the collection) and the name of the other side of the property to keep in sync with (works only for M-N, but 1-N would be simple to add). Of course, you would want to polish the code:

using System.Collections.Generic;

public interface IBiList
{
    // Need this interface only to have a 'generic' way to set the other side
    void Add(object value, bool addOtherSide);
}

public class BiList<T> : List<T>, IBiList
{
    private object owner;
    private string otherSideFieldName;

    public BiList(object owner, string otherSideFieldName) {
        this.owner = owner;
        this.otherSideFieldName = otherSideFieldName;
    }

    public new void Add(T value) {
        // add and set the other side as well
        this.Add(value, true);
    }

    void IBiList.Add(object value, bool addOtherSide) {
        this.Add((T)value, addOtherSide);
    }

    public void Add(T value, bool addOtherSide) {
        // note: may check if already in the list/collection
        if (this.Contains(value))
            return;
        // actuall add the object to the list/collection
        base.Add(value);
        // set the other side
        if (addOtherSide && value != null) {
            System.Reflection.FieldInfo x = value.GetType().GetField(this.otherSideFieldName);
            IBiList otherSide = (IBiList) x.GetValue(value);
            // do not set the other side
            otherSide.Add(this.owner, false);
        }
    }
}

class Foo
{
    public BiList<Bar> MyBars;
    public Foo() {
        MyBars = new BiList<Bar>(this, "MyFoos");
    }
}

class Bar
{
    public BiList<Foo> MyFoos;
    public Bar() {
        MyFoos = new BiList<Foo>(this, "MyBars");
    }
}



public class App
{
    public static void Main()
    {
        System.Console.WriteLine("setting...");

        Foo testFoo = new Foo();
        Bar testBar = new Bar();
        Bar testBar2 = new Bar();
        testFoo.MyBars.Add(testBar);
        testFoo.MyBars.Add(testBar2);
        //testBar.MyFoos.Add(testFoo); // do not set this side, we expect it to be set automatically, but doing so will do no harm
        System.Console.WriteLine("getting foos from Bar...");
        foreach (object x in testBar.MyFoos)
        {
            System.Console.WriteLine("  foo:" + x);
        }
        System.Console.WriteLine("getting baars from Foo...");
        foreach (object x in testFoo.MyBars)
        {
            System.Console.WriteLine("  bar:" + x);
        }
    }
}
van
+1  A: 

First of all DI is not going to solve your problem, but one thing always related to DI which IS going to solve your problem is usage of a Container (or a Context with lookup capability)

Solution:

You code fails in these places:

var catVM = new CategoryViewModel(cat); //Causes infinite loop
...
var recipeVM = new RecipeViewModel(item); //Causes infinite loop

The problem is caused by the fact that you create the wrapper (xxxViewModel) for an object, even if it already exists. Instead of creating a wrapper for the same object again, you need to check if a wrapper for this model already exists and use it instead. So you need a Container to keep track of all created objects. Your options are:

option-1: use a simple a-la Factory Pattern to create your objects, but also keep track of them:

class CategoryViewModelFactory
{
    // TODO: choose your own GOOD implementation - the way here is for code brevity only
    // Or add the logic to some other existing container
    private static IDictionary<Category, CategoryViewModel>  items = new Dictionary<Category, CategoryViewModel>();
    public static CategoryViewModel GetOrCreate(Category cat)
    {
        if (!items.ContainsKey(cat))
            items[cat] = new CategoryViewModel(cat);
        return items[cat];
    }
}

Then you do the same thing on the side of Recipe and problematic code is fixed:

  // OLD: Causes infinite loop
  //var catVM = new CategoryViewModel(cat);
  // NEW: Works 
  var catVM = CategoryViewModelFactory.GetOrCreate(cat);

Beware: possible memory leaks?

One thing you should be aware of (and that is also why you should not use the dummy a-la factory implementation) is the fact that these creator objects will keep references to both the model objects and their View wrappers. Therefore the GC will not be able to clean them from the memory.

option-1a: most probably you have a Controller (or Context) in your application already, to which views have access to. In this case instead of creating those a-la factories, I would just move GetOrCreate methods to this context. In this case when the context is gone (the form is closed), also those dictionaries will be de-referenced and the leak issue is gone.

van