Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

A little background first - I'm working on a server application that requires the ability to use multiple data access class libraries based on the resources available on the server it is installed on (MySQL, SQL Server, XML, etc.). The are currently implemented as modular plugins, where the executable is paired with the appropriate class library on the server and interacts with it through a defined interface (IDataAccess).

The following code is the constructor for a singleton class on the server that exposes the backend:

private IDataAccess mDB;

private MudData()
{
    List<String> files = new List<String>(Directory.GetFiles(".", "*.dll"));
    List<Type> accessors = new List<Type>();

    foreach (String file in files)
    {
        try
        {
            Assembly test = Assembly.UnsafeLoadFrom(file);
            System.Type[] types = test.GetTypes();

            //See if any of the located types implement IDataAccess.
            foreach (System.Type candidate in types)
            {
                if (candidate.GetInterface("IDataAccess") != null)
                {
                    accessors.Add(candidate);
                }
            }
        }
        catch
        {
            //Eat this on purpose - still need to check the rest of the matches.
        }
    }

    if (accessors.Count == 0)
    {
        throw new ApplicationException("No supported data access .dll was found.");
    }

    //TODO: Select from multiples if found, but for now just take the first match.
    PropertyInfo target = accessors.First().GetProperty("Instance");
    MethodInfo getter = target.GetGetMethod();

    //Create a delegate for the reflected assembly so any exceptions propagate correctly.
    Func<IDataAccess> dataDelegate = (Func<IDataAccess>)Delegate.CreateDelegate(typeof(Func<IDataAccess>), null, getter);
    Object found = dataDelegate();
    if (found != null)
    {
        mDB = (IDataAccess)found;
    }
    else
    {
        throw new ApplicationException("Attempt to load an instance of IDataAccess failed.");
    }           
}

Any exceptions at this point will be fatal, and are trapped and logged in the calling code before a (hopefully) graceful exit. This code is only called once at startup and the object it obtains isn't released until the application exits. I've been having a hard time finding information about the performance implications of using an object reference obtained this way, so I'd especially welcome comments from that direction. My assumption is that the main performance penalty is in obtaining the reference, but I haven't been able to find out whether or not that is true in practice.

I'm mainly looking for feedback on my method of connecting with the back-end, but am certainly open to comments/criticism of code style, practices, etc.

Thoughts?

share|improve this question

2 Answers 2

You're loading every DLL in the directory even if you find what you want in the first one.

I'd guess that loading an assembly is what takes more time and resources than looking for what you want in the assembly-after-it's-loaded.

You code will fail if there's a type which implements IDataAccess but which doesn't have an Instance property.

You might want to load into a different AppDomain if you want to unload the assembly which you loaded.

share|improve this answer
    
Yeah, the Instance property is something I've been trying to sort out. I can't add it to the interface because it is meant to be static in the implementation. I'm loading everything currently because the original intent of allowing the targeted library to change at run-time. This has since gone way down on the priority list. Thanks for the heads up on AppDomain. I hadn't looked into it too far but I think you're probably right, especially once I get to the point of allowing run-time changes. –  Comintern Feb 13 at 1:37
    
A slightly better algrithm might be: foreach file { load assembly; foreach type which implements IDataAccess { if type has a public Instance property { hope we found the right instance } else look for another type, else look for another assembly. A more deterministic method might be to use a config file which names the DLL and the desired type within the DLL. –  ChrisW Feb 13 at 1:43
    
That blog post about unloading assemblies is from 2004... before even .NET 2. I wonder if anything has changed that could make unloading assemblies easier or more difficult to implement. –  Bob Feb 13 at 4:10
    
+1 for the AppDomain suggestion –  Lorenzo Dematté Feb 13 at 6:54

Any exceptions at this point will be fatal, and are trapped and logged in the calling code before a (hopefully) graceful exit.

I think you have a use case for a custom exception type here. Throwing System.ApplicationException isn't much better than throwing System.Exception directly. It's actually a relic from the early days of .net; System.ApplicationException was originally intended to be the base class for all custom exceptions (i.e. not thrown directly).

From MSDN:

ApplicationException Class

The exception that is thrown when a non-fatal application error occurs.

Remarks

If you are designing an application that needs to create its own exceptions, you should derive custom exceptions from the Exception class. It was originally thought that custom exceptions should derive from the ApplicationException class; however in practice this has not been found to add significant value.

Thus, I find this:

throw new ApplicationException("No supported data access .dll was found.");

Could be replaced with something like this:

throw new DataAccessLibraryNotFoundException();

And this:

throw new ApplicationException("Attempt to load an instance of IDataAccess failed.");

Could be replaced with something like this:

throw new DataAccessLibraryLoadException();

Also I'm not sure I'd "eat" an exception thrown from the loop. At least keep a trace-level log entry for it, so you have a trace of what the application tried to do. Logging frameworks such as NLog allow you to configure a logger's minimum level without redeploying the app, so your code can log at TRACE level, and then you can deploy and configure the logger's minimum level to, say, DEBUG or INFO, so TRACE entries wouldn't be actually logged. But your code wouldn't be swallowing exceptions.

share|improve this answer
    
Both valid points, thanks. Custom exception types have always been something that I tell myself I'll implement but never get around to doing. –  Comintern Feb 13 at 2:41
    
@Comintern When debugging ASP.NET I can't set it to break into the debugger when an exception is thrown-and-caught. So I have a single MyException class which I use for all exceptions I throw, and I put a break point on the MyException constructor. Thus having even one custom exception type might be more useful than none. –  ChrisW Feb 13 at 12:08

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.