How to refactor this?

Question

Hi,

I'm struggling with a small issue with regard to how I go about refactoring this to a decent pattern.

public class DocumentLibrary
{
    private IFileSystem fileSystem;
    private IDocumentLibraryUser user;

    public DocumentLibrary(IDocumentLibraryUser user) : this(user, FileSystemFrom(user)) { }

    public DocumentLibrary(IDocumentLibraryUser user, IFileSystem fileSystem)
    {
        this.user = user;
        this.fileSystem = fileSystem;
    }

    public void Create(IWorkerDocument document)
    {
        document.SaveTo(fileSystem);
    }

    public IWorkerDocument AttemptContractRetrieval()
    {
        return new Contract(fileSystem, user);
    }

    public IWorkerDocument AttemptAssignmentRetrieval()
    {
        return new Assignment(fileSystem, user);
    }

    private static IFileSystem FileSystemFrom(IDocumentLibraryUser user)
    {
        var userLibraryDirectory = new DirectoryInfo("/DocLib/" + EnvironmentName() + "/" + user.Id);
        return new FileSystem(userLibraryDirectory);
    }

    private static string EnvironmentName()
    {
        using (var edmxContext = new Entities())
        {
            return (from setting in edmxContext.EnvironmentSettings
                         where setting.Name == "EnvironmentName"
                         select setting.Value).First(); 
        }
    }
}

I have two types of worker documents, but I can't seem to easily refactor the two methods above (AttemptContractRetrieval and AttemptAssignmentRetrieval) to a decent form.

Any help would be much appreciated.

Regards, Jim.

Answer 1

Personnally, I would consider either a factory pattern using factory methods or a builder pattern.

Good use of the factory pattern can be seen in the Enterprise Library solution e.g: Database.CreateDatabase();
I would say this would be the most straight forward to integrate.

If you chose the Builder pattern, with a requirement to create more complex objects, then you can separate out the creation of complex objects into a series of build commands e.g: vehicleBuilder.BuildFrame(); vehicleBuilder.BuildEngine(); vehicleBuilder.BuildWheels(); vehicleBuilder.BuildDoors();

Then within these methods, given your chosen implementation, you can add your complexity but make the method calls and construction quite straight forward.

If you haven't come across it, http://www.dofactory.com is a good place to go.

Answer 2

what about this:

  public IWorkerDocument AttemptRetrieval<T>() where T:new, IWorkerDocument
     {
         return new T {FileSystem=fileSystem,User=user}
     }

Out of the top of my head, so may contain a blatant error ;-)

Answer 3

I can see two aspects to this:

1. What do I need to do to add a new IWorkerDocument class? Adding new methods seems heavyweight.
2. What code does the caller need in order to create an IWorkerDocument? Right now the responsibility for calling the correct method lies with the caller, hence it's quite likely that the caller also needs to change each time there is a new IWorkerDocument implementor.

The extent of possible refactoring very much depends upon the answer to 2. Sometimes the caller just has to know what they're making, and in which case the code you have is pretty much all you can do. In other cases you have some "WorkerDefinition" stuff, perhaps in the form of a set of Properties, or a name that can be looked up in a registry. In which case the caller wants an api of the form

 makeMeAWorker(WorkerDefinition def)

on a Factory. Now the caller has no idea what he's asking for, delegates the whole thing to the factory. So the client's world need not change as you add new Worker types.

The Factory can be made extensible by some form of registration scheme or dynamic configuration scheme. We can inject new types into the factory by many different mechanisms.

Answer 4

I think it depends on what other responsibilities the class has that contains those methods. Design patterns are structural constructs. Here we infer that there is a class

class Retriever
{
  ...
  public IWorkerDocument AttemptContractRetrieval()
  {
  }

  public IWorkerDocument AttemptAssignmentRetrieval()
  {
  }
}

The client code is already deciding whether to call AttemptContractRetrieval(), or AttemptAssignmentRetrieval, so maybe polymorphism is in order.

class ContractRetriever
{
   public IWorkerDocument AttemptRetrieval()
   {
   }
}

class AssignmentRetriever
{
   public IWorkerDocument AttemptRetrieval()
   {
   }
}

You can make an abstract Retriever class and have these as descendents of that. This will force the derived classes to have an AttemptRetrieval() method.

If you execute similar actions on the retrieved documents, you may consider having Contract and Assignment classes instead of ContractRetriever and AssignmentRetriever. Then you can put common actions in their parent.

In short, a lot of the answer here lies in the unstated context of the problem.

Answer 5

Are you looking for the Abstract Factory pattern? The declared intent in 'Design Patterns' is "Provide an interface for creating families of related or dependent objects without specifying their concrete classes."

http://en.wikipedia.org/wiki/Abstract_factory

Answer 6

For interested people, I have gone for a factory method.

public IWorkerDocument AttemptRetrieval<T>() where T : IWorkerDocument
{
    return WorkerDocument.Create<T>(fileSystem, user);
}

calls

public static IWorkerDocument Create<T>(IFileSystem fileSystem, IDocumentLibraryUser user) where T : IWorkerDocument
{
    var documentType = typeof(T);
    if (documentType == typeof(Contract))
        return new Contract(fileSystem, user);
    if (documentType == typeof(Assignment))
        return new Assignment(fileSystem, user);
    throw new Exception("Invalid Document Type");
 }

It's a little messy, so does anyone have any suggestions to clean the actual factory method up?