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

i chose expose new behavior using composition vs. injecting new object consumers code or making consumer provide own implementation of new behavior. did make bad design decision?

i had new requirements said needed implement special behavior in circumstances. chose define new interface, implement new interface in concrete class solely responsible carrying out behavior. finally, in concrete class consumer has reference to, implemented new interface , delegate down class work.

here assumptions working with...

• i haven interface, named ifilemanager allows implementors manage various types of files
• i have factory returns concrete implementation of ifilemanager
• i have 3 implementations of ifilemanager, these (localfilemanager, dfsfilemanager, cloudfilemanager)
• i have new requirements says need manage permissions files being managed cloudfilemanager, behavior managing permissions unique cloudfilemanager

here test led me code 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 shell of design (this basic outline not whole shibang)...

public class userfilerepository {     // consumer of code...     public void createresume()     {         var filemanager = factory.getmanager("cloudmanager");         filemanager.addfile();          // argue should inject concrete implementation          // of iaclmanager repository, not sure agree...         var permissionmanager = filemanager 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 created here...         }          return filemanager;     } }  public interface ifilemanager {     void addfile();     void deletefile(); }  public interface iaclmanager {     void addpermission();     void removepermission(); }  /// <summary> /// class has "special" behavior /// </summary> public class cloudfilemanager : ifilemanager, iaclmanager {     public void addfile() {          // implementation elided...     }      public void deletefile(){          // implementation elided...     }      public void addpermission(){         // delegates real implementation         aclmanager.addpermission();     }      public void removepermission() {         // delegates 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> /// class exists manage permissions  /// files in cloud... /// </summary> public class cloudaclmanager : iaclmanager {     public void addpermission() {          // real implementation elided...     }      public void removepermission() {          // real implementation elided...     } } 

your approach add new behavior saved initialization in grand scheme of things because implemented cloudaclmanager separate cloudfilemanager anyways. disagree things how integrates existing design (which isn't bad)...

what's wrong this?

1. you separated file managers , made use of ifilemanager, didn't same iaclmanager. while have factory create various file managers, automatically made cloudaclmanager iaclmanager of cloudfilemanager. then, what's point of having iaclmanager?
2. to make matters worse, initialize new cloudaclmanager inside of cloudfilemanager every time try acl manager - gave factory responsibilities cloudfilemanager.
3. you have cloudfilemanager implement iaclmanager on top of having property. moved rule permissions unique cloudfilemanager model layer rather business rule layer. results in supporting unnecessary potential of circular referencing between self , property.
4. even if wanted cloudfilemanager delegate permission functionality cloudaclmanager, why mislead other classes thinking cloudfilemanager handles own permission sets? made model class facade.

ok, should instead?

first, named class cloudfilemanager, , rightly because responsibility manage files cloud. permission sets must managed cloud, right cloudfilemanager take on these new responsibilities? answer no.

this not can't have code manage files , code manage permissions in same class. however, make more sense class named more general cloudfilesystemmanager responsibilities not limited files or permissions.

unfortunately, if rename class have negative effect on using class. how still using composition, not changing cloudfilemanager?

my suggestion following:

1. keep iaclmanager , 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 set (i.e ioc, di, simple setters,      // constructor parameter, etc.).     //      // either way can delegate actual iaclmanager/ifilemanager     // implementations. } 

why?

this allow use new behavior minimal impact current code base / functionality without affecting using original code. file management , permission management can coincide (i.e. check permissions before attempting actual file action). it's extensible if need other permission set manager or other type of managers matter.

edit - including asker's clarification questions

if create ifilesystemmanager : ifilemanager, iaclmanager, repository still use filemanagerfactory , return instance of cloudfilesystemmanager?

no, filemanagerfactory should not return filesystemmanager. shell have update use new interfaces/classes. perhaps 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() {     // understand determination of "cloudmanager"     // non-trivial, part doesn't change.     // example, environment = "cloudmanager"     var environment = getenvmgr( ... );      var filemanager = m_filemgrfactory.getmanager(environment);     filemanager.addfile();      // permission stuff - see below } 

as invoking permission stuff done, have couple options:

// can use way of determining "cloud" environment // requires permission stuff done if(environment == "cloudmanager") {     var permissionmanager = m_aclmgrfactory.getmanager(environment);     permissionmanager.addpermission(); } 

or

// assumes if no factory exists environment // no permission stuff needs done var permissionmanager = m_aclmgrfactory.getmanager(environment); if (permissionmanager != null) {     permissionmanager.addpermission(); }