14

I understand how to work with interfaces and explicit interface implementation in C#, but I was wondering if it's considered bad form to hide away certain members that would not be used frequently. For example:

public interface IMyInterface
{
    int SomeValue { get; set; }
    int AnotherValue { get; set; }
    bool SomeFlag { get; set; }

    event EventHandler SomeValueChanged;
    event EventHandler AnotherValueChanged;
    event EventHandler SomeFlagChanged;
}

I know in advance that the events, while useful, would very rarely be used. Thus, I thought it would make sense to hide them away from an implementing class via explicit implementation to avoid IntelliSense clutter.

  • 3
    If you're referencing your instance via the interface it wouldn't hide them anyway, it'd only hide if your reference is to a concrete type. – Andy Oct 27 '14 at 23:27
  • 1
    I worked with a guy who did this on a regular basis. It drove me absolutely nuts. – Joel Etherton Oct 28 '14 at 19:54
  • ... and start using aggregation. – mikalai Oct 31 '14 at 19:46
39

Yes, it's generally bad form.

Thus, I thought it would make sense to hide them away from an implementing class via explicit implementation to avoid IntelliSense clutter.

If your IntelliSense is too cluttered, your class is too big. If your class is too big, it's likely doing too many things and/or poorly designed.

Fix your problem, not your symptom.

  • Is it appropriate to use it to remove compiler warnings about unused members? – Gusdor Oct 28 '14 at 12:24
  • 11
    "Fix your problem, not your symptom." +1 – FreeAsInBeer Oct 28 '14 at 12:49
11

Use the feature for what it was intended for. Reducing namespace clutter is not one of those.

Legitimate reasons aren't about "hiding" or organization, they are to resolve ambiguity and to specialize. If you implement two interfaces that define "Foo", the semantics for Foo might differ. Really not a better way to resolve this except for explicit interfaces. The alternative is to disallow implementing overlapping interfaces, or declaring that interfaces cannot associate semantics (which is just a conceptual thing anyway). Both are poor alternatives. Sometimes practicality trumps elegance.

Some authors/experts consider it a kludge of a feature (the methods aren't really part of the type's object model). But like all features, somewhere, someone needs it.

Again, I would not use it for hiding or organization. There are ways to hide members from intellisense.

[System.ComponentModel.EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)]
  • "There are prominent authors/experts that consider it a kludge of a feature." Who is? What is the proposed alternative? There is at least one issue (i.e. specialization of interface methods in a subinterface/implementing class) that would require some other feature to be added to C# in order to solve it in the absence of explicit implementation (see ADO.NET and generic collections). – jhominal Oct 28 '14 at 13:47
  • @jhominal - CLR via C# by Jeffrey Richter specifically uses the word kludge. C++ gets along without it, the programmer has to explicitly refer to the base method implementation to disambiguate, or use a cast. I already mentioned that the alternatives aren't very attractive. But its an aspect of single inheritance and interfaces, not OOP in general. – codenheim Oct 28 '14 at 14:43
  • You could also have mentioned that Java does not have explicit implementation either, despite having the same "single inheritance of implementation+interfaces" design. However, in Java, the return type of an overriden method can be specialized, obviating part of the need for explicit implementation. (In C#, a different design decision was made, probably in part because of the inclusion of properties). – jhominal Oct 28 '14 at 15:32
  • @jhominal - Sure thing, when I have a few minutes later I will update. Thanks. – codenheim Oct 28 '14 at 17:28
  • Hiding may be entirely appropriate in situations where a class might implement an interface method in a fashion which complies with the interface contract but isn't useful. For example, while I dislike the idea of classes where IDisposable.Dispose does something but is given a different name like Close, I would consider it perfectly reasonable for a class whose contract expressly disclaims states that code may create and abandon instances without calling Dispose to use explicit interface implementation to define an IDisposable.Dispose method that does nothing. – supercat Oct 28 '14 at 22:36
5

I might be a sign of messy code, but sometimes the real world is messy. One example from .NET framework is ReadOnlyColection which hides the mutating setter [] ,Add and Set.

I.E ReadOnlyCollection implements IList< T>. See also my question to SO several years ago when i pondered this.

  • Well it's my own interface I'll be using as well, so there's no sense in doing it wrong from the start. – Kyle Baran Oct 27 '14 at 21:04
  • 2
    I wouldn't say it hides the mutating setter but rather that it doesn't provide it at all. A better example would be ConcurrentDictionary, which implements various members of IDictionary<T> explicitly so that consumers of ConcurrentDictionary A) won't call them directly and B) can use methods which require an implementation of IDictionary<T> if absolutely necessary. E.g., users of ConcurrentDictionary<T> should call TryAdd rather than Add to avoid needing unnecessary exceptions. – Brian Oct 28 '14 at 0:01
  • Of course, implementing Add explicitly also reduces clutter. TryAdd can do anything Add can do (Add is implemented as a call to TryAdd, so providing both would be redundant). However, TryAdd necessarily has a different name/signature, so it is not possible to implement IDictionary without this redundancy. Explicit implementation resolves this problem cleanly. – Brian Oct 28 '14 at 0:10
  • Well from a consumer point of view the methods are hidden. – Esben Skov Pedersen Oct 28 '14 at 7:12
  • 1
    You write about IReadonlyCollection<T> but link to ReadOnlyCollection<T>. The first in an interface, the second is a class. IReadonlyCollection<T> does not implement IList<T> even though ReadonlyCollection<T> does. – Jørgen Fogh Oct 28 '14 at 15:25
2

It's good form to do so when the member is pointless in context. For example, if you create a readonly collection that implements IList<T> by delegating to an internal object _wrapped then you might have something like:

public T this[int index]
{
  get
  {
     return _wrapped[index];
  }
}
T IList<T>.this[int index]
{
  get
  {
    return this[index];
  }
  set
  {
    throw new NotSupportedException("Collection is read-only.");
  }
}
public int Count
{
  get { return _wrapped.Count; }
}
bool ICollection<T>.IsReadOnly
{
  get
  {
    return true;
  }
}

Here we've got four different cases.

public T this[int index] is defined by our class rather than the interface, and hence is of course not an explicit implementation, though note that it does happen to be similar to the read-write T this[int index] defined in the interface but is read-only.

T IList<T>.this[int index] is explicit because one part of it (the getter) is perfectly matched by the property above, and the other part will always throw an exception. While vital to someone accessing an instance of this class through the interface, it is pointless to someone using it through a variable of the class's type.

Similarly because bool ICollection<T>.IsReadOnly is always going to return true it is utterly pointless to code that is written against the class's type, but could be vital to that using it through the interface's type, and therefore we implement it explicitly.

Conversely, public int Count is not implemented explicitly because it could potentially be of use to someone using an instance through its own type.

But with your "very rarely used" case, I would lean very strongly indeed toward not using an explicit implementation.

In the cases where I do recommend using an explicit implementation calling the method through a variable of the class's type would either be an error (attempting to use the indexed setter) or pointless (checking a value that will always be the same) so in hiding them you are protecting the user from buggy or sub-optimal code. That's significantly different to code you think is just likely to be rarely used. For that I might consider using the EditorBrowsable attribute to hide the member from intellisense, though even that I would be weary of; people's brains already have their own software for filtering out what doesn't interest them.

  • If you're implementing an interface, implement the interface. Otherwise, this is a clear violation of the Liskov Substitution Principle. – Telastyn Oct 28 '14 at 13:49
  • @Telastyn the interface is indeed implemented. – Jon Hanna Oct 28 '14 at 13:59
  • But the contract of it isn't satisfied - specifically, "This is something that represents a mutable, random access collection". – Telastyn Oct 28 '14 at 14:12
  • 1
    @Telastyn it fulfils the documented contract, particularly in light of "A collection that is read-only does not allow the addition, removal, or modification of elements after the collection is created." See msdn.microsoft.com/en-us/library/0cfatk9t%28v=vs.110%29.aspx – Jon Hanna Oct 28 '14 at 14:26
  • @Telastyn: If the contract included mutability, then why would the interface contain a IsReadOnly property? – nikie Oct 28 '14 at 17:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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