Bonjour Cedric,
On Sun, 2008-12-21 at 19:58 +0800, Cedric Vivier wrote:
> Hey,
>
> This new rule in the Design category checks that visible methods and
> properties returning an array,
> a collection, or an enumerable, do not return null.
I like the idea of the rule but not inside Design. The idea is to keep
Design about API design so that its rules applies very early in the
development (e.g. when generated using CASE tools or for TDD when tests
are complete and, hopefully, the needed code [auto]generated).
This means no processing of Instruction should be needed, which in turns
keep our OpCodeEngine out of the assembly - making using Design-only
rules very fast (near zero initialization time).
There's already an abstract ReturnNullRule* (in BadPractice) that two
other rules are using. So this may be a good location (but the rule
should be renamed) and likely reusable for your rule.
* it's similar to what you're doing but also covers the "ugly"
IL that MS CSC generates in debug builds.
> An API is usually easier to use when such methods return an empty
> instance instead, the consumer
> being able to use the result directly without having to do a
> null-check first.
...
> +namespace Gendarme.Rules.Design {
> +
> + /// <summary>
> + /// This rule checks that visible methods and properties returning an
> array,
> + /// a collection, or an enumerable, do not return null.
> + /// An API is usually easier to use when such methods return an empty
> instance
> + /// instead, the consumer being able to use the result directly
> without having
> + /// to do a null-check first.
> + /// </summary>
> + /// <example>
> + /// Bad example:
When there is more than one example include a small description about
it, between '(' ')' - e.g. "Bad example (string):". That makes it easier
to refer someone to the right example.
> + /// <code>
> + /// public string DisplayName {
> + /// get {
> + /// if (IsAnonymous)
> + /// return null;
> + /// return name;
> + /// }
> + /// }
> + /// </code>
> + /// </example>
> + /// <example>
> + /// Good example:
> + /// <code>
> + /// public string DisplayName {
> + /// get {
> + /// if (IsAnonymous)
> + /// return string.Empty;
> + /// return name;
> + /// }
> + /// }
> + /// </code>
> + /// </example>
> + /// <example>
> + /// Bad example:
> + /// <code>
> + /// public int [] GetOffsets () {
> + /// if (!store.HasOffsets)
> + /// return null;
> + /// store.LoadOffsets ();
> + /// return store.Offsets;
> + /// }
> + /// </code>
> + /// </example>
> + /// <example>
> + /// Good example:
> + /// <code>
> + /// public int [] GetOffsets () {
> + /// if (!store.HasOffsets)
> + /// return new int [0];
It would be nicer to return a statically allocated int[0] instance since
this is the only case where an array cannot be changed (so it can safely
be reused).
> + /// store.LoadOffsets ();
> + /// return store.Offsets.ToArray ();
> + /// }
> + /// </code>
> + /// </example>
> + /// <example>
> + /// Bad example:
> + /// <code>
> + /// public IEnumerable<int> GetOffsets () {
> + /// if (!store.HasOffsets)
> + /// return null;
> + /// store.LoadOffsets ();
> + /// return store.Offsets;
> + /// }
> + /// </code>
> + /// </example>
> + /// <example>
> + /// Good example:
> + /// <code>
> + /// public IEnumerable<int> GetOffsets () {
> + /// if (!store.HasOffsets)
> + /// yield break;
> + /// store.LoadOffsets ();
> + /// foreach (int offset in store.Offsets)
> + /// yield return offset;
> + /// }
> + /// </code>
> + /// </example>
> +
> + [Problem ("This method returns null, an empty instance instead would
> make the API easier to use.")]
> + [Solution ("Return an empty instance instead of null.")]
> + [EngineDependency (typeof (OpCodeEngine))]
> + public class PreferEmptyInstanceOverNullRule : Rule, IMethodRule {
> +
> + static OpCodeBitmask ldnullBitmask = BuildLdnullOpCodeBitmask
> ();
> +
> + public RuleResult CheckMethod (MethodDefinition method)
> + {
> + if (!method.HasBody || !method.IsVisible ())
> + return RuleResult.DoesNotApply;
> +
> + //only apply to methods returning string, array, or
> IEnumerable-impl
> + if (method.ReturnType.ReturnType.FullName !=
> "System.String"
> + && !method.ReturnType.ReturnType.IsArray ()
> + && !method.ReturnType.ReturnType.Implements
> ("System.Collections.IEnumerable"))
That's another case (not foreach related) where a variable should be
used (3 x method.ReturnType.ReturnType). I think I'll look into Marek's
rule suggestion soon ;-)
> + return RuleResult.DoesNotApply;
> +
> + //method never use literal null
> + if (!ldnullBitmask.Intersect (OpCodeEngine.GetBitmask
> (method)))
> + return RuleResult.Success;
> +
> + foreach (Instruction ins in method.Body.Instructions)
> {
> + if (ins.OpCode.Code != Code.Ret)
> + continue;
> +
> + Instruction ld = ins.TraceBack (method);
> + if (ld != null && ld.OpCode.Code ==
> Code.Ldnull) {
See top comment about MS CSC ugly IL.
> + string msg = string.Format ("Replace
> null with {0}.", GetFixSuggestion (method.ReturnType.ReturnType));
> + Runner.Report (method, ins,
> Severity.Medium, Confidence.High, msg);
> + }
> + }
> + return Runner.CurrentRuleResult;
> + }
> +
> + static string GetFixSuggestion (TypeReference type)
> + {
> + if (type.FullName == "System.String")
> + return "string.Empty";
> + else if (type.IsArray ())
> + return string.Format ("an empty {0} array",
> type.Name);
> + else if (type.FullName.StartsWith
> ("System.Collections.Generic.IEnumerable"))
> + return "yield break (or equivalent)";
> + return "an empty collection";
> + }
> +
> + static OpCodeBitmask BuildLdnullOpCodeBitmask ()
> + {
> + #if true
> + return new OpCodeBitmask (0x100000, 0x0, 0x0,
> 0x0);
> + #else
> + OpCodeBitmask mask = new OpCodeBitmask ();
> + mask.Set (Code.Ldnull);
> + return mask;
> + #endif
> + }
> + }
> +}
> diff --git
> a/gendarme/rules/Gendarme.Rules.Design/Test/PreferEmptyInstanceOverNullTest.cs
>
> b/gendarme/rules/Gendarme.Rules.Design/Test/PreferEmptyInstanceOverNullTest.cs
> new file mode 100644
Tests looks fine.
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
-~----------~----~----~----~------~----~------~--~---
