2

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.

6
  • So, what you're doing here is something like the Strategy pattern for deletion/removal; however, IMO, there are a couple of issues to be considered. One of them is inconsistency; you have 3 different higher-level representations (nodes, lines, arches), and methods related to each of them, but after refactoring, most of those methods remain in the main class, only Delete gets a special treatment. What is the end goal? 1/2 Aug 12, 2020 at 18:49
  • If the idea is to be able to add different kinds of higher-level shapes in the future, then you would need to separate all of these methods. If the kinds of shapes are fixed (or can be expected to change rarely), but you need a uniform interface, and you may need to add more operations on shapes, then it's probably better to do SoC by methods and use C# pattern matching capability to sort of "switch" on the concrete type. Aug 12, 2020 at 18:49
  • The end goal is to define classes that would take some of the responsibilities of the Geometry class, not just separate the responsibilities via methods. This IMO would at least improve the syntax. Also, I want to check that if in a similar case in which I would need to extend the implementation by adding shapes, I would not run into a situation in which I have to couple things that shouldn't be coupled in order to obtain a result.
    – Darius
    Aug 14, 2020 at 11:34
  • That's not so much an end goal as it is a description of what you want the structure to be - you have to think about what this structure gives you (beyond just the perceived improvement of syntax), in terms of design benefits. What tradeoffs does it make? It will make some things easier to change, and it will make other things harder to change. E.g. try adding a new kind of shape (and having client code work with it), without changing the Geometry class - you'll probably have to add a method (or a few methods) to it, like InsertNewShape. Test it out, than adjust based on that. Aug 14, 2020 at 16:35
  • 1
    That said, there's also an option to leave the code as is - if this part of the codebase changes very rarely (or never), then redesigning it doesn't really give you much. It's more or less wasted effort (unless you're doing it for practice/experimentation, for your own purposes). If it does change more often, then figure out what about the code changes, and reorganize it so that those kinds of changes are easier - that should be the "end goal" (design-wise). Aug 14, 2020 at 16:35

4 Answers 4

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

2
  • I accept this answer, because it answers directly and with demonstration, on what the question asks for, which is a principle or rule that stand against the refactoring approach I brought as suggestion. However, the question has a broader sense and I wonder if this could be seen as a general rule to be applied for every composition pattern.
    – Darius
    Aug 20, 2020 at 12:47
  • 1
    I think it can, exposing the composition is problematic in many cases, and i would avoid it., child objects that call their parent have similar issues but are sometimes used well for recursive patterns
    – Ewan
    Aug 20, 2020 at 12:51
4

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.

7
  • I was refactoring this code and the way I exemplified is the way which naturally comes for me to do the refactoring. However, in my discussions with other programmers, everyone kinda agreed that 'Remove' should be defined in collection class not in the item class, as if it is a rule for that. This creates a conflict in my head whether I should stop refactoring or finish the way I started.
    – Darius
    Aug 12, 2020 at 8:20
  • The original question may be if 'item.Remove' is faulty, but I had to formulate the question such that it covers a wider area in order to clarify the reason why "altering the composed ..." is bad.
    – Darius
    Aug 12, 2020 at 8:29
  • 2
    @Darius: 'item.Remove' is not faulty, since it works probably exactly the way it was intended. Yes, it has a side effect to the container, and maybe there could be some debate if the API gets more clearer by placing any container-mutating methods into the container itself. But in the end this is just a matter of taste.
    – Doc Brown
    Aug 12, 2020 at 9:42
  • I didn't believe it would be absurd to exist a principle which might state that addition and deletion of items in a collection should be managed only by collection itself, or another which states that composing classes shouldn't manage internal data in the class they compose. It could be in a form of a not so mandatory convention, but become a rule in time, if the alternatives are proven to be too prone to error.
    – Darius
    Aug 13, 2020 at 9:43
  • 1
    @Darius: many askers on this site are looking for rules how to develop software "without thinking about the special case", looking for some "best practice" or "general conventions". That seldom works, most designs are very dependend to the context and the specific requirements.Software engineering is a discipline of trade-offs.
    – Doc Brown
    Aug 14, 2020 at 11:31
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.

6
  • +1: I share the same concerns about elements belonging to multiple collections, and bucking established conventions of collections.
    – Erik Eidt
    Aug 12, 2020 at 14:50
  • 2
    To me, a substantial issue here is that the interface is incomplete and too hypothetical: constructors are missing parameters, and there are missing collection manipulation methods. I like to think that one could work through issues from the perspective of the consuming client (who needs an (ideally simple) model for how things are supposed to work), without necessitating implementation (though implementation would likely help the OP realize some issues).
    – Erik Eidt
    Aug 12, 2020 at 14:50
  • There is some truth here, but some of the arguments are IMHO debatable: (1) misses that "element.Delete()" was misnamed, it is actually a remove operation which internally calls "collection.Remove(element)" internally, which includes additional cleanup. That's intended behaviour, nothing wrong here. (2) The current design does not prevent this - an element now has intentionally a primary container, but it can still be part of a second one like an undo stack. (3) This is C#, the deletion is done by the garbage collector when needed, the "Delete" we see here is a misnamer, as I wrote. ...
    – Doc Brown
    Aug 12, 2020 at 16:08
  • I agree with what you are saying regarding collections but the relationship between a collection and its elements is that of aggregation, not composition. Correct me if I'm wrong. In my case, elements are strictly part of only one "collection", that's why I named that method 'Delete' and not 'Remove'. Under the hood, I could have IDisposable implemented on GeoEntity and call Dispose(true) on GeoEntity.Delete(). When your drawing something with a pencil and want to make a correction, you generally erase(delete) the wrong and redraw. This is what I mean by 'Delete'
    – Darius
    Aug 13, 2020 at 9:01
  • If you are saying that there is a convention inferred from of the way the most libraries are designed and it can be supported with facts(like your first reason points out), then I am pleased to adopt this approach. However, calling item.Translate(), item.Merge() or item.Delete() looks promising when considering the legibility and aesthetics of the code.
    – Darius
    Aug 13, 2020 at 9:02
1

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
}
2
  • This would eventually be a step forward in refactoring. The classes Point, Line and Arch are already present and what this implementation would improve IMO is that it will replace the array getters of GeoEntity: Point[] Points {get;}, Line[] Lines {get;} and Arch[] Arches {get;}; with these more specialised collection types.
    – Darius
    Aug 14, 2020 at 11:57
  • And now I also see that methods like Remove, Translate, DoSpecialAction, etc. would be moved from item class to collection<item> class.
    – Darius
    Aug 14, 2020 at 12:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.