// // Gendarme.Rules.Smells.AvoidSwitchStatementsRule class // // Authors: // Néstor Salceda // // (C) 2008 Néstor Salceda // // 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 Mono.Cecil; using Mono.Cecil.Cil; using Gendarme.Framework; using Gendarme.Framework.Rocks; namespace Gendarme.Rules.Smells { // TODO: The text in this rule completely confuses me. Is is saying that // all switch statements should be avoided? How exactly do switch statements // lead to code duplication? As far as I can tell, this isn't true at all: switch // statements don't promote code duplication more than any other construct. // // The only real problem with switch statements I am aware of is that they // are sometimes used to dispatch on types which is a bad idea because // virtual methods are normally better for that. The end of the summary // seems to address this but in a rather confusing way. The best way to // clarify the problem IMO is to talk about the Open/Closed Principle. // // In addition this rule looks like it will fire for every single switch statement // in the assembly. The majority of switch statements are going to be fine // so the summary and solution text need to make it very clear that the code // may be perfectly fine. /// /// This rule checks for the Switch Statements smell. This can /// lead to code duplication, because the same switch could /// be repeated in various places in your program. Also, if /// need to do a little change, you may have to change every switch /// statement. The preferred way to do this is with virtual methods and polymorphism. /// /// /// Bad example: /// /// int balance = 0; /// foreach (Movie movie in movies) { /// switch (movie.GetTypeCode ()) { /// case MovieType.OldMovie: { /// balance += movie.DaysRented * movie.Price / 2; /// break; /// } /// case MovieType.ChildMovie: { /// //its an special bargain !! /// balance += movie.Price; /// break; /// } /// case MovieType.NewMovie: { /// balance += (movie.DaysRented + 1) * movie.Price; /// break: /// } /// } /// } /// /// /// /// Good example: /// /// abstract class Movie { /// abstract int GetPrice (); /// } /// class OldMovie : Movie { /// public override int GetPrice () /// { /// return DaysRented * Price / 2; /// } /// } /// class ChildMovie : Movie { /// public override int GetPrice () /// { /// return movie.Price; /// } /// } /// class NewMovie : Movie { /// public override int GetPrice () /// { /// return (DaysRented + 1) * Price; /// } /// } /// /// int balance = 0; /// foreach (Movie movie in movies) { /// balance += movie.GetPrice () /// } /// /// /// This rule is available since Gendarme 2.4 [Problem ("The problem with switch statements is the duplication. You may find the same switch in several places.")] [Solution ("You should consider polymorphism.")] public class AvoidSwitchStatementsRule : Rule, IMethodRule { public RuleResult CheckMethod (MethodDefinition method) { if (!method.HasBody) return RuleResult.DoesNotApply; //Perhaps you are checking autogenerated code from a //yield statement if (method.DeclaringType.IsGeneratedCode ()) return RuleResult.DoesNotApply; foreach (Instruction instruction in method.Body.Instructions) { if (instruction.OpCode == OpCodes.Switch) { Runner.Report (method, instruction, Severity.Low, Confidence.Total); return RuleResult.Failure; } //Sometimes the compiler generates a table //driven comparison, there is the code for //handling too. if (instruction.OpCode == OpCodes.Ldsfld) { FieldReference field = (FieldReference) instruction.Operand; if (field.Name.Contains ("switch") && field.IsGeneratedCode ()) { Runner.Report (method, instruction, Severity.Low, Confidence.Total); return RuleResult.Failure; } } } return Runner.CurrentRuleResult; } } }