Three Gendarme rules about properties

[GoogleCodeExporter]

This task is to create three (3) rule for Gendarme [1] to check properties.

a) AvoidPublicInstanceFieldsRule

Find and report public, instance fields since they should probably be
properties.

b) ConsiderConvertingMethodToPropertyRule

Some methods have name that are "property-like" (e.g. starts with Get) and
do not take any argument, e.g. int GetValue (). The rule should find and
report (warn) such method so the developer can change them to properties
(if indeed this is the right thing to do).

c) AvoidReturningArraysOnPropertiesRule

This rule should report all properties that return arrays, e.g. public
byte[] Key { get; set; } because it's never clear if the returned data is
cloned, or if the caller can keep a reference to it and changes value
afterward.

So 3 easy rules for Gendarme. Nice introduction if you don't yet know about
Cecil (or Gendarme). The trick here is that they should share some logic to
avoid "recommending" to turn a method/field into a property that another
rule would reject (e.g. byte[] GetKey () shouldn't be a candidate).

To complete the task it must include:
- the rules source code;
- unit tests to ensure the rules works correctly;
- documentation for the web site (see [2] for an example)

This tasks should take between 2-5 days (depending if you already know
Gendarme/Cecil or not).

[1] http://www.mono-project.com/Gendarme
[2] http://groups.google.com/group/gendarme

Original issue reported on code.google.com by sebastie...@gmail.com on 29 Dec 2007 at 3:03

[GoogleCodeExporter]

> Some methods have name that are "property-like" (e.g. starts with Get) and
do not take any argument, e.g. int GetValue (). The rule should find and
report (warn) such method so the developer can change them to properties
(if indeed this is the right thing to do).

I'd also suggest warning about methods like IsSomething ().

Original comment by dan.abra...@gmail.com on 29 Dec 2007 at 11:44

[GoogleCodeExporter]

Good point! Thanks :)

Original comment by sebastie...@gmail.com on 29 Dec 2007 at 3:38

[GoogleCodeExporter]

I'd like to claim this task.

Original comment by AdT...@gmail.com on 29 Dec 2007 at 3:50

[GoogleCodeExporter]

It's all yours!

Original comment by sebastie...@gmail.com on 29 Dec 2007 at 4:00

• Changed state: Claimed
• Added labels: ClaimedBy-AdTsai, DueDate-2008-01-06

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Attached are the files.

Original comment by AdT...@gmail.com on 30 Dec 2007 at 8:48

Attachments:

• AdTsai-Mono-Gendarme1.zip

[GoogleCodeExporter]

Hello,

Here's some quick notes

1. Wiki.txt
   Looks great, thanks for the formatting :)

2. TypeHelper.cs
   2.1 IsProperty : we now have (in SVN) extensions methods for IsGetter and
IsSetter, which use MethodSemanticAttributes - which is safer that a string 
compare.
   2.2 Please avoid using regex, they are generally slow and harder to understand (I
admit I can't ;-)

3. AvoidPublicInstanceFieldsRule.cs

   Avoid creating a MessageCollection until you're sure to need one. The rules are
called many times and the unneeded memory allocation are slowing down the 
process
(there are still a few rules doing this, but we're updating them slowly)

4. AvoidReturningArraysOnPropertiesRule.cs

   The first check should ensure it's only a getter (no need to check for setters
since they don't return any value). The common style (which will be enforced in
future Gendarme version) is to first check for rule applicability, then the rule
itself. E.g.

    // available with extensions methods and using Gendarme.Framework.Rocks
    if (!method.IsGetter ())
        return runner.RuleSuccess;

    // rule applies
    ...

5. ConsiderConvertingMethodToPropertyRule

   // If it starts with "set" and has at least 1 parameter

   Actually the method should have exactly one parameter, otherwise it can't be turn
into a property.

6. In general there's a few coding style issue (like a missing space before any 
'(')
wrt to Mono guidelines (available on the wiki). You might want to update your 
editor
configuration to match those.

Overall there no real logic problem, so you seems to have catch Gendarme and 
Cecil
pretty quickly :)

p.s. I'll try to do more review tonight (in about 6 hours) but I'll be out of 
town
for the next 2 days.

Original comment by sebastie...@gmail.com on 30 Dec 2007 at 8:42

[GoogleCodeExporter]

Thanks for the tips!

Attached are the updated files, with the fixes you requested.

Original comment by AdT...@gmail.com on 31 Dec 2007 at 5:38

Attachments:

• AdTsai-Mono-Gendarme2.zip

[GoogleCodeExporter]

ok a few more things

* AvoidPublicInstanceFieldsRule will report a warning for a field with an array 
type.
Fixing this will only make another rule fail. E.g.

    public class ShouldBeSpecialBecauseOfArray {
        public byte [] key;
    }

Now the right action may not be to ignore it, but to return a different Message 
to
suggest using a private field and a SetKey method (i.e. something like: "Set" +
Char.Upper(p) + p.Substring(1)) instead of the turn to property message. Please 
add a
test case to cover this.

* ConsiderConvertingMethodToPropertyRule

The case for Set hasn't been fixed (from my previous comment). This case exists 
only
if a single parameter is supplied, i.e. != 0 doesn't work, like in this case:

        public class ShouldBeIgnoredMultipleValuesInSet {
            long value;

            public long GetValue ()
            {
                return value;
            }

            public void SetValue (int value, int factor)
            {
                this.value = (long)(value * factor);
            }
        }

Other files are fine (please don't update them since they are already 
integrated in
my gendarme build locally).

Thanks & Happy New Year!
Sebastien

Original comment by sebastie...@gmail.com on 2 Jan 2008 at 2:32

[GoogleCodeExporter]

3 files were changed: AvoidPublicInstanceFieldsTest.cs, 
AvoidPublicInstanceFieldsRule.cs, ConsiderConvertingMethodToPropertyRule.cs

> The case for Set hasn't been fixed (from my previous comment).
Whoops! It seems that I updated the comment, but forgot to change the relevant 
code. 
Sorry about that. Fixed.

Original comment by AdT...@gmail.com on 3 Jan 2008 at 2:00

Attachments:

• AdTsai-Mono-Gendarme3.zip

[GoogleCodeExporter]

ok, final check here :) i.e. "make self-test", this is where we run gendarme 
(all
rules) against itself and see if we follow our own rules.

1) AvoidPublicInstanceFieldsRule 

* should not be applied on enums, otherwise all it's fields/value will be 
reported as
candidates for properties (i.e. many false positives). You can start 
CheckMethod with

        // rule doesn't apply on enums
        if (type.IsEnum)
            return runner.RuleSuccess;

* should not be applied on static fields (see rule name ;-). Same goes for 
const and
read-only fields. See the updated unit tests (attached). note: namespace 
renamed to
Design (not properties)

2) ConsiderConvertingMethodToPropertyRule 

* static bool StartsWith (string Start, string Name)
  parameters are misnamed (and Gendarme catch this) since they should be camelCased.

* should have a white-list of methods to ignore (because of how the framework 
named
some methods). Using a list ensure we can easily extend it in the future
  e.g. GetHashCode, GetEnumerator

* should check for generated code (I'll add this check since it's going thru an
update in the rocks right now)

Original comment by sebastie...@gmail.com on 3 Jan 2008 at 1:58

Attachments:

• AvoidPublicInstanceFieldsTest.cs

[GoogleCodeExporter]

I've updated the files with your suggestions. In the whitelist, I've only 
included 
the most common method names. There are a lot more which are names similarly in 
the .NET framework, but are aren't as common. Also, there may be a problem if, 
for 
example, I write a class which had a method named "GetEnumerator". In that 
case, 
gendarme won't catch it since it's on the whitelist. I'm not sure of a way to 
solve 
that, short of including the name of the type along with the method name in the 
whitelist (i.e. "Object.GetHashCode()", etc.)

Attached are the updated files.

Original comment by AdT...@gmail.com on 4 Jan 2008 at 11:12

Attachments:

• AdTsai-Mono-Gendarme4.zip

[GoogleCodeExporter]

Congratulations! your first rules are now in SVN :)

Closing task
Thanks
Sebastien

Original comment by sebastie...@gmail.com on 4 Jan 2008 at 1:39

• Changed state: Closed

[GoogleCodeExporter]

Thanks for all the suggestions and advice. I certainly learned a lot!

Original comment by AdT...@gmail.com on 4 Jan 2008 at 2:32