20

We're creating an object hierarchy where each item has a collection of other items, and each item also has a Parent property pointing to its parent item. Pretty standard stuff. We also have an ItemsCollection class that inherits from Collection<Item> which itself has an Owner property pointing to the item the collection belongs to. Again, nothing interesting there.

When an item is added to the ItemsCollection class, we want it to automatically set the parent of Item (using the collection's Owner property) and when the item is removed, we want to clear the parent.

Here's the thing. We only want the Parent setter to be available to ItemsCollection, nothing else. That way not only can we know who the parent of an item is, but we can also ensure an item isn't added to multiple collections by checking for an existing value in Parent, or letting someone arbitrarily change it to something else.

The two ways we know how to do this are:

  1. Mark the setter as private, then enclose the collection definition within the scope of the item itself. Pro: Full protection. Con: Ugly code with nested classes.

  2. Use a private ISetParent interface on Item that only ItemsCollection knows about. Pro: Much cleaner code and easy to follow. Con: Technically anyone who knows of the interface can cast Item and get at the setter.

Now technically via reflection anyone can get at anything anyway, but still... trying to find the best way to do this.

Now I know there was a feature in C++ called Friend or something that let you designate an otherwise private member in one class as being available to another which would be the perfect scenario, but I don't know of any such thing in C#.

In pseudocode (e.g. all the property changed notifications and such have been removed for brevity and I'm just typing this here, not copying from code), we have this...

public class Item
{
    public string Name{ get; set; }
    public Item Parent{ get; private set; }
    public ItemsCollection ChildItems;

    public Item()
    {
        this.ChildItems = new ItemsCollection (this);
    }
}

public class ItemsCollection : ObservableCollection<Item>
{
    public ItemsCollection(Item owner)
    {
        this.Owner = owner;
    }   

    public Item Owner{ get; private set; }

    private CheckParent(Item item)
    {
        if(item.Parent != null) throw new Exception("Item already belongs to another ItemsCollection");
        item.Parent = this.Owner; // <-- This is where we need to access the private Parent setter
    }

    protected override void InsertItem(int index, Item item)
    {
        CheckParent(item);
        base.InsertItem(index, item);
    }

    protected override void RemoveItem(int index)
    {
        this[index].Parent = null;
        base.RemoveItem(index);
    }

    protected override void SetItem(int index, Item item)
    {
        var existingItem = this[index];

        if(item == existingItem) return;

        CheckParent(item);
        existingItem.Parent = null;

        base.SetItem(index, item);
    }

    protected override void ClearItems()
    {
        foreach(var item in this) item.Parent = null; <-- ...as is this
        base.ClearItems();
    }

}

Any other way to do something similar?

Mark A. Donohoe
  • 28,442
  • 25
  • 137
  • 286

9 Answers9

15

I have to solve your problem every day, but I don't do it the way you're trying to do it.

Take a step back. What is the fundamental problem you're trying to solve? Consistency. You are trying to ensure that the "x is a child of y" relationship and the "y is the parent of x" relationship are always consistent. That's a sensible goal.

Your supposition is that every item directly knows both its children and its parent because the children collection and the parent reference are stored locally in fields. This logically requires that when item x becomes a child of item y, you have to consistently change both x.Parent and y.Children. That presents the problem that you've run into: who gets to be "in charge" of making sure that both changes are made consistently? And how do you ensure that only the "in charge" code gets to mutate the parent field?

Tricky.

Suppose we denied your supposition. It need not be the case that every item knows both its children and its parent.

Technique #1:

For example, you could say that there is one special item called "the universe", which is the ancestor of every item except itself. "The universe" could be a singleton stored in a well-known location. When you ask an item for its parent, the implementation could find the universe, and then do a search of every descendant of the universe looking for the item, keeping track of the path as you go. When you find the item, great, you're done. You look one step back on the "path" that got you there, and you have the parent. Even better, you can provide the entire chain of parents if you want; after all, you just computed it.

Technique #2:

That could be expensive if the universe is large and it takes a while to find each item. Another solution would be to have the universe contain a hash table that maps items to their parents, and a second hash table that maps items to a list of their children. When you add child x to parent y, the "add" method actually calls the Universe and says "hey, item x is now parented by y", and the Universe takes care of updating the hash tables. Items do not contain any of their own "connectedness" information; that's the responsibility of the universe to enforce.

A down side of that is it is possible for the universe to then contain cycles; you could tell the universe that x is parented by y and y is parented by x. If you wish to avoid this then you'd have to write a cycle detector.

Technique #3:

You could say that there are two trees; the "real" tree and the "facade" tree. The real tree is immutable and persistent. In the real tree, every item knows its children but not its parent. Once you have built the immutable real tree, you make a facade node that is a proxy to the root of the real tree. When you ask that node for its children, it makes a new facade node wrapped around each child and sets the parent property of the facade node to the node that was queried for its children.

Now you can treat the facade tree as a parented tree, but the parent relationships are only computed as you traverse the tree.

When you want to edit the tree, you produce a new real tree, re-using as much of the old real tree as possible. You then make a new facade root.

The downside of this approach is that it only works if you typically traverse the tree from the top down after every edit.

We use this latter approach in the C# and VB compilers because that is precisely the situation we are in: when we rebuild a parse tree after a code edit we can re-use much of the existing immutable parse tree from the previous text. We always traverse the tree from the top down, and only want to compute the parent references when necessary.

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • 1
    @MarqueIV: A better analogy -- a far more realistic analogy -- is that "public" is everyone in the universe and "internal" is everyone who works for your team at work, and "internals visible to friend assemblies" is everyone who works for your company. If you can't trust people on your team to not misuse the internals of your assemblies, then *get better coworkers* or *create a culture of better code reviews*. – Eric Lippert Jul 07 '11 at 18:11
  • This is what I'm talking about... just look at all of that! Here's an analogy: A mother, a father and their child in a shopping mall. You have public: the entire world. Internal: the people in the mall. Private: individual people. Caley's with mom while Dad's in the Bose store. Mom is the guardian. Later, dad takes Caley and the mom hits Scents-R-Us! Caley's guardian was changed to Dad by mom since she was the guardian. And while anyone in the mall can talk to Mom, dad or Caley, only mom or dad can assign the role of guardian, and she knows who her current guardian is. Family==Friend. – Mark A. Donohoe Jul 07 '11 at 18:14
  • @MarqueIV: the reason to not have "friend" between two classes is that it very quickly devolves into "internal". I once worked on a team that made a reasonably small library written in C++ in which the word "friend" appeared *seven hundred times* in the source code. Now, just because a feature can be abused is no reason not to add it; all features can be abused. But I think it is reasonable to say that you can trust your coworkers to not misuse your internal implementation details. – Eric Lippert Jul 07 '11 at 18:15
  • sorry for the out-of-order thing. Thought I lost the edit and re-typed it all. And while I agree about the 'trust your team' theory, then one can make the same argument you don't need 'Private'. Just 'Internal' or 'Public'. After all, if internal is everyone on your team and you just 'trust' them, then why have private? 'Joe... I know you can see this, but don't touch it!' 'Sure thing, Mark!' ... 'Um... Joe?!' ... 'Er... sorry! It was shiny!... and I missed the memo!' Friend puts the onus on the compiler as dictated by the designer. No 'trust' needed. – Mark A. Donohoe Jul 07 '11 at 18:17
  • Plus... how can you explain what shouldn't be touched by your teammates vs. what should? If you see a read/writable Parent property, and you have an object you want to be parent... so you just set it! Works great! ...until the 'private' implementation of Child is expecting itself to be in the child collection of parent which doesn't actually exist, and BOOM! Crash. Trust should not be part of design if it can be enforced in the compiler. Not having it removes a check, abused otherwise or not. Don't fault a language for a person's poor grammar. – Mark A. Donohoe Jul 07 '11 at 18:24
  • @MarqueIV: I take your point, and I've heard these arguments many times before. The simple fact is that it is not the job of a statically-typed language to enforce *every* semantic constraint that you think is desirable. Why is "you've got to keep children and parents consistent" something that should be enforced by the compiler, but other factors like "this method isn't idempotent, so make sure you don't call it twice!" something that you don't expect to be enforced by the compiler? – Eric Lippert Jul 07 '11 at 18:58
  • 1
    This got bumped back to me today because someone voted it up. Just re-read this with two-years history on this and I have to say I still stand by the need for Friend in C#. Addressing your 'idempotent' comment, to me the difference is in that case, you have to see it because even though you're only supposed to call it once, you have to see it to call it. The case I'm talking about is different because there is *never* a reason for them to call SetParent so IMO the setter should be hidden to them. Anyway, that's my $1.37 (which is 2c with two years of inflation.) – Mark A. Donohoe Jul 09 '13 at 21:23
5

C# Doesn't have a friend keyword, but it has something close called internal. You could mark the methods you want to expose in a limited fashion as internal and then only other types in that assembly will be able to see them. If all your code is in one assembly, this won't help you much, but if this class is packaged in a separate assembly it would work.

public class Item
{
    public string Name{ get; set; }
    public Item Parent{ get; internal set; } // changed to internal...
    public ItemsCollection ChildItems;

    public Item()
    {
        this.ChildItems = new ItemsCollection (this);
    }
}
James Michael Hare
  • 37,767
  • 9
  • 73
  • 83
  • Personally I like JeffN825's #1 option myself. I only recommend internal if you have the code isolated fairly well in a class library where it won't be generally visible to the application at large. – James Michael Hare Jul 07 '11 at 16:14
5

Here's a way you can simulate friend in C#:

Mark your properties internal and then use this attribute to expose them to friend assemblies:

[assembly: InternalsVisibleTo("Friend1, PublicKey=002400000480000094" + 
                              "0000000602000000240000525341310004000" +
                              "001000100bf8c25fcd44838d87e245ab35bf7" +
                              "3ba2615707feea295709559b3de903fb95a93" +
                              "3d2729967c3184a97d7b84c7547cd87e435b5" +
                              "6bdf8621bcb62b59c00c88bd83aa62c4fcdd4" +
                              "712da72eec2533dc00f8529c3a0bbb4103282" +
                              "f0d894d5f34e9f0103c473dce9f4b457a5dee" +
                              "fd8f920d8681ed6dfcb0a81e96bd9b176525a" +
                              "26e0b3")]

public class MyClass {
   // code...
}
Mrchief
  • 75,126
  • 20
  • 142
  • 189
  • 1
    It doesn't sound like he wants them visible across assemblies, in fact quite the contrary he wants them very limited to just a privileged class. – James Michael Hare Jul 07 '11 at 16:27
  • James is right in his assessment, but I'm voting this up anyway because that's damn cool to know! Thx! :) – Mark A. Donohoe Jul 07 '11 at 16:41
  • It is :-) incidentally you only need the public key mess in there if the assembly is strongly named (signed). I use that trick a lot to let me unit test internal classes from one assembly in a separate unit test assembly. – James Michael Hare Jul 07 '11 at 16:53
  • The funny thing is it actually looks soo damn close to what Friend used to do. You could say [PrivatesVisibleTo:foo] or something similar. That would've been sweet. Alas, only if... – Mark A. Donohoe Jul 07 '11 at 16:56
  • 1
    However, don't say that in public. You'll get arrested! :) – Mark A. Donohoe Jul 07 '11 at 16:56
  • Yes, you'll get into a "holy war" on encapsulation... Sometimes, internal is just nice for exposing a method in a limited fashion in such a way that you don't have to obfuscate the class model as much. I really do try to limit it to very rare cases, though, and I would only expose methods/properties as internal and never fields. – James Michael Hare Jul 07 '11 at 17:27
  • Agree with you guys 100%. C# does have its own share of hacks, only this one is documented and useful (on rare occasions). – Mrchief Jul 07 '11 at 17:31
4

The only two things I can think of:

One:

Use sort of option number 2 you mention above (which I do constantly myself)...but make the implementation of the interface (Item) be a nested private class inside of ItemsCollection... that way only ItemsCollection knows about the setter. The interface IItem only declares a getter for Parent...and no one can cast it to Item because Item is private to ItemsCollection. So, something like:

public class ItemsCollection : ObservableCollection<IItem>
{
    private class Item : IItem 
    {
        public object Parent { get; set; }
    }

    private CheckParent(IItem item)
    {
        if(item.Parent != null) throw new Exception("Item already belongs to another ItemsCollection");
        ((Item)item).Parent = this.Owner; // <-- This is where we need to access the private Parent setter
    }

    public static IItem CreateItem() { return new Item(); }
}

public interface IItem 
{
    object Parent {get; }
}

and when you want ItemsCollection to set the item Parent, case the IItem instance to Item (which does expose a setter). Only ItemsCollection can do this cast, since the Item implementation is private to ItemsCollection...so I think that accomplishes what you want.

Two:

Make it internal not private...you don't get exactly what you want, but you can use InternalsVisibleToAttribute to denote that the internal members are visible to another assembly.

nawfal
  • 70,104
  • 56
  • 326
  • 368
Jeff
  • 35,755
  • 15
  • 108
  • 220
  • Yeah, I'm leaning towards the first myself. The 2nd one you mentioned won't work as we don't want even items in the same assembly able to touch that (hence private, not internal.) However, didn't really follow your 'putting the interface inside the class' thing as if only the `ItemsCollection` class knows about the interface, how can `Item` implement it? Can you give an example in your answer? – Mark A. Donohoe Jul 07 '11 at 16:15
  • Aaaah... I gotta be honest. Not a fan of that at all as our Item class is pretty complex with its interface. This would mean reproducing every single non-private property on the interface which gets really messy really quick. If I were going to go with the 'enclosing' choice, I'd put `public ItemsCollection` inside `Item` as at least that way you get the same result you have here but without the need for an interface. – Mark A. Donohoe Jul 07 '11 at 16:40
0

One solution I've used to control visibility of class members is to define the class as partial, and then in a different namespace declare the class as partial, and define the special visibility members you want.

This controls member visibility depending on the namespace chosen.

The only thing you'll have to wrap your head around is referencing. It can get complex, but once you have it figured out, it works.

IamIC
  • 17,747
  • 20
  • 91
  • 154
  • But if they are in two different namespaces, then they aren't really partial classes, but rather two different classes altogether, meaning you can't share member variables, etc. 'Partial' doesn't really mean/do anything without at least one other file pointing to the same class in the same namespace. This doesn't sound like a solution at all. – Mark A. Donohoe Apr 22 '13 at 03:28
  • They can be partial classes, and they can reference each other's variables if set up correctly. – IamIC Apr 23 '13 at 04:23
  • Im sorry, but I don't think you quite understand how partial classes work. It's not a runtime thing, nor is it for segregating functionality or scope like you suggest. Is simply to split a single class into multiple code files strictly for code organization, nothing else. It still compiles to one class. Also, partial class requires two files pointing to the same class *in the same namespace*, which you have stated you don't have. It more sounds to me like you've just split one class into two separate, distinct classes. Try removing the word 'partial' and I bet it still compiles. – Mark A. Donohoe Apr 23 '13 at 05:19
  • 1
    Here's more info confirming my statement: "Using the partial keyword indicates that other parts of the class, struct, or interface can be defined *within the namespace*. All the parts must use the partial keyword. All of the parts must be available at compile time to form the final type. *All the parts must have the same accessibility, such as public, private, and so on.*" (Source: http://msdn.microsoft.com/en-us/library/wa80x488(v=vs.80).aspx) – Mark A. Donohoe Apr 23 '13 at 14:33
  • Ok, point taken. It still offers a potential solution to the OP's question. – IamIC Apr 25 '13 at 08:52
  • 1
    But I *am* the OP! :) Thanks for trying though. It's just a shame there's no 'Friend' scope like in C++, but it does look like a private interface will suffice for the most part. Thanks again! – Mark A. Donohoe Apr 25 '13 at 16:49
  • Ohhh, my mistake. You're the OP :-) I suspect I didn't explain myself properly because I have used the suggestion I made to solve what amounts to your question. Yes, it was, as you said, 2 classes (which i didn't know). But I'm glad you worked out a solution that fits! – IamIC Apr 25 '13 at 16:56
0

The first thing that struck me with this scenario is that there is definite feature envy between ItemCollection and Item. I understand your desire to make adding the child item to the collection and setting the parent to be an autonomous operation, but really I think the responsibility of maintaining that relationship is in the Item, not the ItemCollection.

I would recommend exposing the ChildItems on Item as a Read-Only collection (with IEnumerable<Item> perhaps), and putting the AddChild(Item child),RemoveChild(Item child), ClearChildren(), etc methods on the Item. That puts the responsibility for maintaining the Parent with the Item where you don't have concerns leaking into other classes.

ckramer
  • 9,419
  • 1
  • 24
  • 38
  • Yeah... thought about that, but to me, it's really the collection that handles what's inside of it, not the item inside. Plus, to me the enumerators should be on the class you're acting on. I shouldn't have to think 'To add I use this class, but to enumerate, I use that one.' Also, when an item has multiple child collections (we actually have three), in my solution, each collection can just manage themselves. Yours would mean creating another new add/remove interface for each new type on Item, and that's not even including insertions or replacements which would require even more. – Mark A. Donohoe Jul 07 '11 at 16:36
  • You can always make your Item behave like a collection, or make your collection behave like an item for that matter....it depends on how your wanting to model things. One other approach that occurred to me: Make the setter protected, and add an item sub-type to the collection (something with a copy-constructor). That still doesn't solve the feature envy problem in my mind, though. – ckramer Jul 07 '11 at 16:52
0

How about you make sure that only the item's current collection can orphan the item. That way no other collection can set the item's parent while it belongs to a collection. You could use a unique key of some sort so that a third party couldn't get involved:

public sealed class ItemsCollection : ObservableCollection<Item>
{
    private Dictionary<Item, Guid> guids = new Dictionary<Item, Guid>();

    public ItemsCollection(Item owner)
    {
        this.Owner = owner;
    }

    public Item Owner { get; private set; }

    private Guid CheckParent(Item item)
    {
        if (item.Parent != null)
            throw new Exception("Item already belongs to another ItemsCollection");
        //item.Parent = this.Owner; // <-- This is where we need to access the private Parent setter     
        return item.BecomeMemberOf(this);

    }

    protected override void InsertItem(int index, Item item)
    {
        Guid g = CheckParent(item);
        base.InsertItem(index, item);
        guids.Add(item, g);
    }

    protected override void RemoveItem(int index)
    {
        Item item = this[index];
        DisownItem(item);
        base.RemoveItem(index);
    }

    protected override void DisownItem(Item item)
    {
        item.BecomeOrphan(guids[item]);
        guids.Remove(item);            
    }

    protected override void SetItem(int index, Item item)
    {
        var existingItem = this[index];
        if (item == existingItem)
            return;
        Guid g = CheckParent(item);
        existingItem.BecomeOrphan(guids[existingItem]);
        base.SetItem(index, item);
        guids.Add(item, g);
    }

    protected override void ClearItems()
    {
        foreach (var item in this)
            DisownItem(item);
        base.ClearItems();
    }
}




public class Item
{
    public string Name { get; set; }
    public Item Parent { get; private set; }

    public ItemsCollection ChildItems;

    public Item()
    {
        this.ChildItems = new ItemsCollection(this);
    }

    private Guid guid;

    public Guid BecomeMemberOf(ItemsCollection collection)
    {
        if (Parent != null)
            throw new Exception("Item already belongs to another ItemsCollection");
        Parent = collection.Owner;
        guid = new Guid();
        return guid; // collection stores this privately         
    }

    public void BecomeOrphan(Guid guid) // collection passes back stored guid         
    {
        if (guid != this.guid)
            throw new InvalidOperationException("Item can only be orphaned by its current collection");
        Parent = null;
    }
}

Obviously there is redundancy there; the item collection is storing a second item collection (the dictionary). But there are numerous options for overcoming that which I assume you can think of. It's beside the point here.

However I do suggest you consider moving the task of child-item management to the item class, and keep the collection as 'dumb' as possible.

EDIT: in response to your quesion, how does this prevent and item from being in two ItemsCollections:

You ask what the point of the guids is. Why not just use the collection instance itself?

If you replace the guid argument with a collection reference, you could add an item to two different collections like this:

{
    collection1.InsertItem(item);  // item parent now == collection1
    collection2.InsertItem(item);  // fails, but I can get around it:
    item.BecomeOrphan(collection1); // item parent now == null 
    collection2.InsertItem(item);  // collection2 hijacks item by changing its parent (and exists in both collections)
}

Now imagine doing this with the guid argument:

{
    collection1.InsertItem(item);  // item parent now == collection1
    collection2.InsertItem(item);  // fails, so...
    item.BecomeOrphan(????); // can't do it because I don't know the guid, only collection1 knows it.
}

So you can't add an item to more than one ItemsCollection. And ItemsCollection is sealed so you can't subclass it and override its Insert method (and even if you did that, you still couldn't change the item's parent).

Igby Largeman
  • 16,495
  • 3
  • 60
  • 86
  • The problem I see with this approach is that you're still putting the onus of collection management on the Item class, not the collection. Plus, unless I'm reading this wrong, you're also making the caller responsible for storing, then passing back in the GUID which over-complicates things. And how does the collection store the GUID anyway? More importantly, why?! It doesn't llok like it would stop someone from inserting the item directly into multiple collections unless they use this API, but it's not enforced. I admit maybe I just don't understand what you're doing here. – Mark A. Donohoe Jul 07 '11 at 16:49
  • The collection would need to keep a dictionary of . (Personally I'd have the item managing it though, not the collection). I'll edit my answer to make it clearer. Check back in a while. – Igby Largeman Jul 07 '11 at 17:34
  • No matter what you do, it will always be possible to insert the item into multiple collections. But this way you can ensure that only one collection can set the item's parent property, which is what you seem to be asking how to do. – Igby Largeman Jul 07 '11 at 18:10
  • Yes, but now you have a collection of items, maintaining a second collection of item-to-key pairs. You don't see the irony or redundancy there? ...let alone the overhead to use it?! And as you said, it actually doesn't stop someone from putting it in another collection, which is the real issue we're trying to stop. – Mark A. Donohoe Jul 07 '11 at 18:26
  • @Marq: As I mentioned in my (edited) answer, the redundancy is simple to remove, I just overlooked it for the sake of the exercise (just model your collection as a dictionary). If you want to stop the item from being added to another collection, the answer is: **you can't**. However you _can_ stop it from being added to another `ItemsCollection`, and this code does exactly that. – Igby Largeman Jul 07 '11 at 18:32
  • maybe I'm missing something, but what do you get with the whole GUID thing?! I mean Item has a parent which is the same as the collection's owner, let alone Item would be in that collection, so you know which collection controls it, and parent being set will stop it from being added to another collection. So again, what does thos whole 'GUID' thing even give you? I'm just not seeing it. – Mark A. Donohoe Jul 07 '11 at 18:42
  • Continuing that, couldn't you leaev your exact same interfaces except nix all references to GUID and just use the existence (or not) of Parent (and that it matches Owner of the collection) to determine if that collection can do anything with it? [Still scratching head] – Mark A. Donohoe Jul 07 '11 at 18:44
  • @Marq: I'll add a bit more to the answer. – Igby Largeman Jul 07 '11 at 19:04
  • still not seeing it. Maybe it's just me. Why couldn't you simply use a reference to the collection itself instead of the GUID? That way, only the actual collection that contains Item can be passed to BecomeOrphan. In other words, Item would have both a Parent, and a ContainingCollection property, the latter of which can be completely private. If you pass something else in that's not the 'containing' one, it fails. I just don't see why you're using a GUID at the item level to determine the owning collection *of* the item, not the item itself, which is what the GUID represents. – Mark A. Donohoe Jul 07 '11 at 19:31
  • Plus, what's to stop you from just calling Item.BecomeMemberOf with a collection? Sure it sets the parent, but it doesn't actually add it to the collection itself. And if the caller doesn't save the GUID, now you've locked an object that you can't Orphan. BTW, this is getting long and SO is showing me a warning here so we may want to take this off-line. – Mark A. Donohoe Jul 07 '11 at 19:35
  • @Charles let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/1238/discussion-between-marqueiv-and-charles) – Mark A. Donohoe Jul 07 '11 at 19:35
0

You can do these sorts of things using Delegates:

public delegate void ItemParentChangerDelegate(Item item, Item newParent);

public class Item
{
    public string Name{ get; set; }
    public Item Parent{ get; private set; }
    public ItemsCollection ChildItems;

    static Item()
    {
        // I hereby empower ItemsCollection to be able to set the Parent property:
        ItemsCollection.ItemParentChanger = (item, parent) => { item.Parent = parent };
        // Now I just have to trust the ItemsCollection not to do evil things with it, such as passing it to someone else...
    }
    public static void Dummy() { }

    public Item()
    {
        this.ChildItems = new ItemsCollection (this);
    }
}

public class ItemsCollection : ObservableCollection<Item>
{
    static ItemsCollection()
    {
        /* Forces the static constructor of Item to run, so if anyone tries to set ItemParentChanger,
        it runs this static constructor, which in turn runs the static constructor of Item,
        which sets ItemParentChanger before the initial call can complete.*/
        Item.Dummy();
    }
    private static object itemParentChangerLock = new object();
    private static ItemParentChangerDelegate itemParentChanger;
    public static ItemParentChangerDelegate ItemParentChanger
    {
        private get
        {
            return itemParentChanger;
        }
        set
        {
            lock (itemParentChangerLock)
            {
                if (itemParentChanger != null)
                {
                    throw new InvalidStateException("ItemParentChanger has already been initialised!");
                }
                itemParentChanger = value;
            }
        }
    }

    public ItemsCollection(Item owner)
    {
        this.Owner = owner;
    }   

    public Item Owner{ get; private set; }

    private CheckParent(Item item)
    {
        if(item.Parent != null) throw new Exception("Item already belongs to another ItemsCollection");
        //item.Parent = this.Owner;
        ItemParentChanger(item, this.Owner); // Perfectly legal! :)
    }

    protected override void InsertItem(int index, Item item)
    {
        CheckParent(item);
        base.InsertItem(index, item);
    }

    protected override void RemoveItem(int index)
    {
        ItemParentChanger(this[index], null);
        base.RemoveItem(index);
    }

    protected override void SetItem(int index, Item item)
    {
        var existingItem = this[index];

        if(item == existingItem) return;

        CheckParent(item);
        ItemParentChanger(existingItem, null);

        base.SetItem(index, item);
    }

    protected override void ClearItems()
    {
        foreach(var item in this) ItemParentChanger(item, null);
        base.ClearItems();
    }
Alex ten Brink
  • 899
  • 2
  • 9
  • 19
  • The problem I see here is you're 'newing' up the ItemsCollection inside of 'Item'. In our design, the collection already exists elsewhere and Item is added to it. We just want the collection that it's added to to be able to set itself as the parent. In other words, the collection is the only thing that can set the parent, and does so when it receives a new child Item. – Mark A. Donohoe Jul 21 '11 at 07:10
  • That's a valid point. I fixed it: there's quite a bit of 'ceremonial' or 'boilerplate' code now, but I think it does work and no longer has that issue. – Alex ten Brink Jul 21 '11 at 10:41
0

My answer is built off of two parts

  1. Why is it so "Unsafe" to have the Interface ISetParent?

private/internal access modifiers are ment to prevent from mistakes, not really "Secure Code".

Remember... you can call private methods using some Reflections/Invoke Etc...

.

2 . i usually make everything public and make sure both sides know how to handle each other,

ofcourse, there is a little ping-pong but it takes just few cycles (in this case i have a NamedSet)

    private IPhysicalObject partOf;
    public IPhysicalObject PartOf
    {
        get { return partOf; }
        set
        {
            if (partOf != value)
            {
                if (partOf != null)
                    partOf.Children.Remove(this.Designation);

                partOf = value;

                if (partOf != null)
                    partOf.Children.Add(this.Designation);
            }
        }
    }

    public virtual void Add(String key, IPhysicalObject value)
    {
        IPhysicalObject o;
        if (!TryGetValue(key, out o))
        {
            innerDictionary.Add(key, value);
            value.PartOf = Parent;
        }
    }

    public virtual bool Remove(String key)
    {
        IPhysicalObject o;
        if(TryGetValue(key, out o))
        {
            innerDictionary.Remove(key);
            o.PartOf = null;
        }
    }

Hope this helps... Good Day, Tomer W.

P.S. I'll never get how this Editor is working... should i HTML it, or should i not?

Tomer W
  • 3,395
  • 2
  • 29
  • 44
  • Thanks for the reply. Q's a bit old now and we have a work-around, but I'll look into what you suggested. (Not infront of my PC right now.) As for editing, don't think HTML but rather Markdown. Think of Markdown as plainly-readable text that knows how to display with a format. (Just google Markdown, or check out any of the editors out there.) – Mark A. Donohoe Feb 07 '12 at 23:41