[Design] Kompositionsfrage


  • Administrator

    Hallo zusammen,

    Ich habe zwei Klassen, nennen wir sie Gebäude und Raum. Jeder Raum ist genau einem Gebäude zugeordnet. Wenn das Gebäude entfernt wird, werden auch die Räume vom Gebäude entfernt. Ein Raum ist somit ein Teil des Gebäude und kann ohne Gebäude nicht existieren (siehe Komposition bei UML).

    Sowas möchte ich nun in C# machen. Mein Problem dabei ist vor allem, dass ein Raum immer einem Gebäude zugeordnet werden soll. Wenn ich nun sowas mache:

    class Room { /* ... */ }
    
    class Building
    {
      public List<Room> Rooms { get; private set; }
    
      // ...
    }
    

    Dann habe ich das Problem, dass jemand einen Raum erstellen könnte, welcher er an zwei Gebäuden zuordnen kann. Das möchte ich ja verhindern.

    Bisher habe ich dazu drei Möglichkeiten gefunden:
    1. Ich mache aus dem Building eine Liste.

    class Building
      : IEnumerable<Room>
    {
      public Room CreateRoom()
      {
        // ...
      }
    
      // ...
    }
    

    Man kann nur mit CreateRoom Räume hinzufügen. Dadurch besteht nie die Gefahr, dass ein Raum an zwei Gebäuden zugeordnet wird.
    → Mir gefällt daran nicht, dass ich Funktionalitäten vermische. Ein Gebäude ist nunmal keine Liste. Ein Gebäude hat eine Liste von Räumen.

    2. Ich implementiere meine eigene Liste (z.B. leite ich von System.Collections.ObjectModel.Collection ab). Dem Raum übergebe ich eine Referenz auf sein Gebäude und lasse beim Einfügen prüfen, ob der Raum bereits einem Gebäude zugeordnet ist.

    class Room
    {
      public Building { get; internal set; }
    
      // ...
    }
    
    class Building
    {
      public RoomCollection Rooms { get; private set; }
    
      // ...
    }
    
    class RoomCollection
      : Collection<Room>
    {
      private readonly Building m_building;
    
      public RoomCollection(Building building)
      {
        m_building = building;
      }
    
      protected override void InsertItem(int index, Room room)
      {
        if(room.Building != null) throw new ArgumentException("...");
    
        base.InsertItem(index, room);
      }
    
      protected override void SetItem(int index, Room room)
      {
        Room oldRoom = this[index];
    
        if(oldRoom == room) return;
    
        if(room.Building != null) throw new ArgumentException("...");
    
        base.SetItem(index, room);
    
        oldRoom.Building = null;
        room.Building = m_building;
      }
    
      protected override void RemoveItem(int index)
      {
        Room oldRoom = this[index];
    
        base.RemoveItem(index);
    
        oldRoom.Building = null;
      }
    }
    

    → Daran stört mich die Referenz in Room . Das ist so eine doppelte Abhängigkeit zwischen Bulding und Room . Auch das die Liste im Gebäude den Raum modifizieren muss, erachte ich als unschön. Zudem kann man so immer noch, einen Raum aus einem Gebäude entfernen und einem anderen zuordnen, was nicht viel Sinn macht. Es spricht zwar grundsätzlich nichts dagegen, aber diese Funktionalität wird gar nicht benötigt.

    3. Ich kombiniere 1. und 2. Ich implementiere die eigene Liste und verhindere InsertItem und SetItem von aussen ( InvalidOperationException ...?). Dafür füge ich entweder Building und/oder der RoomCollection eine Methode CreateRoom hinzu. Dadurch besitzt Building wieder die Liste und ist nicht die Liste. Räume können nicht ausgetauscht werden. Und die Liste verändert den Raum nicht.
    → Das Problem, welches ich hier habe, ist, dass ich somit keinen Raum erstellen kann, bevor ich ihn einfüge. Zudem ist das Werfen von InvalidOperationException bei InsertItem und SetItem etwas unschön.

    Naja, zur eigentlichen Frage:
    Was würdet ihr wählen? 1, 2 oder 3? Oder vielleicht was ganz anderes?

    Ich tendiere eigentlich zur 3. Variante. Das ich ein Objekt benötige, bevor ich es in die Liste einfüge, stammt eigentlich aus Zeiten, als ich Business Objekte im GUI verwendet hatte. Dort wollte ich ein Objekt zum bearbeiten, bevor ich es in eine Liste einfüge. Heute habe ich diese Anforderung nicht mehr...
    Und was etwas störend ist, ist die Exception in einer Schnittstelle. Man hat ein Add zur Verfügung, kann es aber nie aufrufen. Sollte ich besser eine eigene Liste implementieren, also einen Wrapper bauen, statt von Collection zu erben?

    Ich bin mir aktuell einfach unschlüssig...
    (Vielleicht bräuchte ich auch einfach mal eine Pause :))

    Bin gespannt auf eure Meinungen.

    Grüssli



  • Wenn du sagst das ein Raum niemals ohne Gebäude existieren kann, was hälst du von einer Inner Class? Room als Inner Class von Building und dann eine CreateRoom Methode im Building anbieten. Dem Room verpasst du nen Internal(oder private) Ctor und dann kann auch keiner außerhalb(Internal Ctor) oder generell(private Ctor) einen Raum erstellen? Was meinst du?



  • Wie wäre es mit sowas?

    class Room {
        public int RoomNumber {
            get;
            private set;
        }
    
        public Room(int number) {
            this.RoomNumber = number;
        }
    }
    
    class Building {
        private List<Room> rooms;
        private static int roomCounter = 0;
    
        public IEnumerable<Room> Rooms {
            get { return this.rooms; }
        }
    
        public Building() {
            this.rooms = new List<Room>();
        }
    
        public Room CreateNewRoom() {
            Room r = new Room(roomCounter++);
            this.rooms.Add(r);
            return r;
        }
    }
    

    Auf diese Art und Weise stellst du sicher, dass ein Raum immer nur zu einem Gebäude gehört. Denn man kann zwar den Room-Konstruktor auch so aufrufen, hat aber keine Möglichkeit, die Klasse dann zu einem Building hinzuzufügen 🙂 Der einzige Weg ist also über CreateNewRoom. Das mit der Raumnummer ist jetzt auch nur so zum Spaß, damit da etwas Action ist 😉



  • Nur als Idee:

    public abstract Part<T>
    {
    	protected Part(T owner)
    	{
    		Owner = owner;
    	}
    
    	public T Owner { get; private set; }
    }
    
    public Building
    {
    	public void AddRoom(Room room)
    	{
    	}
    }
    
    public Room : Part<Building>
    {
    	public Room(Building owner)
    		: base(owner)
    	{
    	}
    }
    

  • Administrator

    @Firefighter,
    Den Vorteil der Inner Class sehe ich da jetzt nicht so ganz. Wo siehst du da den genauen Vorteil bei meiner Problemstellung? Das mit dem CreateRoom kann ich ja auch machen, ohne das die Klasse Room eine Inner Class von Building ist.

    GPC schrieb:

    Auf diese Art und Weise stellst du sicher, dass ein Raum immer nur zu einem Gebäude gehört. Denn man kann zwar den Room-Konstruktor auch so aufrufen, hat aber keine Möglichkeit, die Klasse dann zu einem Building hinzuzufügen 🙂 Der einzige Weg ist also über CreateNewRoom.

    Das wäre dann wohl ca. Variante 3. Nur das du keine eigene Collection nimmst, dafür ein IEnumerable<Room> zur Verfügung stellst. Das mit dem IEnumerable<Room> klingt interessant. Habe ich, glaube ich, auch schon mal gemacht.
    Da ich hier von einem alten Programm ausgehe, bin ich ich von Listen ausgegangen, welche einen Index benötigen. Wenn ich meine aktuellen Anforderungen überfliege, ist dies aber wahrscheinlich gar nicht mehr nötig ... *kritisch in die Anforderungen und auf die Klassendiagramme schaut*

    😕

    Werde mir das in aller Ruhe morgen bei einem Kaffee anschauen. 😃
    Danke.

    Gibt es irgendwelche bekannten Nachteile bei der Rückgabe von IEnumerable<T> Objekten? Will nur mal so aus Neugier nachfragen 🙂

    Edit:
    @David W,
    Aber dann müsste ich in der Collection prüfen, ob der Room bereits einem Building zugeordnet ist. Wäre somit Variante 2.

    Grüssli



  • Dravere schrieb:

    GPC schrieb:

    Auf diese Art und Weise stellst du sicher, dass ein Raum immer nur zu einem Gebäude gehört. Denn man kann zwar den Room-Konstruktor auch so aufrufen, hat aber keine Möglichkeit, die Klasse dann zu einem Building hinzuzufügen 🙂 Der einzige Weg ist also über CreateNewRoom.

    Das wäre dann wohl ca. Variante 3. Nur das du keine eigene Collection nimmst, dafür ein IEnumerable<Room> zur Verfügung stellst. Das mit dem IEnumerable<Room> klingt interessant. Habe ich, glaube ich, auch schon mal gemacht.
    Da ich hier von einem alten Programm ausgehe, bin ich ich von Listen ausgegangen, welche einen Index benötigen. Wenn ich meine aktuellen Anforderungen überfliege, ist dies aber wahrscheinlich gar nicht mehr nötig ... *kritisch in die Anforderungen und auf die Klassendiagramme schaut*

    Na ja, es gibt Enumerable.GetElement(int index).. aber das ist natürlich nicht so performant wie List[index] 😉

    Gibt es irgendwelche bekannten Nachteile bei der Rückgabe von IEnumerable<T> Objekten? Will nur mal so aus Neugier nachfragen 🙂

    In meinem Code nicht wirklich. Der große Vorteil ist aber, dass man zu der IEnumerable<Room> nichts hinzufügen kann von außen 🙂


  • Administrator

    GPC schrieb:

    Na ja, es gibt Enumerable.GetElement(int index).. aber das ist natürlich nicht so performant wie List[index] 😉

    Najo ...
    Enumerable.ElementAt

    MSDN schrieb:

    Remarks
    If the type of source implements IList<T>, that implementation is used to obtain the element at the specified index. Otherwise, this method obtains the specified element.

    Aber man wüsste von Aussen natürlich nicht, ob dies der Fall ist.

    In meinem Code nicht wirklich. Der große Vorteil ist aber, dass man zu der IEnumerable<Room> nichts hinzufügen kann von außen 🙂

    Und auch nichts ersetzen. Und wer will, kann sich mit ToArray oder ToList entsprechende Arbeitskopien machen.
    Ich sehe aber gerade, dass ich wahrscheinlich neben dem CreateRoom noch ein RemoveRoom anbieten muss. Kommt zwar sehr selten vor, aber wird leider benötigt. Und so gibt es wieder mehr eine Vermischung zwischen Building und der Liste.

    Wie wäre es mit sowas?

    class RoomCollection
      : IEnumerable<Room>
    {
      private List<Room> m_rooms;
    
      public int Count { get { return m_rooms.Count; } }
    
      public Room CreateNew()
      {
        var room = new Room();
        m_rooms.Add(room);
        return room;
      }
    
      public void Remove(Room room)
      {
        m_rooms.Remove(room);
      }
    
      public IEnumerator<Room> GetEnumerator()
      {
        return m_list.GetEnumerator();
      }
    
      public System.Collection.IEnumerator System.Collection.IEnumerable.GetEnumerator()
      {
        return GetEnumerator();
      }
    }
    
    class Building
    {
      public RoomCollection Rooms { get; private set; }
    
      public Building()
      {
        this.Rooms = new RoomCollection();
      }
    }
    

    Also eben schlussendlich einen Wrapper. Damit habe ich die Funktionalitäten getrennt und biete auch ein IEnumerable<Room> Objekt an.

    Grüssli



  • Die Idee hinter der InnerClass ist der selbe wie einer Enumerator-InnerClass in jeder Liste im Framework. Für mich ist da das Designkonzept sehr gut erkennbar. Man sieht sofort das diese Klasse unabdingbar zum Building gehört, schließlich liegt sie ja innerhalb der Klasse. Es war nur eine Idee um dein Design der Komposition noch mehr zu unterstreichen.


  • Administrator

    Firefighter schrieb:

    Die Idee hinter der InnerClass ist der selbe wie einer Enumerator-InnerClass in jeder Liste im Framework. Für mich ist da das Designkonzept sehr gut erkennbar. Man sieht sofort das diese Klasse unabdingbar zum Building gehört, schließlich liegt sie ja innerhalb der Klasse. Es war nur eine Idee um dein Design der Komposition noch mehr zu unterstreichen.

    Achso, ok.
    Inner Class verwende ich nicht gerne. Finde die meistens unübersichtlich. Es bläht den ganzen Code auf. Nicht nur im *.cs File, wo man Building definiert, weil dort dann auch noch der Room reinkommt sondern auch sonst, da man überall Building.Room hinschreiben muss. Gut, mit der Einführung von var ist das nicht mehr ganz so schlimm.
    Ich verwende meistens Inner Classes nur für Dinge, wo die Klasse nach aussen gar nicht existiert und die Klasse sehr klein ist - also eine kleine Hilfsklasse für eine andere Klasse.

    Grüssli



  • Alles klar, geht klar.

    Dann doch einfach über den internal/private Ctor und ne CreateRoom Methode?



  • Dravere schrieb:

    GPC schrieb:

    Na ja, es gibt Enumerable.GetElement(int index).. aber das ist natürlich nicht so performant wie List[index] 😉

    Najo ...
    Enumerable.ElementAt

    MSDN schrieb:

    Remarks
    If the type of source implements IList<T>, that implementation is used to obtain the element at the specified index. Otherwise, this method obtains the specified element.

    Ah, genau, sorry 😃 Ich benutz die Methode so selten, da verlass ich mich zu 100% auf IntelliSense und tipp nur "Element" ein 😉

    In meinem Code nicht wirklich. Der große Vorteil ist aber, dass man zu der IEnumerable<Room> nichts hinzufügen kann von außen 🙂

    Und auch nichts ersetzen. Und wer will, kann sich mit ToArray oder ToList entsprechende Arbeitskopien machen.
    Ich sehe aber gerade, dass ich wahrscheinlich neben dem CreateRoom noch ein RemoveRoom anbieten muss. Kommt zwar sehr selten vor, aber wird leider benötigt. Und so gibt es wieder mehr eine Vermischung zwischen Building und der Liste.

    Nun, das erscheint mir nicht wirklich als Problem. Schließlich verwaltet Building ja seine Rooms sowieso. Eine zusätzliche RemoveRoom(Room r)-Methode wäre weniger Arbeit, als eine komplette RoomCollection.
    Ich pers. bin kein Freund von eigenen FooCollection-Klassen.. aber das ist meistens Geschmacksfrage, denn vom OOD-Standpunkt ist es auch ok.

    Dravere schrieb:

    Inner Class verwende ich nicht gerne. Finde die meistens unübersichtlich. Es bläht den ganzen Code auf. Nicht nur im *.cs File, wo man Building definiert, weil dort dann auch noch der Room reinkommt sondern auch sonst, da man überall Building.Room hinschreiben muss. Gut, mit der Einführung von var ist das nicht mehr ganz so schlimm.
    Ich verwende meistens Inner Classes nur für Dinge, wo die Klasse nach aussen gar nicht existiert und die Klasse sehr klein ist - also eine kleine Hilfsklasse für eine andere Klasse.

    👍 Inner Classes in C# find ich scheiße.

    Edit: List<T> im öffentlichen Interface ist btw. immer eine schlechte Idee. Damit bringt man sich selbst um die Möglichkeit, später den Rückgabetyp ohne großen Aufwand zu ändern und etwas "schwächeres" wie IEnumerable<T> oder ICollection<T> zurückzugeben, da der Benutzer des Codes eventuell Methoden von List<T> benutzt (z.B. in-place Reverse oder Indexzugriff oder FindIndex etc), die andere Collections nicht anbieten.



  • Also ein "CreateRoom" welches ein Raum zurück gibt und dem aktuellen Objekt anfügt finde ich nicht gut, ein "CreateRoom" ist eher was für ne Factory oder n Builder, das gehört zum einen nicht in "Building" und zum anderen sieht man, wie gesagt, von außen nicht das es ein Raum nicht nur returniert sondern auch direkt hinzufügt.

    Wenn dann eher ein CreateAndAddRoom, aber da sieht man direkt das Problem, die Methode macht zuviel.

    Ne weitere Idee die mir gerade kommt wäre eventuell

    Es kann dann zwar immer noch ein Raum erstellt werden, aber nicht einem Gebäude hinzugefügt werden.
    Man könnte "Room" nun eine inner class im ConcreteRoomBuilder machen, aber näääh, inner classes sind bäh.

    public interface IRoom { }
    public class Room : IRoom { }
    
    public interface IRoomBuilder
    {
    	public IRoom CreateRoom(Building owner);
    }
    
    public class ConcreteRoomBuilder : IRoomBuilder
    {
    	public IRoom CreateRoom(Building owner)
    	{
    		if (owner == null)
    			throw new InvalidOperationException("The room can only be created for an existing building");
    
    		return new Room();
    	}
    }
    
    public class Building
    {
    	public Building()
    	{
    		_rooms = new List<IRoom>();
    	}
    
    	private List<IRoom> _rooms;
    
    	public IRoom[] Rooms
    	{
    		get { return _rooms.ToArray(); } // Array damit es keine referenz ist und von außen geändert werden kann
    	}
    
    	public void AddRoom(IRoomBuilder roomBuilder)
    	{
    		_rooms.Add(roomBuilder.CreateRoom());
    	}
    
    	public void RemoveRoom(Room room)
    	{
    		_rooms.Remove(room);
    	}
    }
    

    Noch eine Erweiterungsidee wäre

    public class Room : IRoom
    {
    	public Room(IRoomBuilder builder)
    	{
    		if (builder == null)
    			throw new InvalidOperationException("The room can only be created by a IBuilder");
    	}
    }
    
    public class ConcreteRoomBuilder : IRoomBuilder
    {
    	public IRoom CreateRoom(Building owner)
    	{
    		if (owner == null)
    			throw new InvalidOperationException("The room can only be created for an existing building");
    
    		return new Room(this);
    	}
    }
    

    Nur das wäre schon unnötig restriktiv -> YAGNI.



  • Jeeht klar hab verstanden das meine Idee Doof war 😃

    Zu der Idee von David. Ist in deinem Design das Interface IRoom wirklich immer leer oder war das jetzt nur zu Demonstrationszwecken geeignet? Wenn es wirklich immer leer ist finde ich das nämlich doof. Leere Interfaces sind genauso Kacke wie Inner-Classes(was ich ja jetzt verstanden habe). Dann doch lieber ein Attribut wenn es hier nur um das Markieren geht. Aber ich glaube es war nur zu Demo-Zwecken.



  • warum nicht ganz stupide und einfach so?! Damit ist es auch wirklich eine Komposition, weil ein Raum immer einem Gebäude zugeordnet ist.

    Building b1 = new Building();
    Building b2 = new Building();
    
    Room r = new Room(b1);
    
    b2.AddRoom(r);
    
    class Room
    {
    	Building owner;
    
    	public Room(Building building) //könnte man besser machen da AddRoom einmal unnötigerweise RemoveRoom aufruft
    	{
    		if (building == null)
    			throw new ArgumentNullException();
    
    		owner = building;
    		owner.AddRoom(this);
    	}
    
    	public void SetOwner(Building building)
    	{
    		if (building == null)
    			throw new ArgumentNullException();
    
    		owner.RemoveRoom(this);
    		owner = building;
    	}
    }
    
    class Building
    {
    	List<Room> rooms = new List<Room>();
    
    	public void AddRoom(Room room)
    	{
    		room.SetOwner(this);
    		rooms.Add(room);
    	}
    
    	public void RemoveRoom(Room room)
    	{
    		rooms.Remove(room);
    	}
    }
    

    greetz KN4CK3R


  • Administrator

    GPC schrieb:

    Nun, das erscheint mir nicht wirklich als Problem. Schließlich verwaltet Building ja seine Rooms sowieso. Eine zusätzliche RemoveRoom(Room r)-Methode wäre weniger Arbeit, als eine komplette RoomCollection.

    Ich habe lieber etwas mehr Arbeit, dafür eine saubere Trennung. So eine Klasse zu schreiben, ist ja schliesslich auch nicht gerade die grösste Aufgabe. Ich verbringe bereits mehr Zeit in diesem Thread 😉
    Und bei einer besseren Trennung von Aufgaben steigt die Wartbarkeit, dass musste ich nun schon zu oft feststellen 😃

    GPC schrieb:

    Edit: List<T> im öffentlichen Interface ist btw. immer eine schlechte Idee. Damit bringt man sich selbst um die Möglichkeit, später den Rückgabetyp ohne großen Aufwand zu ändern und etwas "schwächeres" wie IEnumerable<T> oder ICollection<T> zurückzugeben, da der Benutzer des Codes eventuell Methoden von List<T> benutzt (z.B. in-place Reverse oder Indexzugriff oder FindIndex etc), die andere Collections nicht anbieten.

    Ja, das ist klar. Immer das kleinst mögliche Interface anbieten. Trotzdem danke 🙂

    @David W,
    Interessante Analyse und Ansatz. Leider löst es das Problem nicht. Wenn jemand einen eigenen RoomBuilder implementiert, kann dieser Räume dem Gebäude hinzufügen, obwohl sie nicht zum Gebäude gehören. Ebenfalls störend finde ich, dass man dem AddRoom die Factory übergeben muss. Dies bläht die Schnittstelle unnötig auf, weil davon von Aussen eigentlich gar nichts sichtbar sein soll.

    Wie wäre es mit dem Namen: AddNewRoom ?
    Ich sehe leider keine andere Möglichkeit, dass eine Methode zwei Dinge tut (erstellen und hinzufügen), um die genannte Anforderung zu erfüllen. Sonst müsste ich zu einer Abwandlung von Variante 2 übergehen und mir in Room merken, wer den Room erstellt hat. Und dies dürfte dann nur die Factory sein des Gebäudes. Also sowas:

    public class Room
    {
      internal Room(RoomFactory creator)
      {
        this.Creator = creator;
      }
    
      internal RoomFactory Creator { get; private set; }
    }
    
    class RoomFactory
    {
      public Room CreateRoom()
      {
        return new Room(this);
      }
    }
    
    public class RoomCollection
      : Collection<Room>
    {
      private readonly RoomFactory m_validCreator;
    
      public RoomCollection(RoomFactory validCreator)
      {
        m_validCreator = validCreator;
      }
    
      public void InsertItem(int index, Room room)
      {
        if(room.Creator != m_validCreator)
        {
          throw new ArgumentException("wrong room");
        }
    
        base.InsertItem(index, room);
      }
    
      // usw.
    }
    
    public class Building
    {
      public RoomFactory RoomFactory { get; private set; }
      public RoomCollection Rooms { get; private set; }
    
      public Building()
      {
        this.RoomFactory = new RoomFactory();
        this.Rooms = new RoomCollection(this.RoomFactory);
      }
    }
    

    Erscheint mir irgendwie unnötig kompliziert, nicht? Und die Überprüfung gefällt mir nachwievor nicht. Da gefällt mir immernoch am meisten meine vorherige Lösung.

    Und noch was am Rande:
    Dein ToArray im Property halte ich für keine gute Idee. Es verhindert zwar, dass man von Aussen nicht die Innereien verändern kann, aber dies weiss man von Aussen nicht. Man müsste es in die Dokumentation einfügen. Zudem wird hier immer ein neues Array erstellt, ohne dass man davon weiss. Und es gibt nochwas für die Dokumentation. Daher halte ich es für eine schlechte Idee, da hier sehr schnell und einfach eine Fehlbenutzung entstehen kann.

    In der Vorschau noch gesehen:
    @KN4CK3R,
    Das ist doppelte Funktionalität. Soll man nun SetOwner oder AddRoom / RemoveRoom verwenden? Dies führt schnell zu Fehlern, wie es in deinem Code auch der Fall ist. Wenn jemand SetOwner aufruft, wird der Raum dem Gebäude nicht zugeordnet 😉
    Dann lieber etwas in die Richtung von Variante 2 in meinem Eröffnungsbeitrag, wo die Sache zentral von einem Ort geregelt wird.

    Grüssli



  • dann halt so:

    class Room
    {
    	Building building;
    
    	public Room(Building building)
    	{
    		SetBuilding(building);
    	}
    
    	public void SetBuilding(Building building)
    	{
    		if (building == null)
    			throw new ArgumentNullException();
    
    		if (this.building == building)
    			return;
    
    		if (this.building != null)
    			this.building.RemoveRoom(this);
    		this.building = building;
    		this.building.AddRoom(this);
    	}
    }
    
    class Building
    {
    	List<Room> rooms = new List<Room>();
    
    	public void AddRoom(Room room)
    	{
    		if (room == null)
    			throw new ArgumentNullException();
    
    		if (rooms.Contains(room))
    			return;
    
    		rooms.Add(room);
    		room.SetBuilding(this);
    	}
    
    	public void RemoveRoom(Room room)
    	{
    		if (room == null)
    			throw new ArgumentNullException();
    
    		if (rooms.Contains(room))
    			rooms.Remove(room);
    	}
    }
    

    Das ist dann keine doppelte Funktionalität, sondern notwendige Aktuallisierungsarbeit.

    greetz KN4CK3R


  • Administrator

    @KN4CK3R,
    Du hast hier eine zirkuläre Abhängigkeit mit deiner doppelten Funktionalität. Dadurch wird die Sache nur unnötig kompliziert. Zum Beispiel haben AddRoom und SetBuilding beide eine Laufzeit von O(n)!
    Schau dir meine Variante 2 im Eröffnungsbeitrag an. Ich mache dort etwas ähnliches, nur ist dort SetBuilding nicht öffentlich und wird von der Collection verwaltet. Dadurch sinkt die Komplexität um ein Vielfaches.

    Das ist definitiv keine gute Lösung.

    Grüssli



  • Firefighter schrieb:

    Zu der Idee von David. Ist in deinem Design das Interface IRoom wirklich immer leer oder war das jetzt nur zu Demonstrationszwecken geeignet?

    Habs nur leer gelassen da ich nicht weiß was Room anbieten soll, wird im reellen fall natürlich auch noch gefüllt.

    Wenn das mit dem Builder nicht erwünscht ist, dann eben die AddRoom Methode, nicht hundert prozentig Sauber aber es wurde schon genug Zeit vertrödelt ^^

    public class Building
    {
        public Building()
        {
            _rooms = new List<Room>();
        }
    
        private List<Room> _rooms;
    
        public ReadOnlyCollection<Room> Rooms
        {
            get { return _rooms.AsReadOnly(); }
        }
    
        public Room AddNewRoom()
        {
            var room = new Room();
            _rooms.Add(room);
            return room;
        }
    
        public void RemoveRoom(Room room)
        {
            _rooms.Remove(room);
        }
    }
    

    Danach kann ein Raum zwar immer noch erstellt, aber nirgends mehr verwendet werden. Kurz, Knapp, Effektiv, Schnell 😃



  • Jo, mein Vorschlag ist eindeutig der Beste :p Wo ist mein Designkrönchen?



  • Bekommst du sobald du das entsprechende Kleid anziehst 😃


Log in to reply