Implement Immutable Collection in C#

Question

I would like to define some constant sequences (of bytes, specifically) in my C# library/API. Both length and content should not vary, preferably by any means (barring reflection). To this end, a byte[] array would not be sufficient, and a ReadOnlyCollection<T> can still be mutated. I have not done a full comparison yet, but this effort may be re-inventing ImmutableList<T>, in which case I would still appreciate feedback on the successes and weaknesses of this implementation. To be precise, this class is meant to be inherited by multiple derived classes, which actually define the constants.

Design & Style

• I have chosen to build this class on the most primitive interface (IEnumerable<T>) that I could; partially to preserve maximal compatibility, and partially to avoid having to hide or throw NotSupportedException for certain ICollection<T> methods and such. Does this have any drawbacks?

• The actual collection that contains the items is a private List<byte>.

• I have implemented a couple convenience fields, AsString, Count, and Length (Length feels more appropriate for the type, but the existence of the Count<> LINQ extension made me feel both should be included for consistency). I considered get-only properties, however, this Q&A on SO points out that readonly is a more explicit modifier. Are there other pros/cons that I am missing in this specific case?

• Given that I only intend to derive this class, I could mark it abstract, but could I also build it as an interface? (This would affect the readonly/get-only decision.) However, stylistically, it feels like derived classes should have an "is-a" relationship with Bytes rather than a "can-do".

• I have omitted all namespace qualifiers here, as seems to be the standard in C#. However, I come mostly from Python, and global imports and polluting the global namespace are almost unbearable to me. I also don't want to see System.Collections.Generic.[...] everywhere; is it an acceptable compromise to use, for example, using CollectionsGen = System.Collections.Generic, or would this horrify C# programmers reading my code?

Code

/// <summary>
/// Implementation of an immutable collection of bytes, with some extras.
/// </summary>
public class Bytes : IEnumerable<byte>
{
    private List<byte> bytesList;
    public readonly string AsString;
    public readonly int    Count;
    public readonly int    Length; // just a synonym for Count

    // accepts values from -Length to Length-1 (similar to Python indexing).
    public byte this[int i] { get { return this.bytesList[ i>=0 ? i : i+this.Length ]; } }

    public Bytes(IEnumerable<byte> bytes)
    {
        this.bytesList = new List<byte>(bytes);
        this.AsString  = string.Join(" ", this.Select(b => $"0x{b:X2}"));
        this.Count     = this.bytesList.Count;
        this.Length    = this.Count;
    }

    public IEnumerator<byte> GetEnumerator() { return this.bytesList.GetEnumerator(); }

    IEnumerator IEnumerable.GetEnumerator() { return this.GetEnumerator(); }

I think could generalize this to any type by defining public class MyImmutableCollection<T> : IEnumerable<T> with roughly the same body, but I have no need for that now.

Usage

var byteArray = new byte[] { 0x01, 0x02, 0x03 };
var bytesInst = new Bytes(byteArray);
byteArray[1] = 0xFF;
// observe that byteArray has changed, where bytesInst has not.
bytesInst[1] = 0xFF; // correctly generates a read-only compiler error

Answer 1

Abstractness

To be precise, this class is meant to be inherited by multiple derived classes, which actually define the constants.

Do you mean there will be many classes each of which contain a constant set of bytes, but are otherwise the same? If so, then there is no need to create a new class for each one. You could instead just provide static readonly instances for each constant, e.g.

public static class MagicNumbers
{
    public static readonly Bytes Example1 = new Bytes(new byte[] { 0xFF, 0xFF });
    public static readonly Bytes Example2 = new Bytes(new byte[] { 0xFE, 0xFF });
}

Readonlyness

This certainly provides a read-only interface, which is good. The only way to violate the read-onlyness would be with reflection (which it is essentially always fine to ignore).

You are right that the backing collection of a ReadOnlyCollection<T> can change; this is all part of the 'interface is gospel' approach of languages like C# (which personally I subscribe to whole-heartedly). Note that if you pass a ReadOnlyCollection<T> to someone else, however, that they can't modify it. This means it is up to whoever creates the backing collections for the ReadOnlyCollection<T> as to what can or cannot be done with it. ReadOnlyCollection<T>, however, is not a type I would use for anything, pretty much, because it unhelpfully implements IList<T> (among other things). It's also annoyingly named (in my opinion), because IReadOnlyCollection<T> doesn't expose indexed retrieval (that's IReadOnlyList<T> job).

It's good that your constructor takes an IEnumerable<T>. You might consider implementing IReadOnlyList<T> (which 'includes' IEnumerable<T>) if you think it appropriate; this would expose an efficient implementation of Count and T this[int] (more discussion about this below).

Inside your class you could also use a read-only collection instead of List<byte>, to enforce the contract internally, e.g. IReadOnlyList<T>.

Properties

Your Count and Length properties are, as you comment, redundant. Don't do this: it will only create confusion. Length is (in my experiance) pretty much exclusive to arrays: go with Count instead (it's the property exposed by IReadOnlyList<T>), and instead of storing the value, I'd suggest retrieving it from the actual list, which means the construtor has less to do (which means there is less to go wrong).

 public int Count => bytesList.Count;

It's good that your external properties were readonly (this is preserved with the 'expression-bodied property' syntax above). You might also want to make bytesList readonly (the class is fine as it is, because it's public API is sound, but this again will enforce the contract inside the class, making it easier to maintain and understand).

Concerning readonly vs getter-only, unless I have good reason (which basically never happens), I go with getter-only. While what Rob says in the linked thread is true, I personlly don't care what a class does so long as the API makes sense, and a public readonly field has two important disadvantages over getter-only properties: it can't implement an interface property (e.g. int IReadOnlyList.Count), and you can't quietly change it to be a property in future. For example, changing your public int read-only Count to public int Count => byteList.Count is a binary breaking change: any code that depends on the old field needs to be recompiled to work with the new property. If, however, you had made it public int Count { get; }, then you could make this change to the implementation without breaking any consuming code. One case where you might want this would be AsString: if you found that this was hardly ever being used, you might consider deferring the creation until it is needed: you can't do this with a readonly field, but you can easily replace a getter-only property with one which implements caching.

In practical terms, as Henk Holterman points out in the linked thread, readonly fields are a nightmare when it comes to serialisation. A getter-only field, however, can become a { get; private set; } field without issue if your serialisation scheme demands it. This is by no means ideal, but it doesn't change the external API, so the unpleasantness is contained within the class.

Indexer this[int]

You might consider the expression-bodied member syntax again:

public byte this[int i] => this.bytesList[ i >= 0 ? i : i + this.Count ];

Though it's just syntatic sugar we could live without, it is arguably a bit easier to read, and makes it immediately apprent that there is no set block.

If you did decide to implement IReadOnlyList, you would need 2 versions of this methods; one with the negative-index support, and one without.

byte IReadOnlyList<T>.this[int i] => this.bytesList[i];
public byte this[int i] => this.bytesList[ i >= 0 ? i : i + this.Count ];

This means that code which misuses the IReadOnlyList<T> interface will still get an out-of-bounds error, but anyone who has an instance of your class by name of the class can use the negative-indexing.

Namespace qualifiers

It's been a while since I used Python for anything substancial, but my understanding is that *-imports pull methods as well as classes, so the issue is lesser in C# (only pulls types and other namespaces). As you say, it's pretty standard for C#ers to use *-imports, and because it is expected, APIs are designed in a manner which assumes it: e.g. there are many more namespaces in C# than C++. Most importantly, however, is that C# fails at compile-time if there are observed name-collisions (e.g. even if Horse could be interpreted as two things, the code is fine so long as you don't try to use Horse unqualified). This means two things have to change for library authors to break your code - the introduction of a 'duplicate' method or type in a dependency in Python will quietly create problems - and you can't break your own (because it won't compile). The only leak in all this, is that classes in their own namespace are preferred over classes in an imported namespace (but only the author of that namespace can change the behaviour of their code, not someone else).

using CollectionsGen = System.Collections.Generic would mostly horrifying me because Gen is truncated. Anothing think you'll find in C# is that we prefer long easy-to-remember-and-understand names over short quicker-to-type (if you can remember them) names. The time-to-type is minimised by auto-complete in the IDE/editor.

Documentation

It's good that your class has an inline-summary, but inline-documentation (///) on all public non-inherited members significantly improved usabily. The Count == Length thing, for example, would be confusing no end, which is why you have a comment in your code to remind you that it is correct: this same comment should be available to the people using your code. The negative-indexing support is another thing that should be documented externally. The constructor should also state that it takes a copy of the IEnumerable; while this is implied (by virtue of taking an IEnumerable<T> rather than, say, an IReadOnlyList<T>), as a consumer of your code I would appreciate knowing that this is the intended long-term behaviour and not just an implementation detail.

Remember: code is consumed far more often than it is read or written (which people all too often seem to forget).

Misc

• I wouldn't bother with the lining stuff up; it's just effort to maintain.

• I'd also avoid things like "x>=2", and some IDEs will 'correct' it to x >= 2, so you'll just end up fighting them. If expressions need grouping, then use parenthesis, which are unambiguous.

• If you are worried about space, consider using a byte[] rather than a list (you can use LINQ's IEnumerable<T>.ToArray() (guarantees copy) to keep the code short. (Note that byte[] implements IReadOnlyList<T>).

Answer 2

There is no need to define a class at all. Everything you want to do, you can do with extension methods:

• The Length field is completely unnecessary. There is not a need to make Length a synonym for Count. Yes, it is unfortunate that in .NET some collections have Count and some have Length. It's a dumb inconsistency. Live with it.

• The indexer with the fancy wraparound behavior can be replaced by an extension method:

---

public static T Index<T>(this IImmutableList<T> items, int i) => 
  items[i >= 0 ? i : items.Count + i]

---

• The AsString can also be replaced with an extension method, or even better, two:

---

public static string WithSpaces<T>(this IEnumerable<T> items) =>
  string.Join(" ", items);

public static string AsHex(this IEnumerable<byte> bytes) => 
  bytes.Select(b => $"0x{b:X2}").WithSpaces();

---

And now you can delete class Bytes and just use an ImmutableList<byte> in all places where you used Bytes before. Want to index it with your fancy indexer? myBytes.Index(-1), done. Want a hex string? myBytes.ToHex(), done. No class required.

global imports and polluting the global namespace are almost unbearable to me

Well, you be you, but C# programmers do not consider using System.Collections.Generic; to be "polluting the global namespace". The notion that bringing useful types into scope is "pollution" is a strange one.

this class is meant to be inherited by multiple derived classes, which actually define the constants.

Please do not. There is no reason you've given to build an inheritance hierarchy for constant byte sequences. Just make an ImmutableList<byte> when you need one!

You may be suffering from a condition I call "object oriented happiness syndrome", which is a condition common to OO programmers who think they have to use OO techniques in order to solve problems that they don't actually have. You want an immutable sequence of bytes, then make an immutable sequence of bytes. Don't make a class and then have to figure out an inheritance mechanism that you don't need. OO techniques are for organizing large bodies of code written by teams that have to communicate across organizational boundaries; you don't have to use all that ceremony to represent what is essentially a string!