On Mon, 2008-12-22 at 03:40 +0800, Cedric Vivier wrote:
> Updated patch (0002-...) according to review.
>
> New patch (0003-) over 0002, which renames ToStringReturnsNullRule to
> ToStringShouldNotReturnNullRule, and improves text and consistency
> between ReturnNullRule inheritors.
>
> Please review,
few minor nitpick inline. Go ahead and commit when fixed.
...
> + [Problem ("This method returns null whereas returning an empty
> instance would make it easier to use.")]
> + [Solution ("Return an empty instance rather than null.")]
> + public class PreferEmptyInstanceOverNullRule : ReturnNullRule,
> IMethodRule {
> +
> + TypeReference returnType;
> +
> + public override RuleResult CheckMethod (MethodDefinition
> method)
> + {
> + if (!method.HasBody)
> + return RuleResult.DoesNotApply;
> +
> + //the rule does not apply to the particular case of
> ToString()
> + //that have its own ToStringShouldNotReturnNullRule
> + if (!method.HasParameters && method.Name ==
> "ToString")
> + return RuleResult.DoesNotApply;
> +
> + //only apply to methods returning string, array, or
> IEnumerable-impl
> + returnType = method.ReturnType.ReturnType;
> + if (returnType.FullName != "System.String"
> + && !returnType.IsArray ()
> + && !returnType.Implements
> ("System.Collections.IEnumerable"))
> + return RuleResult.DoesNotApply;
don't use space to align (it mess display for people who use something
else than 8 spaces for tabs). Add { } if the condition takes more than
one line (that will make the separation easier to spot).
> +
> + return base.CheckMethod (method);
> + }
> +
> + protected override void Report (MethodDefinition method,
> Instruction ins)
> + {
> + string msg = string.Format ("Replace null with {0}.",
> GetReturnTypeSuggestion ());
> + Runner.Report (method, ins, method.IsVisible () ?
> Severity.Medium : Severity.Low, Confidence.High, msg);
> + }
> +
> + string GetReturnTypeSuggestion ()
> + {
> + if (returnType.FullName == "System.String")
> + return "string.Empty";
> + else if (returnType.IsArray ())
> + return string.Format ("an empty {0} array",
> returnType.Name);
> + else if (returnType.FullName.StartsWith
> ("System.Collections.Generic.IEnumerable"))
> + return "yield break (or equivalent)";
> + return "an empty collection";
> + }
> + }
> +}
> diff --git a/gendarme/rules/Gendarme.Rules.BadPractice/ReturnNullRule.cs
> b/gendarme/rules/Gendarme.Rules.BadPractice/ReturnNullRule.cs
> index 26206f7..b797422 100644
> --- a/gendarme/rules/Gendarme.Rules.BadPractice/ReturnNullRule.cs
> +++ b/gendarme/rules/Gendarme.Rules.BadPractice/ReturnNullRule.cs
> @@ -34,12 +34,8 @@ using Gendarme.Framework.Rocks;
> namespace Gendarme.Rules.BadPractice {
>
> // Notes:
> - // * We don't implement IMethodRule on purpose since both rules that
> inherit
> - // from us are ITypeRule (that check for a specific method in the
> type)
> - // * Both rules used Severity.Medium and Confidence.Normal so they
> are, right
> - // now, hardcoded. If anyone else needs them they should be made
> abstract
> - // properties. Severity is likely to change but Confidence should
> stay the
> - // same unless addional logic change it.
> + // * We don't implement IMethodRule on purpose since a rule that
> inherit
> + // from us can be an ITypeRule (checking for a specific method in
> the type)
>
> [EngineDependency (typeof (OpCodeEngine))]
> abstract public class ReturnNullRule : Rule {
> @@ -60,7 +56,7 @@ namespace Gendarme.Rules.BadPractice {
> while (previous != null) {
> // most of the time we'll find the
> null value on the first trace back call
> if (previous.OpCode.Code ==
> Code.Ldnull) {
> - Runner.Report (method, ins,
> Severity.Medium, Confidence.Normal);
> + Report (method, ins);
> break;
> }
>
> @@ -76,5 +72,10 @@ namespace Gendarme.Rules.BadPractice {
>
> return Runner.CurrentRuleResult;
> }
> +
> + protected virtual void Report (MethodDefinition method,
> Instruction ins)
> + {
> + Runner.Report (method, ins, Severity.Medium,
> Confidence.Normal);
> + }
> }
> }
...
> diff --git
> a/gendarme/rules/Gendarme.Rules.BadPractice/CloneMethodShouldNotReturnNullRule.cs
>
> b/gendarme/rules/Gendarme.Rules.BadPractice/CloneMethodShouldNotReturnNullRule.cs
> index c1fb4c3..c08fbca 100644
> ---
> a/gendarme/rules/Gendarme.Rules.BadPractice/CloneMethodShouldNotReturnNullRule.cs
> +++
> b/gendarme/rules/Gendarme.Rules.BadPractice/CloneMethodShouldNotReturnNullRule.cs
> @@ -27,6 +27,7 @@
> // THE SOFTWARE.
>
> using Mono.Cecil;
> +using Mono.Cecil.Cil;
I don't think this is needed
> using Gendarme.Framework;
> using Gendarme.Framework.Helpers;
...
> @@ -1,5 +1,5 @@
> //
> -// Gendarme.Rules.BadPractice.ToStringReturnsNullRule
> +// Gendarme.Rules.BadPractice.ToStringShouldNotReturnNullRule
> //
> // Authors:
> // Nidhi Rawal <[email protected]>
> @@ -27,6 +27,7 @@
> // THE SOFTWARE.
>
> using Mono.Cecil;
> +using Mono.Cecil.Cil;
same
>
> using Gendarme.Framework;
> using Gendarme.Framework.Helpers;
> @@ -35,8 +36,9 @@ using Gendarme.Framework.Rocks;
> namespace Gendarme.Rules.BadPractice {
>
> /// <summary>
> - /// This rule is used for ensure that no overriden <c>ToString()</c>
> method returns a
> - /// null value. This makes the value more useful in debugging.
> + /// This rule checks that no overriden <c>ToString()</c> method
> returns <c>null</c>.
> + /// An appropriately descriptive string, or <c>string.Empty</c>,
> should be returned
> + /// in order to make the value more useful (especially in debugging).
> /// </summary>
> /// <example>
> /// Bad example:
> @@ -56,10 +58,11 @@ namespace Gendarme.Rules.BadPractice {
> /// }
> /// </code>
> /// </example>
> + /// <remarks>This rule was previously named
> ToStringReturnsNull.</remarks>
Please add the version where the name change occurred (e.g. Before
Gendarme 2.4 this rule was named...). At some future point ('X'
versions) we'll start removing them (or at least move them elsewhere).
Thanks!
Sebastien
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Gendarme" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/gendarme?hl=en
-~----------~----~----~----~------~----~------~--~---
