replace conditional with polymorphism Posted on December 01, 2010 @ 13:09 oop
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 }