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>
).
ReadOnlyList
or any of the other type from theSystem.Collections.Immutable
package. The negative indexing could then just be an extension method. Maybe if you showed us how this is supposed to be used it would make more sense. \$\endgroup\$ReadOnlyCollection
? I couldn't find any. \$\endgroup\$:)
. From what I've seenReadOnlyCollection
can be changed if you still have access to the wrapped collection. I think it can also be obtained from theItems
property. The msdn site's example shows mutation in the first case. \$\endgroup\$