should new behavior be introduced via composition or some other means?

I chose to expose some new behavior using composition vs. injecting a new object into my consumers code OR making the consumer provide their own implementation of this new behavior. Did I make a bad design decision?

I had new requirements that said that I needed to implement some special behavior in only certain circumstances. I chose to define a new interface, implement the new interface in a concrete class that was solely responsible for carrying out the behavior. Finally, in the concrete class that the consumer has a reference to, I implemented the new interface and delegate down to the class that does the work.

Here are the assumptions that I was working with...

  • I haven an interface, named IFileManager that allows implementors to manage various types of files
  • I have a factory that returns a concrete implementation of IFileManager
  • I have 3 implementations of IFileManager, these are (LocalFileManager, DfsFileManager, CloudFileManager)
  • I have a new requirements that says that I need to manage permissions for only the files being managed by the CloudFileManager, so the behavior for managing permissions is unique to the CloudFileManager

Here is the test that led me to the code that I wrote...

[TestFixture]
public class UserFilesRepositoryTest
{
    public interface ITestDouble : IFileManager, IAclManager { }

    [Test]
    public void CreateResume_AddsPermission()
    {
        factory.Stub(it => it.GetManager("cloudManager")).Return(testDouble);

        repository.CreateResume();

        testDouble.AssertWasCalled(it => it.AddPermission());
    }

    [SetUp]
    public void Setup()
    {
        testDouble = MockRepository.GenerateStub<ITestDouble>();
        factory = MockRepository.GenerateStub<IFileManagerFactory>();
        repository = new UserFileRepository(factory);
    }

    private IFileManagerFactory factory;
    private UserFileRepository repository;
    private ITestDouble testDouble;
}

Here is the shell of my design (this is just the basic outline not the whole shibang)...

public class UserFileRepository
{
    // this is the consumer of my code...
    public void CreateResume()
    {
        var fileManager = factory.GetManager("cloudManager");
        fileManager.AddFile();

        // some would argue that I should inject a concrete implementation 
        // of IAclManager into the repository, I am not sure that I agree...
        var permissionManager = fileManager as IAclManager;
        if (permissionManager != null)
            permissionManager.AddPermission();
        else
            throw new InvalidOperationException();
    }

    public UserFileRepository(IFileManagerFactory factory)
    {
        this.factory = factory;
    }

    private IFileManagerFactory factory;
}

public interface IFileManagerFactory
{
    IFileManager GetManager(string managerName);
}

public class FileManagerFactory : IFileManagerFactory
{
    public IFileManager GetManager(string managerName)
    {
        IFileManager fileManager = null;
        switch (managerName) {
            case "cloudManager":
                fileManager = new CloudFileManager();
                break;
            // other managers would be created here...
        }

        return fileManager;
    }
}

public interface IFileManager
{
    void AddFile();
    void DeleteFile();
}

public interface IAclManager
{
    void AddPermission();
    void RemovePermission();
}

/// <summary>
/// this class has "special" behavior
/// </summary>
public class CloudFileManager : IFileManager, IAclManager
{
    public void AddFile() { 
        // implementation elided...
    }

    public void DeleteFile(){ 
        // implementation elided...
    }

    public void AddPermission(){
        // delegates to the real implementation
        aclManager.AddPermission();
    }

    public void RemovePermission() {
        // delegates to the real implementation
        aclManager.RemovePermission();
    }

    public CloudFileManager(){
        aclManager = new CloudAclManager();
    }

    private IAclManager aclManager;
}

public class LocalFileManager : IFileManager
{
    public void AddFile() { }

    public void DeleteFile() { }
}

public class DfsFileManager : IFileManager
{
    public void AddFile() { }

    public void DeleteFile() { }
}

/// <summary>
/// this class exists to manage permissions 
/// for files in the cloud...
/// </summary>
public class CloudAclManager : IAclManager
{
    public void AddPermission() { 
        // real implementation elided...
    }

    public void RemovePermission() { 
        // real implementation elided...
    }
}

Answers


Your approach to add your new behavior only saved you an initialization in the grand scheme of things because you to implemented CloudAclManager as separate from CloudFileManager anyways. I disagree with some things with how this integrates with your existing design (which isn't bad)...

What's Wrong With This?
  1. You separated your file managers and made use of IFileManager, but you didn't do the same with IAclManager. While you have a factory to create various file managers, you automatically made CloudAclManager the IAclManager of CloudFileManager. So then, what's the point of having IAclManager?
  2. To make matters worse, you initialize a new CloudAclManager inside of CloudFileManager every time you try to get its ACL manager - you just gave factory responsibilities to your CloudFileManager.
  3. You have CloudFileManager implement IAclManager on top of having it as a property. You just moved the rule that permissions are unique to CloudFileManager into your model layer rather than your business rule layer. This also results in supporting the unnecessary potential of circular referencing between self and property.
  4. Even if you wanted CloudFileManager to delegate the permission functionality to CloudAclManager, why mislead other classes into thinking that CloudFileManager handles its own permission sets? You just made your model class look like a facade.
Ok, So What Should I Do Instead?

First, you named your class CloudFileManager, and rightly so because its only responsibility is to manage files for a cloud. Now that permission sets must also be managed for a cloud, is it really right for a CloudFileManager to take on these new responsibilities? The answer is no.

This is not to say that you can't have code to manage files and code to manage permissions in the same class. However, it would then make more sense for the class to be named something more general like CloudFileSystemManager as its responsibilities would not be limited to just files or permissions.

Unfortunately, if you rename your class it would have a negative effect on those currently using your class. So how about still using composition, but not changing CloudFileManager?

My suggestion would be to do the following:

1. Keep your IAclManager and create IFileSystemManager

public interface IFileSystemManager {
    public IAclManager AclManager { get; }
    public IFileManager FileManager { get; }
}

or

public interface IFileSystemManager : IAclManager, IFileManager {

}

2. Create CloudFileSystemManager

public class CloudFileSystemManager : IFileSystemManager {
    // implement  IFileSystemManager
    // 
    // How each manager is set is up to you (i.e IoC, DI, simple setters, 
    // constructor parameter, etc.).
    // 
    // Either way you can just delegate to the actual IAclManager/IFileManager
    // implementations.
}
Why?

This will allow you to use your new behavior with minimal impact to your current code base / functionality without affecting those who are using your original code. File management and permission management can also coincide (i.e. check permissions before attempting an actual file action). It's also extensible if you need any other permission set manager or any other type of managers for that matter.

EDIT - Including asker's clarification questions

If I create IFileSystemManager : IFileManager, IAclManager, would the repository still use the FileManagerFactory and return an instance of CloudFileSystemManager?

No, a FileManagerFactory should not return a FileSystemManager. Your shell would have to update to use the new interfaces/classes. Perhaps something like the following:

private IAclManagerFactory m_aclMgrFactory;
private IFileManagerFactory m_fileMgrFactory;

public UserFileRepository(IAclManagerFactory aclMgrFactory, IFileManagerFactory fileMgrFactory) {
    this.m_aclMgrFactory = aclMgrFactory;
    this.m_fileMgrFactory = fileMgrFactory;
}

public void CreateResume() {
    // I understand that the determination of "cloudManager"
    // is non-trivial, but that part doesn't change. For
    // your example, say environment = "cloudManager"
    var environment = GetEnvMgr( ... );

    var fileManager = m_fileMgrFactory.GetManager(environment);
    fileManager.AddFile();

    // do permission stuff - see below
}

As for invoking permission stuff to be done, you have a couple options:

// can use another way of determining that a "cloud" environment
// requires permission stuff to be done
if(environment == "cloudManager") {
    var permissionManager = m_aclMgrFactory.GetManager(environment);
    permissionManager.AddPermission();
}

or

// assumes that if no factory exists for the environment that
// no permission stuff needs to be done
var permissionManager = m_aclMgrFactory.GetManager(environment);
if (permissionManager != null) {
    permissionManager.AddPermission();
}

Need Your Help

How can I check whether a string is a (very big) number?

c# string validation numbers

I need to make a number validation for specified string. The problem is, the string could be a large number, larger than any numeric type in C# can represent, so I can't use TryParse functions, bec...

Emacs: the code in the body of a defun or defmacro cannot refer to surrounding lexical variables?

emacs lisp elisp emacs24 lexical-scope

Update 2013 May: As of GNU Emacs 24.3.1, (let .. (defun..)) bytecompiles just fine without warning and the bytecompiled code works the same as not-compiled code. Just don't forget to add the file

About UNIX Resources Network

Original, collect and organize Developers related documents, information and materials, contains jQuery, Html, CSS, MySQL, .NET, ASP.NET, SQL, objective-c, iPhone, Ruby on Rails, C, SQL Server, Ruby, Arrays, Regex, ASP.NET MVC, WPF, XML, Ajax, DataBase, and so on.