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
andradii
) that holds the data that is being interpreted into abstract objects such as Point, Arch or Line. - Classes
Point
,Arch
andLine
inherit from abstract classGeoEntity
which has a dependency onGeometry
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.
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.InsertNewShape
. Test it out, than adjust based on that.