RFC - changing I[Assembly|Module|Type|Method||]Rule interfaces
Sebastien Pouliot
Last month at the Mono Summit we (Nestor, JB an me) had some talks
(and lot of hacking :-) on Gendarme. There are several points we're
planning on changing over the next releases. Some of them are quite
easy or without much impact (i.e. we can do them gradually), while
others will require more work. Here's one of the later case...
The current I[Assembly|Module|Type|Method||]Rule interfaces were
"updated" long ago to solve some problems. The original problems were
solved, however it turns out we're not quite happy because this
introduced other problems:
- we use null (or a RuleSuccess null constant) for success. However we
can't distinguish between a rule "does not apply" or rule "checked
with success";
- rules end up creating a lot of temporary objects (mostly
MessageCollection) which, even when used, aren't really needed since
the rule has access to the runner instance;
Since this one has a big impact on the rule's source code (and even
more on the unit tests) I'd like to solicit your comments before
starting any work on this
The original code [1] would be replaced with something like this...
// we can keep statistics on rule's applicability and success/failure
ratio using this enum
public enum RuleResult {
DoesNotApply,
Success,
Failure
}
public interface IRule {
// to be supplied with the rule constructor
Runner Runner { get; }
}
// ??? should we renamed all methods to Check ???
public interface IAssemblyRule : IRule {
RuleResult CheckAssembly (AssemblyDefinition assembly);
}
public interface IModuleRule : IRule {
RuleResult CheckModule (ModuleDefinition module);
}
public interface ITypeRule : IRule {
RuleResult CheckType (TypeDefinition type);
}
public interface IMethodRule : IRule {
RuleResult CheckMethod (MethodDefinition method);
}
// and we provide new base class (for each case?) to implement the
basic stuff
public abstract class Rule : IRule {
private Runner runner;
protected Rule (Runner runner)
{
this.runner = runner;
}
public Runner Runner {
get { return runner; }
}
}
[1] http://anonsvn.mono-project.com/viewcvs/trunk/cecil/gendarme/framework/IRule.cs?view=markup
Feedback welcome :)
Have fun!
Sebastien
Néstor Salceda
> Hello everyone,
Hello, I put some comments inlined.
> // we can keep statistics on rule's applicability and success/failure
> ratio using this enum
> public enum RuleResult {
> DoesNotApply,
> Success,
> Failure
> }
I like this enum, because the in the rules we have used the null value,
and with this enum, we can improve the readability of the new rules.
The second reason, is because if we need, we can add new return states
if needed.
> public interface IRule {
> // to be supplied with the rule constructor
> Runner Runner { get; }
> }
I like the new interface too, with the Runner property. I feel strange
with empty interfaces but sometimes there are several reasons for
maintain it; it's a well known problem.
http://www.gotdotnet.com/Team/FxCop/Docs/Rules/Design/AvoidEmptyInterfaces.html
Now, with this change the interface isn't empty and I feel better :)
> // ??? should we renamed all methods to Check ???
I'm not sure about this, because with this signatures; the framework
puts special emphasis in the stuff that will be checked. Although, it's
probabily redundant, by example:
public AvoidEmptyInterfacesRule : ITypeRule {
//You repeat only two times you will check a type.
public RuleResult Check (TypeDefinition type)
{
//rule stuff ...
}
}
and
public AvoidEmptyInterfacesRule : ITypeRule {
//You repeat three times you will check a type
public RuleResult CheckType (TypeDefinition type)
{
//rule stuff
}
}
In conclussion, IMHO, I think we can remove the suffixes without
comprehension problems. But if we maintain it, the price isn't high.
> public interface IAssemblyRule : IRule {
> RuleResult CheckAssembly (AssemblyDefinition assembly);
> }
>
> public interface IModuleRule : IRule {
> RuleResult CheckModule (ModuleDefinition module);
> }
>
> public interface ITypeRule : IRule {
> RuleResult CheckType (TypeDefinition type);
> }
>
> public interface IMethodRule : IRule {
> RuleResult CheckMethod (MethodDefinition method);
> }
> // and we provide new base class (for each case?) to implement the
> basic stuff
> public abstract class Rule : IRule {
>
> private Runner runner;
>
> protected Rule (Runner runner)
> {
> this.runner = runner;
> }
>
> public Runner Runner {
> get { return runner; }
> }
> }
I think with only the Rule abstract class will be suffice, because for
create new rules you will do:
public class AvoidEmptyInterfacesRule : Rule, ITypeRule {
//And you must implement the interface
public RuleResult Check (TypeDefinition type)
{
//rule stuff
}
}
And we can reference as ITypeRule.
> [1] http://anonsvn.mono-project.com/viewcvs/trunk/cecil/gendarme/framework/IRule.cs?view=markup
>
> Feedback welcome :)
Summarizing, I like the new solution, it's simple, well named and
straightforward for create new rules. And with a little tutorial and
the monodoc documentation people can create the rules quickly and
concentrate the effort in the checking. Good work ! :)
Néstor.
Sebastien Pouliot
On Dec 23, 2:48 pm, Néstor Salceda <nestor.salc...@gmail.com> wrote:
> On sáb, 2007-12-22 at 16:29 -0800, Sebastien Pouliot wrote:
> > public interface IRule {
> > // to be supplied with the rule constructor
> > Runner Runner { get; }
> > }
>
> I like the new interface too, with the Runner property. I feel strange
> with empty interfaces but sometimes there are several reasons for
> maintain it; it's a well known problem.
>
> Now, with this change the interface isn't empty and I feel better :)
so it would be nice to fix the violation before the next release ;-)
> > // ??? should we renamed all methods to Check ???
>
> I'm not sure about this, because with this signatures; the framework
> puts special emphasis in the stuff that will be checked. Although, it's
> probabily redundant, by example:
>
> public AvoidEmptyInterfacesRule : ITypeRule {
> //You repeat only two times you will check a type.
> public RuleResult Check (TypeDefinition type)
> {
> //rule stuff ...
> }
>
> }
>
> and
>
> public AvoidEmptyInterfacesRule : ITypeRule {
> //You repeat three times you will check a type
> public RuleResult CheckType (TypeDefinition type)
> {
> //rule stuff
> }
>
> }
>
> In conclussion, IMHO, I think we can remove the suffixes without
> comprehension problems. But if we maintain it, the price isn't high.
think the most confusing time would be when some rules implements more
than one interface, e.g. a naming rule that checks type and method
names. But even then it wouldn't be very confused...
Anyway in doubt maybe it would be better to avoid the change ???
unless other comments clear the doubts :-)
>
>
> > public interface IAssemblyRule : IRule {
> > RuleResult CheckAssembly (AssemblyDefinition assembly);
> > }
>
> > public interface IModuleRule : IRule {
> > RuleResult CheckModule (ModuleDefinition module);
> > }
>
> > public interface ITypeRule : IRule {
> > RuleResult CheckType (TypeDefinition type);
> > }
>
> > public interface IMethodRule : IRule {
> > RuleResult CheckMethod (MethodDefinition method);
> > }
> > // and we provide new base class (for each case?) to implement the
> > basic stuff
> > public abstract class Rule : IRule {
>
> > private Runner runner;
>
> > protected Rule (Runner runner)
> > {
> > this.runner = runner;
> > }
>
> > public Runner Runner {
> > get { return runner; }
> > }
> > }
>
> I think with only the Rule abstract class will be suffice, because for
> create new rules you will do:
attractive, but that's for another email).
> public class AvoidEmptyInterfacesRule : Rule, ITypeRule {
> //And you must implement the interface
> public RuleResult Check (TypeDefinition type)
> {
> //rule stuff
> }
>
> }
>
> And we can reference as ITypeRule.
>
> > Feedback welcome :)
>
> Summarizing, I like the new solution, it's simple, well named and
> straightforward for create new rules. And with a little tutorial and
> the monodoc documentation people can create the rules quickly and
> concentrate the effort in the checking. Good work ! :)
>
> Néstor.
Sebastien
Néstor Salceda
On dom, 2007-12-23 at 11:56 -0800, Sebastien Pouliot wrote:
> > > // ??? should we renamed all methods to Check ???
> >
> > I'm not sure about this, because with this signatures; the framework
> > puts special emphasis in the stuff that will be checked. Although, it's
> > probabily redundant, by example:
> >
> > public AvoidEmptyInterfacesRule : ITypeRule {
> > //You repeat only two times you will check a type.
> > public RuleResult Check (TypeDefinition type)
> > {
> > //rule stuff ...
> > }
> >
> > }
> >
> > and
> >
> > public AvoidEmptyInterfacesRule : ITypeRule {
> > //You repeat three times you will check a type
> > public RuleResult CheckType (TypeDefinition type)
> > {
> > //rule stuff
> > }
> >
> > }
> >
> > In conclussion, IMHO, I think we can remove the suffixes without
> > comprehension problems. But if we maintain it, the price isn't high.
>
> Yes, I'm still unsure myself (enough that it was only a comment ;-). I
> think the most confusing time would be when some rules implements more
> than one interface, e.g. a naming rule that checks type and method
> names. But even then it wouldn't be very confused...
You have reason, I haven't thought in implement more than one
interface. Good catch!
> Anyway in doubt maybe it would be better to avoid the change ???
> unless other comments clear the doubts :-)
Yes, really we haven't a need for changing the names.
Néstor.