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.
-
3If 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– Andy2014年10月27日 23:27:27 +00:00Commented Oct 27, 2014 at 23:27
-
1I worked with a guy who did this on a regular basis. It drove me absolutely nuts.Joel Etherton– Joel Etherton2014年10月28日 19:54:59 +00:00Commented Oct 28, 2014 at 19:54
-
... and start using aggregation.mikalai– mikalai2014年10月31日 19:46:46 +00:00Commented Oct 31, 2014 at 19:46
4 Answers 4
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– Gusdor2014年10月28日 12:24:29 +00:00Commented Oct 28, 2014 at 12:24
-
11"Fix your problem, not your symptom." +1FreeAsInBeer– FreeAsInBeer2014年10月28日 12:49:34 +00:00Commented Oct 28, 2014 at 12:49
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– jhominal2014年10月28日 13:47:14 +00:00Commented Oct 28, 2014 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.mrjoltcola– mrjoltcola2014年10月28日 14:43:25 +00:00Commented Oct 28, 2014 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– jhominal2014年10月28日 15:32:12 +00:00Commented Oct 28, 2014 at 15:32
-
@jhominal - Sure thing, when I have a few minutes later I will update. Thanks.mrjoltcola– mrjoltcola2014年10月28日 17:28:37 +00:00Commented Oct 28, 2014 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 likeClose
, I would consider it perfectly reasonable for a class whose contract expressly disclaims states that code may create and abandon instances without callingDispose
to use explicit interface implementation to define anIDisposable.Dispose
method that does nothing.supercat– supercat2014年10月28日 22:36:17 +00:00Commented Oct 28, 2014 at 22:36
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– Kyle Baran2014年10月27日 21:04:08 +00:00Commented Oct 27, 2014 at 21:04
-
2I 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 ofIDictionary<T>
explicitly so that consumers ofConcurrentDictionary
A) won't call them directly and B) can use methods which require an implementation ofIDictionary<T>
if absolutely necessary. E.g., users ofConcurrentDictionary<T>
should callTryAdd
rather thanAdd
to avoid needing unnecessary exceptions.Brian– Brian2014年10月28日 00:01:56 +00:00Commented Oct 28, 2014 at 0:01 -
Of course, implementing
Add
explicitly also reduces clutter.TryAdd
can do anythingAdd
can do (Add
is implemented as a call toTryAdd
, so providing both would be redundant). However,TryAdd
necessarily has a different name/signature, so it is not possible to implementIDictionary
without this redundancy. Explicit implementation resolves this problem cleanly.Brian– Brian2014年10月28日 00:10:38 +00:00Commented Oct 28, 2014 at 0:10 -
Well from a consumer point of view the methods are hidden.Esben Skov Pedersen– Esben Skov Pedersen2014年10月28日 07:12:37 +00:00Commented Oct 28, 2014 at 7:12
-
1You 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– Jørgen Fogh2014年10月28日 15:25:16 +00:00Commented Oct 28, 2014 at 15:25
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– Telastyn2014年10月28日 13:49:09 +00:00Commented Oct 28, 2014 at 13:49
-
@Telastyn the interface is indeed implemented.Jon Hanna– Jon Hanna2014年10月28日 13:59:12 +00:00Commented Oct 28, 2014 at 13:59
-
But the contract of it isn't satisfied - specifically, "This is something that represents a mutable, random access collection".Telastyn– Telastyn2014年10月28日 14:12:28 +00:00Commented Oct 28, 2014 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.aspxJon Hanna– Jon Hanna2014年10月28日 14:26:25 +00:00Commented Oct 28, 2014 at 14:26
-
@Telastyn: If the contract included mutability, then why would the interface contain a
IsReadOnly
property?nikie– nikie2014年10月28日 17:36:51 +00:00Commented Oct 28, 2014 at 17:36