[Design] Kompositionsfrage

Firefighter

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.

KN4CK3R

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

Dravere

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

KN4CK3R

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

Dravere

@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

EXDW

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

GPC

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

EXDW

Bekommst du sobald du das entsprechende Kleid anziehst

Dravere

David W schrieb:

Bekommst du sobald du das entsprechende Kleid anziehst

Hmmm ...

Wollt ihr übrigens noch eine Verkomplizierung des Designs? Die erstellen Räume können einer anderen Liste dazugefügt werden. Wenn sie aus dem Gebäude rausgelöscht werden, sollen sie ebenfalls aus der anderen Liste verschwinden. À la ON DELETE CASCADE

Vorschlag:

public sealed class Room
  : IDisposable
{
  public event EventHandler Disposed;

  public void Dispose()
  {
    var handler = this.Disposed;

    if(handler != null)
    {
      handler(this, new EventArgs());
    }
  }
}

public class RoomCollection
  : IEnumerable<Room>
{
  private List<Room> m_rooms;

  public Room AddNewRoom()
  {
    var room = new Room();
    room.Disposed += this.OnRoomDisposed;

    m_rooms.Add(room);

    return room;
  }

  private void OnRoomDisposed(object sender, EventArgs e)
  {
    var room = (Room)sender;

    m_rooms.Remove(room);

    room.Disposed -= this.OnRoomDisposed;
  }
}

...

Grüssli

GPC

Ja, wenn das auch noch dazukommt, dann macht die eigene Collection Sinn