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
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27

    foreach (var permission in principal.Permissions)
    {
      if (newGroup != null)
      {
        var assignment = new SPRoleAssignment(newGroup);
        if (assignment != null)
        {
          switch (permission.RoleDefinition)
          {
            case PermissionTypes.FullControl:
              assignment.RoleDefinitionBindings.Add(fullControlRole);
            break;
            case PermissionTypes.Design:
              assignment.RoleDefinitionBindings.Add(designRole);
            break;
            case PermissionTypes.Read:
              assignment.RoleDefinitionBindings.Add(readRole);
            break;
            case PermissionTypes.Contribute:
              assignment.RoleDefinitionBindings.Add(contributeRole);
            break;
          }

          web.RoleAssignments.Add(assignment);
        }
      }
    }

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
2
3
4
5

    foreach (var permission in principal.Permissions)
    {
      var assignment = permission.CreateFor(newGroup);
      web.AddRoleAssignments(assignment);
    }

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
2
3
4
5
6
7
8
9
10
11
12
13
14
15

    public class FullControllRole : RoleDefinition
    {
      public void AddRolesTo(IList<RoleDefinitionBindings> roleDefinitionBindings)
      {
        roleDefinitionBindings.Add(fullControlRole /* or this */);
      }
    }

    public class DesignRole : RoleDefinition
    {
      public void AddRolesTo(IList<RoleDefinitionBindings> roleDefinitionBindings)
      {
        roleDefinitionBindings.Add(designRole);
      }
    }

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
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31

    public class Permission
    {
      RoleDefinition role;

      public Permission(RoleDefinition role)
      {
        this.role = role;
      }

      public SPRoleAssignment CreateFor(string group)
      {
        var assignment = new SPRoleAssignment(group);
        role.AddRolesTo(assignment.RoleDefinitionBindings);
        return assignment;
      }
    }

    public class SPRoleAssignment
    {
      public SPRoleAssignment(string group){}
    }

    public class Web
    {
      IList<SPRoleAssignment> RoleAssignments;

      public void AddRoleAssignments(SPRoleAssignment assignment)
      {   
        RoleAssignments.Add(assignment);
      }
    }

additional help