Gendarme Rule: Detect long methods.
Néstor Salceda
As I said yesterday, I'm going to specify a new rule for a new smell.
Title: Detect Long Methods.
Description: The rule should check that a method isn't too long.
References:
http://sis36.berkeley.edu/projects/streek/agile/bad-smells-in-code.html#Long+Method
I have get more references from some books:
* Refactoring by Martin Fowler (Page 76)
* Refactoring to Patterns by Joshua Kerivevsky (Page 40)
* Code Complete 2nd Edition by Steve McConnell (Page 565)
Examples:
Well, writting examples for this smell is a complex task because find
the measure unit could be a bit controversial.
If I put a small limit (by example 10 lines of code per method) I will
get a lot of errors. If I put a big limit (by example 300 lines of code
per method) I will get few errors. With these limits the rule won't be
useful.
Then, IMHO I should measure first a real program (by example the mono
corlib) and get some data about methods' length. I also can measure a
screen of written code and obtain the instructions and count it (I like
this approach). I also can read about software metrics and apply it.
Also if anyone has some advice or measure, I will be glad for listen
it.
Néstor.
Sebastien
You're right that the biggest challenge for this one will be to
balance correctly.
BTW, I'll be on vacation this week. I'll be back next weekend (in time
for mid-summer review).
Please post your next status report(s) before then and commit
everything you have into SVN.
Thanks!
On Jul 6, 4:33 pm, Néstor Salceda <nestor.salc...@gmail.com> wrote:
> Hello,
>
> As I said yesterday, I'm going to specify a new rule for a new smell.
>
> Title: Detect Long Methods.
>
> Description: The rule should check that a method isn't too long.
>
> References:
>
> http://sis36.berkeley.edu/projects/streek/agile/bad-smells-in-code.ht...
game...@gmail.com
account for.
Presence of conditionals, iterations, comments. A practical case to
consider is
that the auto generated InitializeComponent method would often be a
long method
(but without any conditionals, iterations). Furthermore, in my
research on bad smells,
I've seen that bad smell candidates follow a non-normal distribution.
Meaning that most methods
would be small, and few would be 'large'. You could probably take a
look a the distribution of
method sizes to help you pick a cut-off.
On Jul 6, 4:33 pm, Néstor Salceda <nestor.salc...@gmail.com> wrote:
> Hello,
>
> As I said yesterday, I'm going to specify a new rule for a new smell.
>
> Title: Detect Long Methods.
>
> Description: The rule should check that a method isn't too long.
>
> References:
>
> http://sis36.berkeley.edu/projects/streek/agile/bad-smells-in-code.ht...
Néstor Salceda
> Hello Nestor,
>
> You're right that the biggest challenge for this one will be to
> balance correctly.
>
> BTW, I'll be on vacation this week. I'll be back next weekend (in time
> for mid-summer review).
> Please post your next status report(s) before then and commit
> everything you have into SVN.
Okey I will do it. Enjoy with your holidays :)
> Thanks!
Néstor Salceda
> Fowler gives other advice in addition method length that you can
> account for.
> Presence of conditionals, iterations, comments. A practical case to
> consider is
> that the auto generated InitializeComponent method would often be a
> long method
> (but without any conditionals, iterations). Furthermore, in my
> research on bad smells,
> I've seen that bad smell candidates follow a non-normal distribution.
> Meaning that most methods
> would be small, and few would be 'large'. You could probably take a
> look a the distribution of
> method sizes to help you pick a cut-off.
Yes, I also believe that.
I can check presence of conditionals and iterations, but I can't check
the comments in the code because I inspect the IL code, not the source
code.
But with your advice I can check the auto generated code looking for
conditional branches.
Thank you for your advice. I haven't thought in this special case.
Néstor.
Néstor Salceda
I have finished the rule, and I will post the links to the code for
review.
Well, first of all; I have measure a screen of code (and a bit more)
and count the instructions, and I rounded up. This is the reason for
the 170 IL instructions as max.
I want also comment about the autogenerated methods, I have checked if
the pattern that autogenerated method doesn't (generally) contain
condition branch instructions is always true, but for example stetic
generate a line with this instruction and the rule didn't pass the
tests, then I have prefered write a well known autogenerated method
names for skip the check in those methods. If anyone knows more well
known names and can tell me those names I will thank.
Néstor.
game...@gmail.com
On Jul 10, 12:37 pm, Néstor Salceda <nestor.salc...@gmail.com> wrote:
> Hello,
>
> I have finished the rule, and I will post the links to the code for
> review.
>
> Testshttp://mono-soc-2007.googlecode.com/svn/trunk/nestor/rules/Gendarme.R...
>
> Codehttp://mono-soc-2007.googlecode.com/svn/trunk/nestor/rules/Gendarme.R...
Néstor Salceda
> Can you check the base type of the class as an extra precaution?
Sure, I will do it.
Thanks for the tip :)
Néstor Salceda
I'm sorry, I forgot send the mail saying that I updated this rule with
this comment.
You can get the code and the tests in usual links:
Any comment?
Néstor.
game...@gmail.com
different users.
Do you think that you could eventually have a config file or dialog
window that allows
you to change these variables without having to change the code
itself.
On Jul 27, 6:47 am, Néstor Salceda <nestor.salc...@gmail.com> wrote:
> El mié, 11-07-2007 a las 03:09 +0000, gamew...@gmail.com escribió:
>
> > Can you check the base type of the class as an extra precaution?
>
> I'm sorry, I forgot send the mail saying that I updated this rule with
> this comment.
>
> You can get the code and the tests in usual links:
>
> Testhttp://mono-soc-2007.googlecode.com/svn/trunk/nestor/rules/Gendarme.R...
>
> Codehttp://mono-soc-2007.googlecode.com/svn/trunk/nestor/rules/Gendarme.R...
>
> Any comment?
>
> Néstor.
Néstor Salceda
> A lot of the smells will require some tweaking of the parameters for
> different users.
> Do you think that you could eventually have a config file or dialog
> window that allows
> you to change these variables without having to change the code
> itself.
Yes, I have thought this option. I believe that I can add this
configuration to the rules.xml file, by example:
<ruleset name="smells">
<rule include="*" from="Gendarme.Rules.Smells.dll"/>
<parameter name="MaxInstructions" value="170"
type="DetectLongMethodRule"/>
</ruleset>
But, I want read the Sebastien opinion before start writting some code,
because it's probable that he had thought about this kind of
initialization.
Thanks for the tip Chris.
Néstor.
Sebastien
compiling the rule.
Please make sure there's a default value if the XML file doesn't
specify one.
On Jul 30, 10:21 am, Néstor Salceda <nestor.salc...@gmail.com> wrote:
Néstor Salceda
> Good idea. It will make it easier to test different value wrt re-
> compiling the rule.
> Please make sure there's a default value if the XML file doesn't
> specify one.
Thanks Sebastien.
I will write the code when I had finished the rule for Avoid unused
parameters.
Néstor.
Néstor Salceda
El lun, 30-07-2007 a las 15:25 +0000, Sebastien escribió:
> Good idea. It will make it easier to test different value wrt re-
> compiling the rule.
> Please make sure there's a default value if the XML file doesn't
> specify one.
Okey, I have a patch for allow parameters in the rules.xml config.
Well, the first, the patch.
The patch allows the following:
* Set some parameters in a ruleset. By example, my custom.xml for
develop the patch is the following:
<gendarme>
<ruleset name="default">
<rules include="*"
from="rules/Gendarme.Rules.Concurrency/Gendarme.Rules.Concurrency.dll" />
<rules include="*"
from="rules/Gendarme.Rules.Performance/Gendarme.Rules.Performance.dll" />
<rules include="*"
from="rules/Gendarme.Rules.Security/Gendarme.Rules.Security.dll" />
<rules include="*"
from="rules/Gendarme.Rules.Portability/Gendarme.Rules.Portability.dll" />
<rules include="*"
from="rules/Gendarme.Rules.Exceptions/Gendarme.Rules.Exceptions.dll" />
<rules include="*"
from="rules/Gendarme.Rules.Ui/Gendarme.Rules.Ui.dll" />
<rules include="*"
from="/home/nestor/devel/soc/mono-soc-2007/nestor/rules/Gendarme.Rules.Smells/Gendarme.Rules.Smells.dll"/>
<parameter name="MaxInstructions" value="170"
rule="Gendarme.Rules.Smells.DetectLongMethodRule"/>
</ruleset>
</gendarme>
I have to adjust the path to rules, and I have added the smells
assembly.
The last line is the important:
<parameter name="MaxInstructions" value="170"
rule="Gendarme.Rules.Smells.DetectLongMethodRule"/>
Then, you have to specify the property name that references the
parameter, the value and the rule for apply.
* You can specify different rulesets with different parameters. For
example, you can configure the smells for make some hard checks. Method
Long < 50 and Parameter List < 4.
* You can modify the values without recompile.
And finally, you can see a rule modified in the following link:
http://mono-soc-2007.googlecode.com/svn/trunk/nestor/rules/Gendarme.Rules.Smells/DetectLongMethodRule.cs
* The solution is backward compatible, then we haven't to modify the
existing rules. If I want to make a rule with parameters, I only have
to write the public property and that's all, but the rule author is the
responsible of the initialization for the default value.
* By the moment, I only allow integers to a valid value. This is
because I haven't need other types.
If all is okey, I can send it to mono-devel list or anyone can make the
commit. If there are errors or issues or something that could be
improved, please tell me :)
Néstor.