Gendarme Rule: Avoid Speculative Generality.
Néstor Salceda
Title: Avoid Speculative Generality
References:
http://sis36.berkeley.edu/projects/streek/agile/bad-smells-in-code.html#SpeculativeGenerality
Description: The rule should check that there aren't presence of this
smell. This smell could be summarized in the phrase "We will need this
feature that day..."
Then there are some signs of this smell:
* Abstract classes that aren't doing much.
For detect them, I can obtain the subclasses in the same assembly and
check if the subclasses doesn´t differ a lot with the base class.
Examples:
Bad:
public abstract class AbstractClass {
public abstract void MakeStuff ();
}
public class OverriderClass : AbstractClass {
public override void MakeStuff ()
{
//Code
}
}
Why we should write an abstract class if only exists one subclass?
Exists some reasons, but we can write:
Good:
public class OverrideClass {
public void MakeStuff () {
//Code
}
}
And the code will be simpler and smaller. When the project advance, we
can refactor quickly.
If we need some extension mechanisms, we can expand the hierarchy and
create the abstract class and create the subclasses then, or we can
inherit from the OverrideClass and modify the code.
* Unnecesary delegation:
I can look for classes that aren't doing to much.
Examples:
Bad:
public class Person {
int age;
string name;
Telephone officePhone;
}
public class Telephone {
int codeArea;
int phone;
}
If we only use Telephone in the Person class, why we should mantain this
class? Can exists reasons, but we can write:
Good:
public class Person {
int age;
string name;
int codeArea;
int phone;
}
And if the project advance and we need the Telephone class in other
class, we can refactor.
* Unused parameters in methods:
2 weeks ago, I write a rule for detect this issue.
Example:
For this case, you can see examples in:
* Methods with odd abstract names.
Generally, these methods contains weird words, by example:
Example:
Bad:
public void SetCdLmt (int lmt)
{
}
What does it mean CdLmt? The name should be clearer:
Good:
public void SetCreditLimit (int creditLimit)
{
}
* And for the test cases, I'm not sure if I can check this sign.
Finally, the implementation has a complex issue: How report the best
rule violation.
The problem is the following, suppose a method with unused parameters:
void Foo (int x, char j)
{
Console.WriteLine (x);
}
Then, I can obtain 2 violations, the unused parameter from
Gendarme.Rules.Performance.AvoidUnusedParameters and other for this
rule. In this case, only show a violation for the Speculative
Generality could be excessive, and only the AvoidUnusedParameters should
be notified. I'm thinking a solution for this kind of cases.
Comments are welcome :)
Néstor.
Sebastien
> Gendarme.Rules.Performance.AvoidUnusedParameters and other for this
> rule. In this case, only show a violation for the Speculative
> Generality could be excessive, and only the AvoidUnusedParameters should
> be notified.
Yes, similar to previous smells comments, it would be nice, when
possible, to avoid reporting a violation of this rule if the only
detected problem is already caught by AvoidUnusedParameters.
> I'm thinking a solution for this kind of cases.
Technically a rule can call another rule (to get hints) or, better,
share parts of it's implementation in (a) common class(es). However in
this case it's may be simpler only to avoid checking the unused
parameters (since that existing rule is much more precise in it's
diagnostic).
Sebastien
Néstor Salceda
Well, first of all, thanks for the tip and the comments Sebastien.
The second is that I have implemented the rule but I have some thoughts.
You can see the tests here:
http://mono-soc-2007.googlecode.com/svn/trunk/nestor/rules/Gendarme.Rules.Smells/Test/AvoidSpeculativeGeneralityTest.cs
And you can see the code here:
http://mono-soc-2007.googlecode.com/svn/trunk/nestor/rules/Gendarme.Rules.Smells/AvoidSpeculativeGeneralityRule.cs
Then, I have detected the abstract classes without responsability, I
have checked the unnecesary delegation simply counting the clients, but
there are 2 issues:
a) If the client is in other assembly, I can't count it, because I will
need a TypeDefinition.
b) I have counted only the fields, I believe that could be interesting
count if appear in the method's signatures or bodies too, because a
client is a class that uses the methods that belongs to other class.
If the AvoidParametersUnusedRule is present in the runner, then I don't
check for unused parameters in this smell.
And I have thought about the bad named methods, I think that could be
useful some spelling rules because in the link that Miguel posted in the
list http://blogs.msdn.com/fxcop/default.aspx some people were
requesting this kind of rules, then I have thought about if we can use
aspell or something similar for this stuff.
Any comments?
Néstor.