Circular reference — architecture question

Question

This is probably a very beginner question but I have searched a lot of topics and couldn't really find the same situation, although I'm sure this kind of situation happens all the time.

My project/program is going to track changes to drawings on construction projects and send notifications to people when drawings are changed.

There will be many construction projects (job sites), which will in turn have many drawings in each one. Each drawing will have a couple of revisions (as they get changed, a new revision is created).

Here is my Project Class

public class Project
{
    private readonly List<Drawing> _drawings = new List<Drawing>(30);
    private readonly List<Person> _autoRecepients = new List<Person>(30);

    public int ID { get; private set; }
    public string ProjectNumber { get; private set; }
    public string Name { get; private set; }
    public bool Archived { get; private set; }
    public List<Person> AutoRecepients { get { return _autoRecepients; } }


    public Project(int id, string projectNumber, string name)
    {
        if (id < 1) { id = -1; }

        ID = id;
        ProjectNumber = projectNumber;
        Name = name;
    }


    public bool AddDrawing(Drawing drawing)
    {
        if (drawing == null) return false;
        if (_drawings.Contains(drawing)) { return true; }

        _drawings.Add(drawing);

        return _drawings.Contains(drawing);
    }


    public void Archive()
    {
        Archived = true;
    }

    public bool DeleteDrawing(Drawing drawing)
    {
        return _drawings.Remove(drawing);
    }

    public IEnumerable<Drawing> ListDrawings()
    {
        return _drawings.AsReadOnly();
    }

    public override string ToString()
    {
        return string.Format("{0} {1}", ProjectNumber, Name);
    }
}

Here is my Drawing Class

public class Drawing : IDrawing
{
    private List<IRevision> _revisions = new List<IRevision>(5);
    private List<IssueRecord> _issueRecords = new List<IssueRecord>(30);
    private IRevision _currentRevision;

    public int ID { get; private set; }
    public string Name { get; private set; }
    public string Description { get; set; }
    public Project Project { get; private set; }
    public IRevision CurrentRevision { get { return _currentRevision; } }


    public Drawing(int id, string name, string description, Project project)
    {
        // To be implemented
    }


    /// <summary>
    /// Automatically issue the current revision to all Auto Recepients
    /// </summary>
    public void AutoIssue(DateTime date)
    {
        AutoIssue(date, _currentRevision);
    }

    /// <summary>
    /// Automatically issue a particular revision to all Auto Recepients
    /// </summary>
    public void AutoIssue(DateTime date, IRevision revision)
    {

    }

    public void IssueTo(Person person, DateTime date, IRevision revision)
    {
        _issueRecords.Add(new IssueRecord(date, this, revision, person));

        throw new NotImplementedException();
    }


    public void IssueTo(Person person, DateTime date)
    {
        IssueTo(person, date, _currentRevision);
    }        

    public void IssueTo(IEnumerable<Person> people, DateTime date)
    {
        IssueTo(people, date, _currentRevision);
    }

    public void IssueTo(IEnumerable<Person> people, DateTime date, IRevision revision)
    {
        foreach (var person in people)
        {
            IssueTo(person, date, revision);
        }

    }

    public void Rename(string name)
    {
        if (string.IsNullOrWhiteSpace(name)) { return; }

        Name = name;
    }

    public void Revise(IRevision revision)
    {
        if (revision.Name == null ) return;

        _revisions.Add(revision);
        _currentRevision = revision;
    }

    public struct IssueRecord
    {
        public int ID { get; private set; }
        public DateTime Date { get; private set; }
        public IDrawing Drawing { get; private set; }
        public IRevision Revision { get; private set; }
        public Person Person { get; private set; }

        public IssueRecord(int id, DateTime date, IDrawing drawing, IRevision revision, Person person)
        {
            if (id < 1) { id = -1; }

            ID = id;
            Date = date;
            Drawing = drawing;
            Revision = revision;
            Person = person;
        }

    }
}

And here is the Revision struct

public struct Revision : IRevision
{        
    public int ID { get; private set; }
    public string Name { get; }
    public DateTime Date { get; set; }
    public IDrawing Drawing { get; }
    public IDrawingFile DrawingFile { get; private set; }

    public Revision(int id, string name, IDrawing drawing, DateTime date, IDrawingFile drawingFile)
    {
        if (name == null) { throw new ArgumentNullException("name", "Cannot create a revision with a null name"); }
        if (drawing == null) { throw new ArgumentNullException("drawing", "Cannot create a revision with a null drawing"); }
        if (id < 1) { id = -1; }

        ID = id;
        Name = name;
        Drawing = drawing;
        Date = date;
        DrawingFile = drawingFile;
    }

    public Revision(string name, IDrawing drawing, DateTime date, IDrawingFile drawingFile)
        : this(-1, name, drawing, date, drawingFile)
    {

    }

    public Revision(string name, IDrawing drawing)
        : this(-1, name, drawing, DateTime.Today, null)
    {

    }

    public void ChangeID(int id)
    {
        if (id < 1) { id = -1; }

        ID = id;
    }

    public void SetDrawingFile(IDrawingFile drawingFile)
    {
        DrawingFile = drawingFile;
    }
}

My question is to do with the project reference in the drawing class and the drawing reference in the revision struct. It seems like a bit of a code smell? It also seems like it may cause issues with serialization in the future. Is there a better way to do this?

It seems necessary for a drawing object to know what project it belongs to so that if I'm working with individual drawing objects I can know which project they belong to.

Similarly, each revision is essentially "owned" by or part of a drawing. A revision doesn't make sense without a drawing so it needs a reference to the drawing it belongs to?

Any advise would be much appreciated.

Answer 1

What you have are not so much circular references as two examples of

a parent-child relationship which is navigable from both ends.

Yes it is normal and acceptable and no it isn't a code smell. Yes, some serialization tools require you to hint. e.g. Newtonsoft.Json would want the ReferenceLoopHandling.Ignore setting.

Navigability as a concept is not always talked about in OO design, which is unfortunate because it's just the concept you want here. (It is an explicit term in UML).

You often don't need navigability from both ends. Parent-child relationships are often coded from parent to child only. This is really common. For instance, an invoiceline class rarely needs an explicit field for its parent invoice because most applications only ever look at the line after retrieving the parent invoice.

So the design decision is not,

"Does a revison make sense without a drawing?"

But

"Will I ever need to find a drawing given only a revision?"

My guess is that your revisions are like invoice lines, and don't need to navigate to their parent. The answer for the drawings <——> project relation is not obvious to me. (It is an analysis question about your domain, not a question about coding style).

There is a striking difference here between OO code and, for instance, SQL. In a SQL database, it must be the revision table that holds the reference to its parent drawing id. In OO code, the parent class nearly-always holds a reference to the children. The children often don't need a reference to their parent because the only way you access the children is by already having the parent.

Answer 2

Circular references are quite normal in C# programs and data models in general, so don't worry about them. They do have to be specially handled during serialization though.

Answer 3

Yes, it's a circular reference and yes it's a code smell. Furthermore I do think the smell is right in this instance, it is not a good OO design.

Disclaimers

1. It might be normal for C# programs as @Rugbrød puts it, I can't comment on that, I'm not a C# coder.

2. This kind of design might be ok for non-oo paradigms, such as "component-based" or procedural programming.

So you can ignore this smell I guess if this is the context of your code.

Details

The main problem is that you are modeling data, not behavior. You want to have the "data" right first, and then you will go on to think about actual functions you want to implement on top of that. Like displaying drawings, archiving, etc. You don't have those yet, but that is in your mind right?

The OO approach (as admittedly not everyone agrees with) is to model behavior. If you want your drawings archived, then implement Drawing.Archive(). And I don't mean setting a flag, I mean really copying it to cold-storage or whatever. The real business-function your application is supposed to do.

If you do that, then you will find, there are no behaviors that mutually need each other, since that is then one behavior obviously. What can happen is, that two behaviors need a third abstract one perhaps (sometimes called Dependency Inversion).

Answer 4

I think the only problem here is Drawing.CurrentRevision

Otherwise, a Revision belongs to a Drawing, which belongs to a Project.

CurrentRevision isn't really a property of the Drawing, it's a shortcut to one item in its 'Revisions' list.

How about changing it to a method GetCurrentRevision() and a CurrentRevisionID property? That way it's obvious that GetCurrentRevision should not be serialised, although the ID is.