Am I handling "spot the missing DLL" right?

I have a .net 4.5 desktop script which has dependencies on some third party libraries (dll's). These libraries are used in my MainWindowViewModel. I need to check that these dll's are present before their absence allow the program to break (which is basically at the starting point of the code). This is how I am handling it at the moment: wrapping the code which fires up the MainWindowViewModel's constructor inside a try/catch. Is this the right way to do this?

In my App.xaml.cs

protected override void OnStartup(StartupEventArgs e)
    {
        base.OnStartup(e);

        MainWindow window = new MainWindow();

        //Create the ViewModel to which the main window binds.
        try
        {
            var viewModel = new MainWindowViewModel();
            // When the ViewModel asks to be closed, 
            // close the window.
            EventHandler handler = null;
            handler = delegate
            {
                viewModel.RequestClose -= handler;
                window.Close();
            };
            viewModel.RequestClose += handler;

            window.DataContext = viewModel;
            window.Show();
        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.Message);
            window.Close();
        }
    }

If the dll is missing, running the program causes a message box to pop up (as you see in the code), and tells the user:


Could not load file or assembly 'xxx.net.xxxx, version = x.x.xx.x,
culture ... or one of the dependencies. The system cannot find the
file specified.

I would like to have a better message rather than the whole exception body in the message box. Maybe some dedicated method could return the dll name, version, etc. separately rather than me trying to clip it out of the exception.

As I mentioned in the comments, the code you've posted could very well be doing anything that could potentially throw any exception.

MainWindow window = new MainWindow();

By putting this constructor call outside the try block, any exception thrown in the MainWindow constructor will be unhandled and the program will terminate in a not-so-gracious way. I assume the constructor does nothing special, but your OnStartup override is also making that assumption; I'd move the call inside the try block.

var viewModel = new MainWindowViewModel();

Your usage of var is inconsistent - I'd declare the MainWindow with var as well, but you could also declare the MainWindowViewModel instance with an explicit type; all that matters is consistency.

The fact that the ViewModel's constructor isn't taking any parameters tells me that the MainWindowViewModel class is likely to be tightly coupled with whatever dependencies it might have; the ViewModel could instead be constructor-injected with some service whose sole purpose would be to perform the 3rd-party DLL late binding - if that service has its own dependencies, for example a Log4net logger, it can receive it in its own constructor:

var logger = LogManager.GetLogger(typeof(LateBindingService))
var service = new LateBindingService(logger);
var viewModel = new MainWindowViewModel(service);

That would make the intent of your code much clearer, and would keep the ViewModel class more focused on being a ViewModel.

Then if you want to use an IoC container to automatically resolve and inject dependencies, you'll have absolutely nothing to change!

I like that you are holding a reference to your handler so it can be unregistered, but I find it would be cleaner to just register a private method instead of a delegate:

var view = new MainWindow();

var logger = LogManager.GetLogger(typeof(LateBindingService))
var service = new LateBindingService(logger);
var viewModel = new MainWindowViewModel(service);

viewModel.RequestClose += ViewModel_RequestClose;
view.DataContext = viewModel;
view.Show();

And that would be the whole try block.

The LateBindingService would be responsible for knowing everything there is to know about the 3rd party DLL, and would expose a method to try loading it.

The catch block should catch a much more specific exception type than System.Exception, since you want to show that message box if the late binding fails, not if anything else goes wrong at one point or another in the View's lifetime.

I'd probably have the LateBindingService throw some LateBindingException which exposes properties for everything there is to know about the missing assembly; the Message property would be 100% under your control, and then the catch block could be like this:

catch(LateBindingException exception)
{
    MessageBox.Show(exception.Message);
    viewModel.OnRequestClose();
}

Notice that I'm not calling Close directly on the view, because I want the handler to unregister itself before exiting, so I'd just have the ViewModel expose a method that raises the RequestClose event - convention for such methods is On[EventName], so OnRequestClose.

StackExchange Code Review Q#54115, answer score: 8