Registering derived classes with reflection, good or evil?

Question

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.

Answer 1

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.

Answer 2

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