FxCop - CA1034 error - WHY?

Question

I am running static code analysis with FxCop 1.36 and I keep getting warning CA1034: NestedTypesShouldNotBeVisible.

I would understand if the parent class were declared as internal or private, but it is public. Why would it be bad for TimerReset to be declared public?

Am I missing something, or is this something that can be ignored?

Thanks for any input!

Here is an excerpt of the code causing this warning:

namespace Company.App.Thing
{
    public partial class Page : XtraPage
    {
        public delegate void TimerResetDelegate(object sender, EventArgs e);
        private TimerResetDelegate _timerReset;

        public Page()
        {
            InitializeComponent();
        }

        public TimerResetDelegate TimerReset
        {
            set
            {
                if (null != (_timerReset = value))
                {
                    checkBox.Click += new EventHandler(_timerReset);
                    textField.Click += new EventHandler(_timerReset);
                    textField.KeyDown += new KeyEventHandler(_timerReset);
                    TimeField.Click += new EventHandler(_timerReset);
                    TimeField.KeyDown += new KeyEventHandler(_timerReset);
                }
            }
        }
    }
}

Answer 1

It is because your delegate is a type, but it is defined within the Page class. I would just define it in the Company.App.Thing namespace, but it is not a problem really. If you were writing an API it would just make it a bit messy, that's all.

Also, it is a bit odd to return the delegate like that, but I guess I don't really know what you are trying to accomplish.

Answer 2

IMHO, this is an FxCop rule that can be ignored.

There is nothing wrong from a CLR level with having a nested class. It is just a guideline rule added to FxCop because the authors felt it was less usable or a poorer design than making the class non-nested.

Answer 3

Apparently it doesn't like the idea of nested classes, when they might be used outside the context of your Page class.

I personally agree with that in principle, though I can imagine some exceptions where it might be desirable.

Answer 4

Generally speaking, Nested Types are harder to 'discover'.

E.g. To use your nested type, I will have to write following

Page.TimerResetDelegate timer = new Page.TimerResetDelegate();

Even though above is valid C# code, it doesn't read like the usual type usage.

Nested Types are generally used when you want to define a type to be used internally and you would avoid code like above. This is the reason why FxCop is giving you warning. If you want, you can ignore it. Personally, I would keep my Nested types as private. If I expect caller to make use of the type, I will move them to a proper namespace.

Answer 5

Why would it be bad for TimerReset to be declared public?

Exactly as the description states:

Nested types are useful for encapsulating private implementation details of the containing type. Used for this purpose, nested types should not be externally visible.

Since you're exposing TimerResetDelegate publically with TimerReset, I guess it's not an implementation detail.

Do not use externally visible nested types for logical grouping or to avoid name collisions; instead, use namespaces.

Which makes it look like you're using a nested type for grouping. As FxCop states, use a namespace instead.

Nested types include the notion of member accessibility, which some programmers do not understand clearly.

Since TimerResetDelegate is a delegate, this doesn't really apply.

Move TimerResetDelegate to it's own TimeResetDelegate.cs file, and put it in your Company.App.Thing namespace. Then, it's no longer nested.

Of course, it'd be even better to just go with EventHandler instead of defining your own delegate type.