Altering the state of a composed class by its composing classes. Is there any rule or principle for not doing so?

Question

In other words, is it good to define the method that removes an element in a collection inside the class representing the element, considering a composition relationship?

Something like: listElement.Delete();

In the following example, I'm refactoring the code by creating additional classes which are taking over some of the responsibilities of the main class(Geometry). Hopefully, following the SoC principle.

Notes:

• The Geometry class has fields(nodes and radii) that holds the data that is being interpreted into abstract objects such as Point, Arch or Line.
• Classes Point, Arch and Line inherit from abstract class GeoEntity which has a dependency on Geometry class using dependency injection on it's constructor.

Before refactoring

public class Geometry
{
    private List<Vector2> nodes;
    private Dictionary<int, double>[] radii;

    public void DrawLine() { // Do the magic.}
    public void InsertPoint() { // Do the magic.}
    public void InsertArch() { // Do the magic.}

    public void TranslateNode(double dx, double dy) { // Do the magic.}
    public void TranslateLine(double dx, double dy) { // Do the magic.}

    public void RemoveNode(int index) { // Do the magic.}
    public void RemoveLine(int index) { // Do the magic.}
    public void RemoveArch(int index) { // Do the magic.}

    public void DoSpecialNodeRelatedAction1() { // Do the magic.}
    public void DoSpecialNodeRelatedAction2() { // Do the magic.}
    public void DoSpecialLineRelatedAction(double someValue) { // Do the magic.}
}

After refactoring

public class Geometry
{
    private List<Vector2> nodes;
    private Dictionary<int, double>[] radii;

    public Geometry.Point[] Points { get => // Get them magically. }
    public Geometry.Line[] Lines { get => // Get them magically. }
    public Geometry.Arch[] Arches { get => // Get them magically. }

    public void DrawLine() { // Do the magic.}
    public void InsertPoint() { // Do the magic.}
    public void InsertArch() { // Do the magic.}


    public abstract class GeoEntity
    {
        private readonly Geometry geometry;

        protected GeoEntity(Geometry geometry, int index) 
        {
            this.geometry = geometry;
            this.Index = intex;
        }

        public int Index { get; }

        protected abstract void DoSpecificDeletion();        
        public void Delete()
        {
            DoSpecificDeletion();
            geometry.nodes.Remove(Index);
            
            var exists = radii.TryGetValue(Index, out var kvp);
            if(exists) radii.Remove(Index);
        }
    }

    public class Point : GeoEntity
    {
        internal Point(Geometry geometry, int Index) : 
            base(geometry, index) {}

        protected override void DoSpecificDeletion() { // Do the magic.}
        public void Translate(double dx, double dy) { // Do the magic.}
        public void DoSpecialAction1() { // Do the magic.}
        public void DoSpecialAction2() { // Do the magic.}
    }

    public class Line : GeoEntity
    {
        internal Line(Geometry geometry, int Index) : 
            base(geometry, index) {}

        protected override void DoSpecificDeletion() { // Do the magic.}
        public void Translate(double dx, double dy) { // Do the magic.}
        public void DoSpecialAction(double someValue) { // Do the magic.}
    }

    public class Arch: GeoEntity
    {
        internal Arch(Geometry geometry, int Index) : 
            base(geometry, index) {}

        protected override void DoSpecificDeletion() { // Do the magic.}
    }
}

---

Additional notes:

The refactoring in this case should enforce the SoC principle resulting into a cleaner structure with multiple smaller classes, each responsible to alter the data in Geometry class in their specific way, rather than having all methods defined into Geometry class.

A possible issue that I found is shown in the example:

void GeometryConsumingMethod(Geometry geometry)
{
    var a = geometry.Points[0];
    var b = geometry.Points[0];
    
    a.Delete();
    a.DoSpecialAction1();    // Possible logical error.
    b.DoSpecialAction1();    // Possible logical error.
}

However, I'm not sure if this is acceptable or not from an OOP perspective.

I'm curious what else could be wrong with this approach.

Answer 1

It's wrong as it breaks everyone's favourite principle the Liskov Substitution Principle,

Inorder to remove itsself from its parent geometry the point object needs a reference to its parent.

If Geometry doesnt expose a DeletePoint or a Points.Delete method, then Point will need to know about the internals of the class in order to delete itself.

However, I need to be able to replace that parent with an inherited class. say 4DGeom : Geometry If no DeletePoint method is exposed then 4DGeom is free to change its internal lists and may break the operation of Point.Delete

If Geometry does expose a DeletePoint method then you have got the same method twice.

Does 4DGeom.DeletePoint(p) call p.Delete()? or does P.Delete() call g.DeletePoint(p)? better make sure all your classes do it the same way or you're in trouble again

Answer 2

I think you are asking the wrong question, since you expect to find a braindead general design rule which tells you if this kind of design is "good" or "object oriented" (whatever that means). I doubt there is such a rule. Better starting thinking about the specific case and ask if the design can be improved to achieve some specific goals, like

• is current API is understandable?

• can it be made less error prone?

These goals in mind, I would suggest to rename the Delete method into Remove, since this is actually what it does. GeoEntity objects are not literally deleted by the Delete method, they are only removed from their container. An even better name could be RemoveFromParent or RemoveFromContainer, to make the side effect more visible.

I would also suggest to set the reference GeoEntity.geometry to null during the removal. That give methods which require GeoEntity to be part of a container to check if this is still the case, and if not, throw an exception with a clear error message.

About your "issue": Remove operations on a container will always invalidate part of its content, that is something you cannot really avoid (not even in your first design): if the code would look like this

void GeometryConsumingMethod(Geometry geometry)
{
    int aIndex = 0;
    int bIndex = 0;
    
    geometry.RemoveNode(aIndex);
    geometry.DoSpecialAction1(aIndex);  
    geometry.DoSpecialAction1(bIndex); 
}

one will get the same logical issues as in your new design. So this is not a reason against having a item.Remove operations.

Answer 3

This is one thing that annoys me about OOP design. People think they can have these debates looking just at the interface, when there is a clear winner when you look at the actual implementation.

There are a few big reasons why the convention is for the collection class to own modifications to the collection:

• The collection owns the physical memory holding the list of elements. With refactors like yours, your element.Delete() almost always ends up needing to call some sort of collection.Delete(element).
• Elements can usually belong to multiple collections, and be operated on independently of a collection. Your design doesn't account for that. Even if conceptually a node can belong to exactly one geometry, you have collections like selected nodes or states in an undo stack.
• The programming language already has a concept of when an object is deleted, and destructors to perform actions upon deletion. You're creating this weird third state where it's deleted according to your program, but the programming language still has a reference to it.
• There's also something to be said for consistency. It requires a strong justification to buck the convention established by every standard library collection your colleagues already know.

There are exceptions to every rule, but conventions are usually there for a good reason. You don't have to take my word for it. These issues should become apparent when you get further into the implementation of this code and the code that calls into it.

Answer 4

A conventional approach (i.e. an approach that adheres to conventions found in the CLR and elsewhere) would be to use the built-in interfaces that represent collections, e.g. IList<T> or ICollection<T>.

public interface IGeoEntityCollection<T> : ICollection<T> where T : GeoEntity
{
}

public class GeoEntityCollection<T> : IGeoEntityCollection<T> where T : GeoEntity
{
    protected readonly Geometry _parent;

    public GeoEntityCollection(Geometry parent)
    {
        _parent = parent;
    }

    public IEnumerator<T> GetEnumerator() => _parent.Nodes.OfType<T>().GetEnumerator();

    public void Remove(T item) 
    {
        //Do the magic
    }

    //Implement the rest of ICollection (Add, Clear, Contains, CopyTo, Count, IsReadOnly)
}


public class Geometry
{
    private List<Vector2> nodes;
    private Dictionary<int, double>[] radii;

    public IGeoEntityCollection<Point> Points { get; }
    public IGeoEntityCollection<Line> Lines { get; }
    public IGeoEntityCollection<Arch> Arches { get; }

    public Geometry()
    {
        this.Points = new GeoEntityCollection<Point>(this);
        this.Lines = new GeoEntityCollection<Line>(this);
        this.Arches = new GeoEntityCollection<Arch>(this);
    }
}

By designing it this way, you can put the add/remove implementation in a single place, and perhaps define it generically instead of three times. But you also have the option to define a dedicated collection type if, say, the logic to remove an Arch is different from the logic to remove the others.

class ArchCollection : IGeoEntityCollection<Arch>
{
    //Specialized logic
}