I recently stumbled upon some code which looked wrong. I edited it only to see that my changes broke the code. I tried to structure the code in different other ways to make the code both look and be right, but I wasn't satisfied with any alternative I could come up with.
The logic of the code is as follows:
- A
Pathis made of zero or moreSegments. - A
Segmentcontains:- one or more properties (here only one for brevity), and
- a reference to the
Pathit is on.
- Invariants that must be upheld:
- The
Pathreference on aSegmentis always initialized. - A
Segmentis an item of somePath'sSegments. - The
Partreference on aSegmentreferences thePart, whichSegmentscontain theSegment.
- The
Here's the code I looked at, when I thought I had found a bug:
public class Path
{
public List<Segment> Segments { get; } = new List<Segment>();
}
public class Segment
{
public int Property { get; }
public Path Path { get; }
...
}
public static class Test
{
public static Path CreatePath()
{
var path = new Path();
var segment = new Segment(path, 42);
return path;
}
}
By just glimpsing at CreatePath and without inspecting further I concluded that there clearly was a bug, since segment wasn't added to path.Segments and thereby violating an invariant.
Apparently I was wrong as the constructor of Segment did more work than I expected.
The actual constructor looked like this:
public Segment(Path path, int property)
{
Property = property;
Path = path;
path.Segments.Add(this); // <--
}
While I acknowledge the intent of the constructor to ensure the invariants, I didn't like the way it was done mainly of two reasons:
- The constructor does more work than validating arguments and initializing fields.
CreatePathonly looks correct when you know the internals of the constructor.
This to me breaks the Principle of Least Astonishment -- at least I was astonished.
I tried to come up with alternative ways to accomplish this, but I'm not entirely satisfied with any of them.
The original code where the consistency is ensured inside the constructor of Segment. It's problem is, that segment looks unused in CreatePath if you don't know about the internals of the constructor.
namespace PathExample0
{
public class Path
{
public List<Segment> Segments { get; } = new List<Segment>();
}
public class Segment
{
public int Property { get; }
public Path Path { get; }
public Segment(Path path, int property)
{
Property = property;
Path = path;
path.Segments.Add(this);
}
}
public static class Test
{
public static Path CreatePath()
{
var path = new Path();
var segment = new Segment(path, 42);
return path;
}
}
}
This first alternative takes out the consistency part of the constructor and into the CreatePath. The constructor is now free of surprises and segment is clearly used in CreatePath.
The minor problem is now, that were are not guaranteed that segment is added to Segments of the right Path.
namespace PathExample1
{
public class Path
{
public List<Segment> Segments { get; } = new List<Segment>();
}
public class Segment
{
public int Property { get; }
public Path Path { get; }
public Segment(Path path, int property)
{
Property = property;
Path = path;
}
}
public static class Test
{
public static Path CreatePath()
{
var path = new Path();
var segment = new Segment(path, 42);
path.Segments.Add(segment);
return path;
}
}
}
My second attempt Adds an AddSegment to Path which ensures the consistency.
This still don't hinders you to call new Segment(somePath, 42) and break the consistency.
namespace PathExample2
{
public class Path
{
private readonly List<Segment> segments = new List<Segment>();
public IReadOnlyList<Segment> Segments => segments;
public void AddSegment(int property)
{
var segment = new Segment(this, property);
segments.Add(segment);
}
}
public class Segment
{
public int Property { get; }
public Path Path { get; }
public Segment(Path path, int property)
{
Property = property;
Path = path;
}
}
public static class Test
{
public static Path CreatePath()
{
var path = new Path();
path.AddSegment(42);
return path;
}
}
}
The third and last example I could come up with interfaces out Segment into ISegment and makes Segment a private class to Path.
This should now ensure full consistency between a Path and its Segments.
To me it feel like a cumbersome approach and almost as a misuse of interfaces to hide the constructor of Segment.
namespace PathExample3
{
public interface ISegment
{
Path Path { get; }
int Property { get; }
}
public class Path
{
private readonly List<Segment> segments = new List<Segment>();
public IReadOnlyList<ISegment> Segments => segments;
public void AddSegment(int property)
{
var segment = new Segment(this, property);
segments.Add(segment);
}
private class Segment : ISegment
{
public int Property { get; }
public Path Path { get; }
public Segment(Path path, int property)
{
Property = property;
Path = path;
}
}
}
public static class Test
{
public static Path CreatePath()
{
var path = new Path();
path.AddSegment(42);
return path;
}
}
}
So my question is how to structure the code in a way, to ensure the invariants while retaining idiomatic C# code. Maybe I'm missing out on a well-known design pattern to accomplish this?
