[Design] Kompositionsfrage

Firefighter

Alles klar, geht klar.

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

GPC

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.

EXDW

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.

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