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 Path is made of zero or more Segments.
  • A Segment contains:
    • one or more properties (here only one for brevity), and
    • a reference to the Path it is on.
  • Invariants that must be upheld:
    • The Path reference on a Segment is always initialized.
    • A Segment is an item of some Path's Segments.
    • The Part reference on a Segment references the Part, which Segments contain the Segment.

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.
  • CreatePath only 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?

migrated from codereview.stackexchange.com Nov 16 '17 at 15:11

This question came from our site for peer programmer code reviews.

  • 1
    (In my eyes, your question would benefit from an introductory summary where all of it is going to, and a (list of) explicit question(s) at the end.) – greybeard Nov 16 '17 at 14:02
  • My gut feeling is that the further you separate creating the Segment from adding it to the Path, the more chance for bugs, especially concurrency bugs. But needs more analysis. – user949300 Nov 16 '17 at 23:35
  • 1
    A relevant counterexample to the "constructors must have no side effects" is SWT graphic widgets, which always take a parent widget and always add themselves. – user949300 Nov 17 '17 at 21:00
up vote 10 down vote accepted

A way to avoid usage of a misleading constructor is to make it private and instead provide a static factory method stating what it does. It also allows you to keep the constructor clean. (I would not say that is has no side-effect. A function changing some state besides returning a value is said to have a side-effect. Since constructors are not functions and cannot return any value, producing side-effects is all they can do. They change the state of the object from uninitialized to initialized.)

public class Segment
{
    private Segment(Path path, int property)
    {
        Path = path;
        Property = property;
    }

    public int Property { get; }
    public Path Path { get; }

    public static Segment AddToPath(Path path, int property)
    {
        var segment = new Segment(path, property);
        path.Segments.Add(segment);
        return segment;
    }
}

You can use this static factory method in the Path class

public class Path
{
    private readonly List<Segment> segments = new List<Segment>();

    public IReadOnlyList<Segment> Segments => segments;

    public Segment AddSegment(int property)
    {
        return Segment.AddToPath(this, property);
    }
}

Now it is not possible anymore to create a segment without adding it to a path and without setting the reference to its path.

It is unclear why segment has to reference a path. It does not strike me as particularly good idea. If it can be avoided - that is what I would refactor to begin with. The problem you are having is a direct result of having a circular dependency, which (in my experience) is never a good thing. I'm afraid there is no pretty way to solve this problem, unless you eliminate the cause.

That being said, if you have to keep the Path reference for w/e reason, I think your last option is the best one. I don't quite see why would you need an interface though. As long as AddSegment(int property) is the only way to add a segment to path, it does not matter whether or not Segment class is public. Path class will stay consistent. However I would probably add an overload that takes a segment:

public class Path
{
    private readonly List<Segment> segments = new List<Segment>();

    public IReadOnlyList<Segment> Segments => segments;

    public void AddSegment(int property)
    {
        AddSegment(new Segment(property));
    }

    public void AddSegment(Segment segment)
    {
        if (segment.Path != null) throw ...; 
        segment.Path = segment;
        segments.Add(segment);
    }
}

public class Segment
{
    public int Property { get; }

    public Path Path { get; internal set; }

    public Segment(int property)
    {
        Property = property;
    }
}
  • The bi-directional reference is just like in a graph, where nodes and edges could have references to each other. I wasn't explicit enough about it, but a Segment must always be a part of a Path and therefore I wanted to avoid a public constructor of Segment. – Jonas Nyrup Nov 17 '17 at 15:18

You can enforce consistency to a certain degree by making the Segment constructor internal, and putting the Path and Segment classes in a different DLL than your main application code:

// In one DLL
public class Path
{
    private List<Segment> segments;

    public IEnumerable<Segment> Segments => segments;

    public Segment AddSegment(int property)
    {
        var segment = new Segment(this, property);

        segments.Add(segment);

        return segment;
    }
}

public class Segment
{
    public int Property { get; private set; }
    public Path Path { get; private set; }

    internal Segment(Path path, int property)
    {
        Path = path ?? throw new ArgumentNullException(nameof(path));
        Property = property;
    }
}

Then use the Path class in another DLL file:

// In another DLL

var path = new Path();
var segment1 = path.AddSegment(42);   // Compiles
var segment2 = new Segment(path, 56); // Does not compile. Constructor is internal

Because the Segment constructor is marked internal you must go through the Path.AddSegment method to create new Segment objects.

The Segment constructor is free of side effects, because the Path.AddSegment method adds the new segment to the Path's collection.

  • Not a bad idea, but forcing people to move classes between DLLs is quite invasive advice. Usually we don't have a lot of choice in these matters. – Frank Hileman Nov 17 '17 at 1:48

If created Segment absolutely must be referenced be a Path, it reflects some business requirements, and it seems that this requirement constitutes a Segment's invariant. And objects should be created always valid, with all invariants ensured. Since the code executed while object creation resides in constructor, it's very logical place for enforcing his invariants.

And I agree with Nikita, I'm sure you could avoid that circular dependency. It's considered a bad practice, at least in Domain-driven design.

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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