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.
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.
Bad example:
public string DisplayName {
get {
if (IsAnonymous)
return null;
return name;
}
}
Good example:
public string DisplayName {
get {
if (IsAnonymous)
return string.Empty;
return name;
}
}
Bad example 2:
public int [] GetOffsets () {
if (!store.HasOffsets)
return null;
store.LoadOffsets ();
return store.Offsets.ToArray ();
}
Good example 2:
public int [] GetOffsets () {
if (!store.HasOffsets)
return new int [0];
store.LoadOffsets ();
return store.Offsets.ToArray ();
}
Bad example 3:
public IEnumerable<int> GetOffsets () {
if (!store.HasOffsets)
return null;
store.LoadOffsets ();
return store.Offsets;
}
Good example 3:
public IEnumerable<int> GetOffsets () {
if (!store.HasOffsets)
yield break;
store.LoadOffsets ();
foreach (int offset in store.Offsets)
yield return offset;
}
Please review,
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---
From cce9623dc217df9b9ab06b2ccdf3f883234e8f0c Mon Sep 17 00:00:00 2001
From: Cedric Vivier <[email protected]>
Date: Sun, 21 Dec 2008 18:46:03 +0800
Subject: [PATCH] WIP in 'PreferEmptyInstanceOverNullRule' branch.
2008-12-21 Cedric Vivier <[email protected]>
* PreferEmptyInstanceOverNullRule.cs: New. This rule checks that
visible methods/properties returning an array, a collection, or
an enumerable, do not return null.
---
gendarme/rules/Gendarme.Rules.Design/ChangeLog | 6 +
gendarme/rules/Gendarme.Rules.Design/Makefile.am | 2 +
.../PreferEmptyInstanceOverNullRule.cs | 173 +++++++++++++++++
.../Test/PreferEmptyInstanceOverNullTest.cs | 199 ++++++++++++++++++++
4 files changed, 380 insertions(+), 0 deletions(-)
create mode 100644 gendarme/rules/Gendarme.Rules.Design/PreferEmptyInstanceOverNullRule.cs
create mode 100644 gendarme/rules/Gendarme.Rules.Design/Test/PreferEmptyInstanceOverNullTest.cs
diff --git a/gendarme/rules/Gendarme.Rules.Design/ChangeLog b/gendarme/rules/Gendarme.Rules.Design/ChangeLog
index 897c2ad..89d39e1 100644
--- a/gendarme/rules/Gendarme.Rules.Design/ChangeLog
+++ b/gendarme/rules/Gendarme.Rules.Design/ChangeLog
@@ -1,3 +1,9 @@
+2008-12-21 Cedric Vivier <[email protected]>
+
+ * PreferEmptyInstanceOverNullRule.cs: New. This rule checks that
+ visible methods/properties returning an array, a collection, or
+ an enumerable, do not return null.
+
2008-12-06 Sebastien Pouliot <[email protected]>
* AvoidRefAndOutParametersRule.cs: Use HasParameters
diff --git a/gendarme/rules/Gendarme.Rules.Design/Makefile.am b/gendarme/rules/Gendarme.Rules.Design/Makefile.am
index 9d8f7a4..09dd726 100644
--- a/gendarme/rules/Gendarme.Rules.Design/Makefile.am
+++ b/gendarme/rules/Gendarme.Rules.Design/Makefile.am
@@ -35,6 +35,7 @@ rules_sources = \
MissingAttributeUsageOnCustomAttributeRule.cs \
OperatorEqualsShouldBeOverloadedRule.cs \
OverrideEqualsMethodRule.cs \
+ PreferEmptyInstanceOverNullRule.cs \
PreferEventsOverMethodsRule.cs \
PreferIntegerOrStringForIndexersRule.cs \
ProvideAlternativeNamesForOperatorOverloadsRule.cs \
@@ -76,6 +77,7 @@ tests_sources = \
MissingAttributeUsageOnCustomAttributeTest.cs \
OperatorEqualsShouldBeOverloadedTest.cs \
OverrideEqualsMethodTest.cs \
+ PreferEmptyInstanceOverNullTest.cs \
PreferEventsOverMethodsTest.cs \
PreferIntegerOrStringForIndexersTest.cs \
ProvideAlternativeNamesForOperatorOverloadsTest.cs \
diff --git a/gendarme/rules/Gendarme.Rules.Design/PreferEmptyInstanceOverNullRule.cs b/gendarme/rules/Gendarme.Rules.Design/PreferEmptyInstanceOverNullRule.cs
new file mode 100644
index 0000000..2eefbd9
--- /dev/null
+++ b/gendarme/rules/Gendarme.Rules.Design/PreferEmptyInstanceOverNullRule.cs
@@ -0,0 +1,173 @@
+//
+// Gendarme.Rules.Design.PreferEmptyInstanceOverNullRule
+//
+// Authors:
+// Cedric Vivier <[email protected]>
+//
+// Copyright (C) 2008 Cedric Vivier
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+using System;
+
+using Mono.Cecil;
+using Mono.Cecil.Cil;
+
+using Gendarme.Framework;
+using Gendarme.Framework.Rocks;
+using Gendarme.Framework.Engines;
+using Gendarme.Framework.Helpers;
+
+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:
+ /// <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];
+ /// 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"))
+ 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) {
+ 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
index 0000000..6adc2da
--- /dev/null
+++ b/gendarme/rules/Gendarme.Rules.Design/Test/PreferEmptyInstanceOverNullTest.cs
@@ -0,0 +1,199 @@
+//
+// Unit tests for PreferEmptyInstanceOverNullRule
+//
+// Authors:
+// Cedric Vivier <[email protected]>
+//
+// Copyright (C) 2008 Cedric Vivier
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+using System;
+using System.Collections;
+using System.Collections.Generic;
+
+using Gendarme.Rules.Design;
+
+using NUnit.Framework;
+using Test.Rules.Fixtures;
+using Test.Rules.Definitions;
+
+namespace Test.Rules.Design {
+
+ [TestFixture]
+ public class PreferEmptyInstanceOverNullTest : MethodRuleTestFixture<PreferEmptyInstanceOverNullRule> {
+
+ public class DoesNotApplyCases {
+ int [] Private () {
+ return null;
+ }
+
+ public void Void () {
+ Console.WriteLine ("Foo");
+ }
+
+ public int Int () {
+ return 0;
+ }
+
+ public Type Type () {
+ return null;
+ }
+ }
+
+ public class SuccessCases {
+ bool foo;
+ string x;
+
+ public int [] IntArray () {
+ x = null;
+ return new int [0];
+ }
+
+ public string String () {
+ x = null;
+ return "foo";
+ }
+
+ public string String2 () {
+ return string.Empty;
+ }
+
+ public string String3 () {
+ if (!foo)
+ return string.Empty;
+ return this.ToString ();
+ }
+
+ public IEnumerable<int> GetOffsets () {
+ if (!foo)
+ yield break;
+ //store.LoadOffsets ();
+ foreach (int offset in GetOffsets ())
+ yield return offset;
+ }
+
+ public ArrayList Collection () {
+ if (!foo)
+ return new ArrayList ();
+ var list = new ArrayList ();
+ list.Add (4);
+ list.Add (8);
+ return list;
+ }
+
+ public List<int> GenericCollection () {
+ if (!foo)
+ return new List<int> ();
+ var list = new List<int> ();
+ list.Add (4);
+ list.Add (8);
+ return list;
+ }
+ }
+
+ public class FailureCases {
+ bool foo = false;
+ string s;
+
+ public int [] IntArray () {
+ if (!foo)
+ return null;
+ return new int [1];
+ }
+
+ public string String () {
+ return null;
+ }
+
+ public string String2 () {
+ if (!foo)
+ return null;
+ return "foo";
+ }
+
+ public string String3 () {
+ if (!foo)
+ return null;
+ if (s == "N/A")
+ return null;
+ return s;
+ }
+
+ public IEnumerable<int> GetOffsets () {
+ if (!foo)
+ return null;
+ //store.LoadOffsets ();
+ return GetOffsets ();
+ }
+
+ public ArrayList Collection () {
+ if (!foo)
+ return null;
+ var list = new ArrayList ();
+ list.Add (4);
+ list.Add (8);
+ return list;
+ }
+
+ public List<int> GenericCollection () {
+ if (!foo)
+ return null;
+ var list = new List<int> ();
+ list.Add (4);
+ list.Add (8);
+ return list;
+ }
+ }
+
+ [Test]
+ public void DoesNotApply ()
+ {
+ AssertRuleDoesNotApply (SimpleMethods.EmptyMethod);
+ AssertRuleDoesNotApply<DoesNotApplyCases> ("Private");
+ AssertRuleDoesNotApply<DoesNotApplyCases> ("Void");
+ AssertRuleDoesNotApply<DoesNotApplyCases> ("Int");
+ AssertRuleDoesNotApply<DoesNotApplyCases> ("Type");
+ }
+
+ [Test]
+ public void Success ()
+ {
+ AssertRuleSuccess<SuccessCases> ("IntArray");
+ AssertRuleSuccess<SuccessCases> ("String");
+ AssertRuleSuccess<SuccessCases> ("String2");
+ AssertRuleSuccess<SuccessCases> ("String3");
+ AssertRuleSuccess<SuccessCases> ("GetOffsets");
+ AssertRuleSuccess<SuccessCases> ("Collection");
+ AssertRuleSuccess<SuccessCases> ("GenericCollection");
+ }
+
+ [Test]
+ public void Failure ()
+ {
+ AssertRuleFailure<FailureCases> ("IntArray", 1);
+ AssertRuleFailure<FailureCases> ("String", 1);
+ AssertRuleFailure<FailureCases> ("String2", 1);
+ AssertRuleFailure<FailureCases> ("String3", 2);
+ AssertRuleFailure<FailureCases> ("GetOffsets", 1);
+ AssertRuleFailure<FailureCases> ("Collection", 1);
+ AssertRuleFailure<FailureCases> ("GenericCollection", 1);
+ }
+ }
+}
--
1.6.0.3
