0

Let say I have a class like this:

public class A
{
    private BaseSettings fieldA;
    public ISettings PropertyA
    {
         get {return fieldA;}
         set {fieldA= value as BaseSettings;}
    }
}

where BaseSettings implements ISettings. Inside class A, if I want to access BaseSettings' property called PropertyB, which of this is a good practice: 

fieldA.PropertyB;

or

((BaseSettings)PropertyA).PropertyB;

One may say the first approach may hide the hint to when a property changed. For example, a code may listen to PropertyChangedEvent and then the value for property changed without raising the event.

Another one may say the second approach may expose a risk that when a person who is not familiar with current code modify it, he may cast the PropertyA to different type that implements ISettings.

Both approaches have its downside. In a good programming practice, which one should be more preferable?

EDIT: Added based on the comments belows:

I agree that setting the backing-field as ISettings makes absolute sense. But what should I do to make sure that the PropertyA is always type of BaseSettings. That will raise a question: "Then why don't you set both property and backing-field to BaseSettings?".

The reason behind why property and its backing field are different is that class A also implement an interface like this

public interface IControlWithSettings
{
     ISettings OwnerSettings
     {
         get;
         set;
     }
     ISettings Settings
     {
         get;
         set;
     }
}

So the actual classA would look like this

public class BaseForm: Form, IControlWithSettings
{
    private BaseFormSettings settings;
    public ISettings Settings
    {
         get {return settings;}
         set {settings= value as BaseFormSettings;}
    }
}

And I have another class B would also implement IControlWithSettings

public class BaseUserControl: UserControl, IControlWithSettings
{
    private BaseUserControlSettings settings;
    public ISettings Settings
    {
         get {return settings;}
         set {settings= value as BaseUserControlSettings ;}
    }
}

Both BaseFormSettings : ISettings and BaseUserControlSettings : ISettings . This is actual ISettings interface

public interface ISettings
{
    Dictionary<string, ISettings> Children { get; set; }
}

the 'as' casting is a side effect I put into the setter so that it will ignore and return null if the setting is set to wrong one. I read somewhere saying I shouldn't throw exception in a setter. So making it null is my way to inform there is something wrong has been done.

So what is the better approach. Did I design it wrong?

LxL
  • 1,878
  • 2
  • 20
  • 39
  • 6
    Why are you using a specific type to back the property but a generic type to expose the property? This will be problematic at best for the consumers of your class. – Justin Niessner Jan 09 '14 at 20:11
  • Agreed with @JustinNiessner. You could go the other way (interface for the private field, specific type for the public property) but doing it this way will cause problems. – Adam V Jan 09 '14 at 20:13
  • Optimally you should be using ISettings for both the backing field and the property declaration. If class A needs to use PropertyB then it should be exposed by the Interface – BlackICE Jan 09 '14 at 20:16
  • If I try to set `PropertyA` to an instance of `class OtherSettings : ISettings`, it will end up `null`. This breaks the assumption that after I set a property to a value, it will have that value. Maybe you should be throwing an exception in that case, e.g. by casting instead of using `as`? Or, as others have suggested, make the property and field types matched (if possible) – Tim S. Jan 09 '14 at 20:18
  • 2
    possible duplicate of [Use of properties vs backing-field inside owner class](http://stackoverflow.com/questions/2457010/use-of-properties-vs-backing-field-inside-owner-class) – Eric Lippert Jan 09 '14 at 20:22
  • 2
    See also my article on the subject. http://blogs.msdn.com/b/ericlippert/archive/2009/01/14/automatic-vs-explicit-properties.aspx – Eric Lippert Jan 09 '14 at 20:23
  • @EricLippert Please view my edited question. – LxL Jan 09 '14 at 21:23
  • @JustinNiessner Please view my edited question – LxL Jan 09 '14 at 21:23
  • Your property setter is now a lie. It claims to accept any `ISettings` but in fact accepts only a particular type. **You are not implementing the interface**, which requires that the setter accept *any* `ISettings`. You ask "*what should I do to make sure that the PropertyA is always type of BaseSettings?*" and the answer is to unask the question. You shouldn't do that! The interface requires that the implementer be able to handle anything thrown at it of the given type. – Eric Lippert Jan 09 '14 at 22:59
  • I would fix this by removing all the setters from all your interfaces. The less change you allow, the better. If a `BaseUserControl` as an implementation detail requires that its settings be `BaseUserControlSettings` then don't make the *consumer* of the class have to know that. **Never give the user the opportunity to do it wrong in the first place**. Any time you let a user do it wrong, they'll do it wrong. Rather, make the class create its own settings object and let the user mutate *the settings*, but not *the reference to the settings object*. – Eric Lippert Jan 09 '14 at 23:06
  • Of course all this assumes that you need this polymorphism in the first place, which is by no means clear to me. Polymorphism is a way to manage generality, and generality is **expensive**. Premature generality gives premature optimization a run for the title of "root of all evil" as far as I'm concerned. It looks like you are building a highly general control hierarchy. My advice: don't. Re-use an existing control hierarchy, or build one tailored to your specific needs with as little generality as possible. – Eric Lippert Jan 09 '14 at 23:07
  • It would be a good question if it had not so much description. – SerG Feb 11 '14 at 14:00

1 Answers1

1

As you stated, both approaches have their downsides, and it also depends on whether the property setter may contain some additional logic (e.g. validation), and you may use or circumvent this additional logic from inside the class.
If there's nothing that speaks against it, I'll use direct access to the field. It's neater and it avoids all this typecasting stuff.

But generally: Why would you back your property with the derived type, while the property itself has the interface type? This doesn't make much sense. Why not just:

public class A
{
    public ISettings PropertyA { get; set; }
}

This would be much cleaner, and your question wouldn't even arise.

Edit (based on the answer's edit)
In case of the 'double use' of the backing field the typecasting makes sense. But I don't think (and never heard that before) that it's a bad thing to throw an exception from a property setter. On the contrary: Validating a value and throwing an exception if it doesn't pass is a very common pattern.
So, in your concrete case, I would validate the value for the correct type, throw if it's not correct, and use the backing field internally to bypass this type check.

Thomas Weller
  • 11,631
  • 3
  • 26
  • 34