Design TODOs
Jesse Jones
AvoidSmallNamespaceRule, DeclareEventHandlersCorrectlyRule,
DontDeclareProtectedFieldsInSealedClassRule,
AttributeArgumentsShouldHaveAccessorsRule, and
AvoidMultidimensionalIndexerRule.
-- Jesse
Sebastien Pouliot
>
> Index: rules/Gendarme.Rules.Design/AvoidSmallNamespaceRule.cs
> ===================================================================
> --- rules/Gendarme.Rules.Design/AvoidSmallNamespaceRule.cs (revision 137626)
> +++ rules/Gendarme.Rules.Design/AvoidSmallNamespaceRule.cs (working copy)
> @@ -38,13 +38,11 @@
>
> namespace Gendarme.Rules.Design {
>
> - // TODO: If five is the default then how do users change it? If they cannot change it
> - // why are we implying that they can?
> -
> /// <summary>
> /// This rule fires if a namespace contains less than five (default) visible types. Note
> /// that this rule enumerates the types in all the assemblies being analyzed instead of
> - /// simply considering each assembly in turn.
> + /// simply considering each assembly in turn. The default can be changed using the
> + /// --config switch and the 'Minimum' property.
Not a good idea. There are many "default" properties and they need to
common way to be documented. More importantly is that giving the console
runner syntax is not a future proof idea (since half our users are using
the wizard). But, like discussed on IRC, we do need to document this
feature appropriately.
note: when we switched to xmldoc the idea was (well still is, in the
future tense) to use xmldoc comments on the configurable properties[1]
so they will be included in the rule documentation [2].
[1] the use of S.ComponentModel attributes is also in the plan to let a
(future) GUI runner automagically build the UI for configuring any rule.
[2] We could start documenting properties after 2.6 - but not before
since my xmldoc2wiki tool is a bit too basic to handle this right now.
> /// The rule exempts:
> /// <list>
> /// <item><term>specialized namespaces</term><description>e.g. <c>*.Design</c>,
> Index: rules/Gendarme.Rules.Design/DeclareEventHandlersCorrectlyRule.cs
> ===================================================================
> --- rules/Gendarme.Rules.Design/DeclareEventHandlersCorrectlyRule.cs (revision 137626)
> +++ rules/Gendarme.Rules.Design/DeclareEventHandlersCorrectlyRule.cs (working copy)
> @@ -35,14 +35,18 @@
>
> namespace Gendarme.Rules.Design {
>
> - // TODO: This rule needs to say why the signature needs to follow the .NET conventions.
> - // For example, a non-void return type is problematic because some of the return values
> - // may be thrown away if multiple delegates are attached to the event. And a missing
> - // or unconventional second argument may cause problems with visual designer tools.
> - // It may also make sense to use a higher severity for non-void return types.
> + // TODO: It may make sense to use a higher severity for non-void return types.
Please add any new/updated TODO in the group document.
>
> /// <summary>
> - /// This rule verifies that all events declared within a type have correct signatures.
> + /// This rule will fire if an event is declared with a signature which does not match
> + /// the .NET guidelines. The return type of the event should be void (because there
> + /// is no good way to handle return values if multiple delegates are attached to the
> + /// event). And the event should take two arguments. The first should be of type
> + /// <c>System.Object</c> and be named 'sender'. The second should be of type
> + /// <c>System.EventArgs</c> (or a subclass) and named 'e'. This helps tools
> + /// such as visual designers identify the delegates and methods which may be
> + /// attached to events. Note that .NET 2.0 added a generic <c>System.EventHandler</c>
> + /// type which can be used to easily create events with the correct signature.
> /// </summary>
> /// <example>
> /// Bad example:
> Index: rules/Gendarme.Rules.Design/DontDeclareProtectedFieldsInSealedClassRule.cs
> ===================================================================
> --- rules/Gendarme.Rules.Design/DontDeclareProtectedFieldsInSealedClassRule.cs (revision 137626)
> +++ rules/Gendarme.Rules.Design/DontDeclareProtectedFieldsInSealedClassRule.cs (working copy)
> @@ -35,13 +35,10 @@
>
> namespace Gendarme.Rules.Design {
>
> - // TODO: why does this rule even bring up public? It seems like new protected members
> - // of sealed types should always be made private.
> -
Yes, very likely. Protected is a mistake so it's a bit hard to know how
to came about (e.g. refactoring). However you're right that the rule
should limit itself to say that 'protected == private in such cases, so
be clear about it'.
> /// <summary>
> /// This rule ensures that <c>sealed</c> types (i.e. types that you can't inherit from)
> /// do not define family (<c>protected</c> in C#) fields or methods. Instead make the
> - /// member private (or possibly public) so that its accessibility is not misleading.
> + /// member private so that its accessibility is not misleading.
> /// </summary>
> /// <example>
> /// Bad example (field):
> @@ -74,7 +71,7 @@
> /// Good example (method):
> /// <code>
> /// public sealed class MyClass {
> - /// public virtual int GetAnswer ()
> + /// private virtual int GetAnswer ()
oh and not much point in being virtual too :)
> /// {
> /// return 42;
> /// }
> @@ -84,7 +81,7 @@
> /// <remarks>Prior to Gendarme 2.2 this rule applied only to fields and was named DoNotDeclareProtectedFieldsInSealedClassRule</remarks>
>
> [Problem ("This sealed type contains family (protected in C#) fields and/or methods.")]
> - [Solution ("Change the access to private or public to represent its true intended use.")]
> + [Solution ("Change the access to private to represent its true intended use.")]
hmm... maybe "Change the protected access to represent its true
intended use." ? or drop the "intended use" (e.g. current use) since the
rule does not know anything about the intention behind the original
protected code ?
> [FxCopCompatibility ("Microsoft.Design", "CA1047:DoNotDeclareProtectedMembersInSealedTypes")]
> public class DoNotDeclareProtectedMembersInSealedTypeRule: Rule, ITypeRule {
>
> Index: rules/Gendarme.Rules.Design/AttributeArgumentsShouldHaveAccessorsRule.cs
> ===================================================================
> --- rules/Gendarme.Rules.Design/AttributeArgumentsShouldHaveAccessorsRule.cs (revision 137626)
> +++ rules/Gendarme.Rules.Design/AttributeArgumentsShouldHaveAccessorsRule.cs (working copy)
> @@ -34,11 +34,15 @@
>
> namespace Gendarme.Rules.Design {
>
> - // TODO: This rule should explain why this is a problem.
> + // TODO: This rule seems to mix two problems: 1) attribute state is generally not
> + // useful if it is not exposed 2) the naming conventions want properties to be PascalCase.
> + // It would seem better if this rule addressed the first problem and allowed the naming
> + // rules to address the second.
agreed, but let's move this into the group doc.
> /// <summary>
> /// This rule fires if a parameter to an <c>Attribute</c> constructor is not exposed
> - /// using a properly cased property.
> + /// using a properly cased property. This is a problem because it is generally not useful
> + /// to set state within an attribute without providing a way to get at that state.
> /// </summary>
> /// <example>
> /// Bad example:
> Index: rules/Gendarme.Rules.Design/AvoidMultidimensionalIndexerRule.cs
> ===================================================================
> --- rules/Gendarme.Rules.Design/AvoidMultidimensionalIndexerRule.cs (revision 137626)
> +++ rules/Gendarme.Rules.Design/AvoidMultidimensionalIndexerRule.cs (working copy)
> @@ -35,16 +35,11 @@
>
> namespace Gendarme.Rules.Design {
>
> - // TODO: It's not at all clear why a method is better than an indexer here. Is it because
> - // a multiple dimensional indexer may not represent a "logical data store" as the FxCop
> - // rule implies? Or do IDEs provide better auto-complete for methods so that the index
> - // order problem is more tractable if the indexer is a method?
> -
> /// <summary>
> - /// This rule checks for indexer properties in externally visible types which have more
> - /// than one index argument.
> - /// Such indexers are often confusing to use (e.g. it's not always clear to users how
> - /// the actual arguments should be ordered) and are better represented by methods.
> + /// This rule checks for externally visible indexer properties which have more
> + /// than one index argument. These can be confusing to some developers and
> + /// IDEs with auto-complete don't always handle them as well as methods so
> + /// it can be hard to know which argument is which.
> /// </summary>
> /// <example>
> /// Bad example:
> @@ -68,7 +63,7 @@
> /// <remarks>This rule is available since Gendarme 2.0</remarks>
>
> [Problem ("This indexer use multiple indexes which can impair its usability.")]
> - [Solution ("Convert this indexer into a method.")]
> + [Solution ("Consider converting the indexer into a method.")]
> [FxCopCompatibility ("Microsoft.Design", "CA1023:IndexersShouldNotBeMultidimensional")]
> public class AvoidMultidimensionalIndexerRule : Rule, IMethodRule {
>
Thanks again!
Sebastien