0

I get this warning from stylecop always. this makes sense from class perspective. fields should be private and use property to expose the fields

but i have a codebehind where i have declared a control as below. and this warning doesn't make sense.

     /// <summary>
    /// Table used to generate the UI
    /// </summary>
    protected Table HighlightTable;

i don't want to make my controls has private or property and expose it. it has to protected.

please can some clarify this.

neha
  • 1
  • 1
  • 1

2 Answers2

1

It does make sense, you do not want to expose the internals of the type to other child types.

protected field means it is accessible by the inheritors, thus you break encapsulation - type exposes internal state. You can fix this by converting your field to a property with restricted visibility

protected Table HighlightTable {get; set;}

or by restricting visibility of the field

private Table highlightTable;

See a similar question here for other opinions.

Community
  • 1
  • 1
oleksii
  • 35,458
  • 16
  • 93
  • 163
  • You can have alternative .aspx files that can use the same base class, so these control fields need to be protected, not private, so the other .aspxs can see them. – Netricity May 18 '13 at 12:00
  • 1
    I am not agree on that. Protected is ok. Exposing in field to child is not that a big deal. Adding property just to fix the warning is kind of silly thing to do. The child class may want to access to all his field. Even the inherited one. You break reusability by making all field private. Invariant can be maintain in the child class and if you think that is not possible most probably you have too complicate invariant that need to be split. – mathk Nov 11 '13 at 11:01
  • @mathk it's just a guideline that people adopted. It is easier to maintain code that adheres to some guidelines. The guidelines in stylecop suggest to use either a property or private field. You can disagree with this and use protected fields. And the code will still work. The problem is that now you brake these guidelines. Not a big deal for many, but some do care. An auto property doesn't add much to the code, but the intent of usage is much cleaner. – oleksii Nov 11 '13 at 12:47
0

I think StyleCop is right... that field should be private.

Why did you mark HighlightTable as protected? Are you doing something with it in a derived class?

If you aren't doing anything with it in a dervived class, it should just be private.

If you are using it in a derived class, as SyleCop says, you should create a property and mark that as protected, and your field should just be private.

Kaushal De Silva
  • 592
  • 3
  • 8
  • *Why did you mark HighlightTable as protected?* - the OP didn't, Visual Studio does that automatically when you declare a control with `runat="server"` in the .aspx markup. – Netricity May 18 '13 at 11:56
  • Ah yes, its been a while since I have dealt with WebForms... so @Netricity would you say that VS is doing the wrong thing here or is Stylecop being too pedantic? I noticed in your other comment you mention multiple ASPX files, so maybe Stylecop is wrong? – Kaushal De Silva May 18 '13 at 12:08
  • 1
    VS could improve by creating auto-implemented protected properties, rather than fields. In older versions of C# auto-implemented properties weren't available, so a lot of code would have been generated, so maybe that's why protected fields were used. On the other hand, I think Stylecop should take this behaviour into account. Can Stylecop be configured to ignore such fields in .aspx.designer.cs files? – Netricity May 18 '13 at 13:17