4

So, I've got a use-case similar to this one, but with some additional specifics that I feel warrant a new question. (related questions, for reference)

I'm writing a data structure to that implements a cycle. The basic design is something like this:

public class Cycle<T>
{
    public Node<T> Origin { get; private set; }
    public int Count { get; private set; }
}

public class Node<T>
{
    public Cycle<T> Cycle { get; private set; }
    public Node<T> Next { get; private set; }
    public Node<T> Previous { get; private set; }
    public T Value { get; set; }
}

However, I want to implement all of the following behaviors:

  1. Update Cycle.Count as nodes are inserted/deleted
  2. Allow an "empty" Cycle (i.e. Origin = null) to create a new Node for the Origin
  3. Prevent the construction of new Node objects outside of cases 1 & 2
  4. Avoid exposing methods that don't need to be called by classes other than these two

In C++, I'd simply make each class a friend of the other. But in C#, I don't see a way to make it work.

I know I can can fulfill all but #3 if I nest Node inside of Cycle and only expose a single constructor for Node, like so:

public class Cycle<T>
{
    public Node Origin { get; private set; }
    public int Count { get; private set; }
    
    public class Node
    {
        public Cycle<T> Cycle { get; private set; }
        public Node Next { get; private set; }
        public Node Previous { get; private set; }
        public T Value { get; set; }
        
        internal Node<T>(Cycle<T> cycle)
        {
            if (cycle.Origin != null)
                throw new InvalidOperationException();
            else
            {
                Cycle = cycle;
                Next = this;
                Previous = this;
                cycle.Origin = this;
                cycle.Count = 1;
            }
        }
    }
}

But as you can see, I can only go as far as internal, so I still have to verify data integrity or break encapsulation.

I do have one "clever" idea, but it's kind of a black magic answer:

public abstract class Cycle<T>
{
    public Node Origin { get; private set; }
    public int Count { get; private set; }
    
    public sealed class Node
    {
        private CycleInternal _cycle;
        public Cycle<T> Cycle { get { return _cycle; } }
        public Node Next { get; private set; }
        public Node Previous { get; private set; }
        public T Value { get; set; }
        
        // this constructor can be called by CycleInternal, but not other classes!
        private Node(CycleInternal cycle)
        {
            Cycle = cycle;
            Next = this;
            Previous = this;
            cycle.Origin = this;
            cycle.Count = 1;
        }
        
        private sealed class CycleInternal :  Cycle<T>
        {
            // this constructor can be called by Node, but not other classes!
            public CycleInternal() {}
        }
    }
}

In this case, my worry is that something else could inherit from Cycle; could that be prevented by making the constructor to Cycle private? Or am I just being paranoid here?

Community
  • 1
  • 1
Travis Reed
  • 1,572
  • 2
  • 8
  • 11
  • What's wrong with using `internal`? What are you trying to prevent from happening here? – Matthew Mar 27 '19 at 20:29
  • Maybe I’m not getting the topic, but why a Cycle instance inside a Node? Wouldn’t you also need an IEnumerable> in the Cycle? – Davide Vitali Mar 27 '19 at 20:31
  • @Matthew `internal` exposes it to other classes in the same assembly, which I don't want to do. This is part of a library, and I don't want to risk *anything* I don't have to. @DavideVitali Each `Node` is keeping track of which `Cycle` it is a part of in order to make calulating `Cycle.Count`, checking if two `Node`s are part of the same `Cycle`, and updating `Cycle.Origin` easier. `Cycle` only needs a reference to one node in order to implement `IEnumerable>` itself. – Travis Reed Mar 27 '19 at 20:49

1 Answers1

6

internal exposes it to other classes in the same assembly, which I don't want to do

My advice is: get over this fear and mark it internal.

I have heard this feature request -- that C# implement C++ style friend semantics -- many, many times. The feature is usually motivated by a concern that "if I allow any class in my assembly to party on the internal state, my coworkers will abuse the privilege".

But your coworkers already have the ability to abuse that privilege, because your coworkers can already add friends (in C++) or make internal methods that are back doors (in C#).

The C# accessibility system is simply not designed to protect you from scenarios in which people who have write access to the source code are hostile to your interests. Just as private means "this is an implementation detail of this class" and protected means "this is an implementation detail of this hierarchy, internal means "this is an implementation detail of this assembly". If your coworkers who are responsible for the correct operation of that assembly cannot be trusted to use their powers wisely, get better coworkers. If your types require access to each other's internal details to make the assembly as a whole work correctly, that is what internal is for, so use it.

Or, put another way, this is a social problem, not a technical problem, so you should solve it by applying social pressure. The C# accessibility system is not designed to solve problems that should be solved by code reviews. If your coworkers are abusing their privileges, code review time is the time to make them stop, the same way you would use code review to stop them from adding any other bad code to your project.

To answer your specific questions:

my worry is that something else could inherit from Cycle; could that be prevented by making the constructor to Cycle private?

Yes, an abstract class can have a private constructor, and then the only derived types are nested types.

I use that pattern quite frequently, and I make the nested types private. It's a bad smell in C# to make a nested type that is public.

Aside: Typically I do so to make "case classes", like:

abstract class ImmutableStack<T>
{
  private ImmutableStack() { }
  private sealed class EmptyStack : ImmutableStack<T> { ... }
  private sealed class NormalStack : ImmutableStack<T> { ... }

and so on. It's a nice way to make the implementation details of a type spread out amongst multiple classes but still all encapsulated into a single class.

am I just being paranoid here?

It sounds to me like yes.

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • 1
    I'm not worried about any *deliberate* abuse, I'm worried that someone (including my future self) will misunderstand how to use the class, and do something (in a different file) that compiles fine but fails at runtime (possibly silently). I'd rather make failure impossible (as long as nobody edits the class itself). – Travis Reed Mar 27 '19 at 22:05
  • @TravisReed would a thorough documentation help? include do's and dont's. code samples. etc – Jan Paolo Go Mar 27 '19 at 22:09
  • @TravisReed: In that case, you need two things: first, a test suite that verifies *public* behaviours of the type, and second, lots of `Debug.Assert` statements throughout your code that verify the *internal invariants* that must be maintained by the code. Run the test suite in debug mode before you check in every change, and if future you breaks a public behaviour or an internal invariant, you'll be notified and you can fix the problem. – Eric Lippert Mar 28 '19 at 14:59