views:

464

answers:

4

As we all know, when we derive a class and use polymorphism, someone, somewhere needs to know what class to instanciate. We can use factories, a big switch statement, if-else-if, etc. I just learnt from Bill K this is called Dependency Injection.

My Question: Is it good practice to use reflection and attributes as the dependency injection mechanism? That way, the list gets populated dynamically as we add new types.

Here is an example. Please no comment about how loading images can be done other ways, we know.

Suppose we have the following IImageFileFormat interface:

public interface IImageFileFormat
{
   string[] SupportedFormats { get; };
   Image Load(string fileName);
   void Save(Image image, string fileName);
}

Different classes will implement this interface:

[FileFormat]
public class BmpFileFormat : IImageFileFormat { ... }

[FileFormat]
public class JpegFileFormat : IImageFileFormat { ... }

When a file needs to be loaded or saved, a manager needs to iterate through all known loader and call the Load()/Save() from the appropriate instance depending on their SupportedExtensions.

class ImageLoader
{
   public Image Load(string fileName)
   {
      return FindFormat(fileName).Load(fileName);
   }

   public void Save(Image image, string fileName)
   {
      FindFormat(fileName).Save(image, fileName);
   }

   IImageFileFormat FindFormat(string fileName)
   {
      string extension = Path.GetExtension(fileName);
      return formats.First(f => f.SupportedExtensions.Contains(extension));
   }

   private List<IImageFileFormat> formats;
}

I guess the important point here is whether the list of available loader (formats) should be populated by hand or using reflection.

By hand:

public ImageLoader()
{
   formats = new List<IImageFileFormat>();
   formats.Add(new BmpFileFormat());
   formats.Add(new JpegFileFormat());
}

By reflection:

public ImageLoader()
{
   formats = new List<IImageFileFormat>();
   foreach(Type type in Assembly.GetExecutingAssembly().GetTypes())
   {
      if(type.GetCustomAttributes(typeof(FileFormatAttribute), false).Length > 0)
      {
         formats.Add(Activator.CreateInstance(type))
      }
   }
}

I sometimes use the later and it never occured to me that it could be a very bad idea. Yes, adding new classes is easy, but the mechanic registering those same classes is harder to grasp and therefore maintain than a simple coded-by-hand list.

Please discuss.

+3  A: 

My vote is that the reflection method is nicer. With that method, adding a new file format only modifies one part of the code - the place where you define the class to handle the file format. Without reflection, you'll have to remember to modify the other class, the ImageLoader, as well

Claudiu
The only difficulty I see with the approach is that it means redeploying the code, rather than adding additional modules in plugin fashion. Granted, redeployment isn't always bad or difficult - but for the general case, I prefer to avoid it if possible.
Harper Shelby
+4  A: 

My personal preference is neither - when there is a mapping of classes to some arbitrary string, a configuration file is the place to do it IMHO. This way, you never need to modify the code - especially if you use a dynamic loading mechanism to add new dynamic libraries.

In general, I always prefer some method that allows me to write code once as much as possible - both your methods require altering already-written/built/deployed code (since your reflection route makes no provision for adding file format loaders in new DLLs).

Edit by Coincoin:

Reflection approach could be effectively combined with configuration files to locate the implmentations to be injected.

  • The type could be declared explicitely in the config file using canonical names, similar to MSBuild <UsingTask>
  • The config could locate the assemblies, but then we have to inject all matching types, ala Microsoft Visual Studio Packages.
  • Any other mechanism to match a value or set of condition to the needed type.
Harper Shelby
Good point! However, it requires writting stuff at different places (the class and the config). Could we make an hybrid, the assemblies (or directories where assemblies are) in the config, the types get reflected?
Coincoin
And the config is loading the class... "reflectively" isn't? So this leads to a "configurable-reflection" approach, which is still better. Absence of conf file could mean, load them all.
OscarRyz
@Coincoin: good suggestion, I like it!
Harper Shelby
A: 

I know this borders on the "no comment about loading images other ways", but why not just flip your dependencies -- rather than have ImageLoader depend on ImageFileFormats, have each IImageFileFormat depend on an ImageLoader? You'll gain a few things out of this:

  • Each time you add a new IImageFileFormat, you won't need to make any changes anywhere else (and you won't have to use reflection, either)
  • If you take it one step further and abstract ImageLoader, you can mock it in Unit Tests, making testing the concrete implementations of each IImageFileFormat that much easier
Jeremy Frey
I'm not sure I understand your answer. How does the file format gets created in the first place? That thing that creates the ImageFileFormat, how does it knows it exists?
Coincoin
+1  A: 

Isn't this pretty much what the Dependency Injection pattern is all about?

If you can isolate the dependencies then the mechanics will almost certainly be reflection based, but it will be configuration file driven so the messiness of the reflection can be pretty well encapsulated and isolated.

I believe with DI you simply say I need an object of type <interface> with some other parameters, and the DI system returns an object to you that satisfies your conditions.

This goes together with IoC (Inversion of Control) where the object being supplied may need something else, so that other thing is automatically created and installed into your object (being created by DI) before it's returned to the user.

Bill K
Yes, precisely. The question is: Is it ok to use reflection for dependency injection?
Coincoin
Yes, it's virtually required. The trick is to isolate the reflection in a single area dedicated to that alone, not to spread it throughout your code or use it in the location where you actually instantiate the object. Also, there are toolkits to isolate DI for you.
Bill K