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.

Kyle Baran
  • 436
  • 7
  • 14

4 Answers4

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.

Telastyn
  • 108,850
  • 29
  • 239
  • 365
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)]
codenheim
  • 2,963
  • 16
  • 18
  • "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.

Community
  • 1
Esben Skov Pedersen
  • 5,098
  • 2
  • 21
  • 24
  • 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`](http://msdn.microsoft.com/en-us/library/dd287191.aspx), which implements various members of `IDictionary` explicitly so that consumers of `ConcurrentDictionary` A) won't call them directly and B) can use methods which require an implementation of `IDictionary` if absolutely necessary. E.g., users of `ConcurrentDictionary` *should* call `TryAdd` rather than `Add` to avoid needing unnecessary exceptions. – Brian Oct 28 '14 at 00: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 00:10
  • Well from a consumer point of view the methods are hidden. – Esben Skov Pedersen Oct 28 '14 at 07:12
  • Why would you assume that the methods would even be implemented at all? If a class implements IReadOnlyList it doesn't have to implement IList too. This has nothing to do with explicit interface implementation, which hides methods that *must* be present. – Jørgen Fogh Oct 28 '14 at 10:38
  • @JørgenFogh see my edit – Esben Skov Pedersen Oct 28 '14 at 14:33
  • 1
    You write about IReadonlyCollection but link to ReadOnlyCollection. The first in an interface, the second is a class. IReadonlyCollection does not implement IList even though ReadonlyCollection does. – Jørgen Fogh Oct 28 '14 at 15:25
  • Sorry for the confusion. It should be ok now. – Esben Skov Pedersen Oct 28 '14 at 19:50
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.

Jon Hanna
  • 2,115
  • 12
  • 15
  • 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 http://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
  • One could certainly argue that a better approach would have been to first define an interface for read-only-appropriate members, then another for those that mutate, and .NET4.5 does have something closer to that, but there are arguments for the more complete interface of `IList` being the one used in both cases too. Whatever way one wants to argue that design decision, the fact is it was done and that is the design decision one has to then work with when implementing. – Jon Hanna Oct 28 '14 at 17:52
  • @nikie: The collection interfaces are poorly designed but the idea of having members that may or may not be supported is perfectly sound. For example, if an object which receives a reference to an object from one piece of outside code, is supposed to expose some of its abilities to another piece of outside code, it should be usable with any object which can support all the methods the outside code requires. Defining interfaces to precisely fit every combination of `IList` features that particular implementations would or would not support would probably take over a dozen; a better idea... – supercat Oct 28 '14 at 22:49
  • @nikie: ...would be to have a base collection interface include a property that would return a reference to a `CollectionAbilities` object that would indicate which features were or were not supported [allowing much more detail than the poorly-named `ICollection.IsReadOnly`], while allowing a typical implementation to simply define and return a singleton. – supercat Oct 28 '14 at 22:52