1

This question already has an answer here:

Introductory piece of code:

class BaseClass
{
    protected Foo MyFoo { get; }
}

class ChildClass : BaseClass
{
    void SomeMethod()
    {
        MyFoo.DoStuff();
        //Here, I have no idea that MyFoo is not defined in this class,
        //but rather in the base class.
    }
}

The problem/debate is that you have to either know or check manually where MyFoo is defined. And you're better off using two private fields, one in the base class, on in the child class.

In our case, most of the time, these properties come from dependency injection and are available in the constructor of both classes. So it's really easy to just map them to private properties from that constructor, instead of inheriting it from the base class, which is also less clear.

Is there good practice that I'm missing? What is your experience there?

Underlying question: Are protected properties just a bad habit, and should I always go for private fields instead?

Will this go against me when unit testing?

Is this considered clean/unclean code?

marked as duplicate by gnat, Doc Brown, Pieter B, Robert Harvey, Greg Burghardt Nov 14 '17 at 0:51

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 1
    You are not telling us what you wish to accomplish and why you believe protected is in your way. – frostings Nov 13 '17 at 14:53
  • 1
    "In our case, most of the time, these properties come from dependency injection, and are available in the constructor of both classes" - it seems you have a very special case of protected properties in mind, and your example does not reflect this. Voting to close as "too broad" or "unclear". – Doc Brown Nov 13 '17 at 15:32
  • 10
    Here, I have no idea that MyFoo is not defined in this class, but rather in the base class. -- That's true with any inherited method or property. – Robert Harvey Nov 13 '17 at 15:37
  • 7
    There is no good or evil. There is only power, and those too weak to wield it. – user22815 Nov 13 '17 at 17:29
  • 1
    @Snowman: If you are too weak to give yourselves your own law, then a tyrant shall lay his yoke upon you and say: "Obey! Clench your teeth and obey!" And all good and evil shall be drowned in obedience to him. – Eric Lippert Nov 13 '17 at 21:28
22

I don't think duplicating the property in the subclass (and its constructor) is really a better alternative. For actual read/write properties, that can't even yield the same functionality, and for read-only injected properties as in your case (some kind of service probably) it does not matter that much where it's defined. Besides, your IDE can easily show you that.

Basically, the way to look at it is this:

  • Protected fields are part of the class API for subclassing, just like methods.
  • Protected fields that actually contain mutable data can definitely result in code that is hard to understand, but so can contain protected methods when they are overridden in subclasses but called in the superclass.
  • The problem here is not protected fields or methods, the problem is subclassing. It should be used carefully and sparingly.
  • A rule of thumb I just made up on the spot: either the superclass should be very small and simple, or the subclass. And deep inheritance hierarchies are bad.
12

you have to either know or check manually where MyFoo is defined

But how is that specific to protected properties? The same is true for methods and really any public member. One big reason for inheritance is to avoid having to repeat the same thing for every concrete type. So you can have a common base type that contains base behavior and base storage definitions.

A property or field being protected just tells you that this is an internal implementation detail which is likely to be relevant for subclasses. In a way, this actually tells you more than having completely unconnected private properties on each level.

And the compiler already knows where it is defined and how its accessible. And good editors will tell you that at design time, so there is really no need to “check manually”. In addition, when inheriting a type, I would expect you to be comfortable with its interface, so you know that there are protected members you can use.

In our case these properties come from dependency injection

This might be just my opinion, but I would always use readonly fields for dependencies, not properties.

So it's really easy to just map them to private properties from that constructor

Sure, you could have a private field on each level of your type hierarchy. But that will also duplicate the memory usage you need to keep track of those. And if you for a moment ignore the fact that these are unchanging dependencies injected into the constructor, a base class not being able to change the value for its subclasses might also introduce problems.

Are protected properties just a bad habit, and should I always go for private fields instead ?

There are two questions here: Property vs. field, and protected vs private. To answer the latter: Use what makes sense in an encapsulation sense. If your base type has actual internal APIs that subclasses could use, those should be protected. But you shouldn’t just make any dependency protected just in case.

  • Much agree. What happens when the private members in the two classes have a (slightly) different name? Now, you have to look in 3 or 4 places to verify that they refer to the same thing. – jpaugh Nov 13 '17 at 17:50
6

Are protected properties evil?

No. Protected properties are smelly, but not evil. That is, they should make you think "am I modeling this correctly?" One thinks of a property as being part of the public surface area of a model, and a field as being a private implementation detail of a model. Since "protected" simply means "a private implementation detail of this hierarchy", one expects protected fields rather than protected properties.

That said, protected properties are not wrong; they're just a sign that perhaps you need to think about your design a little harder.

Here, I have no idea that MyFoo is not defined in this class, but rather in the base class.

Of course you know that. You can read the source code of the class and plainly see that there is no such property. And you can "go to definition" and go to the source code for the property.

The problem/debate is that you have to either know or check manually where MyFoo is defined.

No, you don't. Why do you think you have to know where it is defined?

And you're better off using 2 private fields, one in the base class, on in the child class.

No, I don't agree with that at all. You'd possibly be better off using a protected field in the base class.

Are protected properties just a bad habit, and should I always go for private fields instead ?

If you find yourself writing a lot of protected properties, I would ask yourself if they are actually better modeled as protected fields. Remember, a property should model a property of the modeled thing. If you're writing a Car class then its properties should be the properties of a car. What things are both properties of a car and also the implementation details of the hierarchy of cars? I can't think of anything that is both those things, and so maybe there ought not to be protected properties.

  • 1
    "Since "protected" simply means "a private implementation detail of this hierarchy", one expected protected fields rather than protected properties." not sure I can follow. Certainly if I have the base class and all implementations under control a protected property is pointless. But if I provide a base class that can be implemented by 3rd parties, using properties instead of fields makes it easier to change the implementation in the future - same reason we use public properties instead of fields. Why would that be a code smell? – Voo Nov 13 '17 at 23:06
  • 1
    @Voo: I see your point. There are three things in tension here: (1) provide an implementation detail to a class hierarchy, (2) keep the details of that thing as an abstraction itself, and (3) make properties only of things that are logically properties. All design decisions involve making tradeoffs between incompatible goals; my point is simply that if you're in such a situation, that you take a little extra time and effort to ensure that the design really is the best compromise. – Eric Lippert Nov 14 '17 at 1:21

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