Experienced OO practitioners know that switch statements are a design smell. Usually it’s an indication of missing polymorphic behaviour.

Take a look at the following snippet of code:

 1 foreach (var permission in principal.Permissions)
 2     {
 3       if (newGroup != null)
 4       {
 5         var assignment = new SPRoleAssignment(newGroup);
 6         if (assignment != null)
 7         {
 8           switch (permission.RoleDefinition)
 9           {
10             case PermissionTypes.FullControl:
11               assignment.RoleDefinitionBindings.Add(fullControlRole);
12             break;
13             case PermissionTypes.Design:
14               assignment.RoleDefinitionBindings.Add(designRole);
15             break;
16             case PermissionTypes.Read:
17               assignment.RoleDefinitionBindings.Add(readRole);
18             break;
19             case PermissionTypes.Contribute:
20               assignment.RoleDefinitionBindings.Add(contributeRole);
21             break;
22           }
23 
24           web.RoleAssignments.Add(assignment);
25         }
26       }
27     }

It looks simple enough, doesn’t it? But what happens if we add a new PermissionType? Are we switching on the PermissionType in other areas of the code base?

I think it’s safe to say that the above code violates the Open/Closed Principle as well as the Single Responsibility Principle.

When I look at this code, my mind immediately starts looking for the missing abstraction. In this case I believe it’s an abstraction over different Permission Types.

Here’s how my mind has re-factored the above code.

1 foreach (var permission in principal.Permissions)
2     {
3       var assignment = permission.CreateFor(newGroup);
4       web.AddRoleAssignments(assignment);
5     }

Instead of making RoleDefinition an enum, we can turn it into a class to represent each case in the switch statement. We then just delegate the polymorphic behavior of each RoleDefinition to decide which role should be added to the RoleDefinitionBinding.

 1 public class FullControllRole : RoleDefinition
 2     {
 3       public void AddRolesTo(IList<RoleDefinitionBindings> roleDefinitionBindings)
 4       {
 5         roleDefinitionBindings.Add(fullControlRole /* or this */);
 6       }
 7     }
 8 
 9     public class DesignRole : RoleDefinition
10     {
11       public void AddRolesTo(IList<RoleDefinitionBindings> roleDefinitionBindings)
12       {
13         roleDefinitionBindings.Add(designRole);
14       }
15     }

This also better satisfies the Law of Demeter.

The rest of my mental picture

Please note that the below code was written in a text editor and was not actually run against a compiler.

 1 public class Permission
 2     {
 3       RoleDefinition role;
 4 
 5       public Permission(RoleDefinition role)
 6       {
 7         this.role = role;
 8       }
 9 
10       public SPRoleAssignment CreateFor(string group)
11       {
12         var assignment = new SPRoleAssignment(group);
13         role.AddRolesTo(assignment.RoleDefinitionBindings);
14         return assignment;
15       }
16     }
17 
18     public class SPRoleAssignment
19     {
20       public SPRoleAssignment(string group){}
21     }
22 
23     public class Web
24     {
25       IList<SPRoleAssignment> RoleAssignments;
26 
27       public void AddRoleAssignments(SPRoleAssignment assignment)
28       {   
29         RoleAssignments.Add(assignment);
30       }
31     }

additional help

comments powered by Disqus