Refactoring - Replace Conditional with Polymorphism

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:

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 refactored the above code.

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.

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.

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);
  }
}