MVVM light message handling

Question

Recently I started to use mvvm light messenger and it lead to creating this ugliness which I have to refactor now. What would be the best future proof way to refactor this whole class?

public partial class RootView : Window
{


    public RootView()
    {
        try
        {
            InitializeComponent();
            greyOutAdorner = new GreyOutAdorner(MainWindow);
            Messenger.Default.Register<NotificationMessage>(this, NotificationMessageRecived);
            Messenger.Default.Register<string>(this, StringMessageRecived);
            Messenger.Default.Register<string>(this,"Sending DesignID", DesignIdRecived);
            Messenger.Default.Register<DesignInformation>(this, "Sending DesignInfo", DesignInfoReciver);
            Messenger.Default.Register<SpecialDesignInformation>(this, "Sending SpecialDesignInfo", SpecialDesignInfoReciver);


        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.ToString());
        }
    }



    string report;
    string desingName;
    GreyOutAdorner greyOutAdorner;
    DesignInformation designInfo;
    SpecialDesignInformation specDesignInfo;
    DatabaseDesingview databaseDesingview;
    SpecialDesignView specialDesignView;

    private void SpecialDesignInfoReciver(SpecialDesignInformation msg)
    {
        specDesignInfo = msg;
    }

    private void DesignInfoReciver(DesignInformation msg)
    {
        designInfo = msg;
    }

    private void StringMessageRecived(string msg)
    {
        report = msg;
    }

    private void DesignIdRecived(string msg)
    {
        desingName = msg;
    }

    private void NotificationMessageRecived(NotificationMessage msg)
    {


        if (msg.Notification == "Open load from database windows")
        {
            this.AttachAdorner();

            databaseDesingview = new DatabaseDesingview()
            {
                Owner = GetWindow(this),
                ShowInTaskbar = false,
                WindowStartupLocation = WindowStartupLocation.CenterOwner,

            };

            databaseDesingview.ShowDialog();
        }

        if (msg.Notification == "Validation error message")
        {
            MessageBox.Show("Please Correct Validation Errors", "Validation", MessageBoxButton.OK);
        }

        if (msg.Notification == "Design already exist message")
        {
            MessageBoxResult result = MessageBoxResult.Yes;
            result = MessageBox.Show("Design already exist in database would you like to replace it ?", "Replace design", MessageBoxButton.YesNo);
            Messenger.Default.Send(result);
        }

        if (msg.Notification == "Design is not valid message")
        {
            MessageBoxResult result = MessageBoxResult.Yes;
            result = MessageBox.Show("Design is not valid. Are you sure you want to save it?", "Confirmation", MessageBoxButton.YesNo);
            Messenger.Default.Send(result);
        }

        if (msg.Notification == "Window closed")
        {
            this.DettachAdorner();
        }

        if (msg.Notification == "Calculation window closed")
        {

            this.DettachAdorner();
        }

        if (msg.Notification == "Open old calculation report window")
        {
            var calcualtionReportOldView = new CalculationReportOldView(report, databaseDesingview)
            {
                Title = desingName +" " +"Calculation report"
            };
            calcualtionReportOldView.Show();
        }

        if(msg.Notification == "Open old calculation winding instructions window")
        {
            var calcualtionReportOldView = new CalculationReportOldView(report, databaseDesingview)
            {
                Title = desingName +" " +"Winding instructions"
            };
            calcualtionReportOldView.Show();
        }

        if (msg.Notification == "Open dialog old calculation report window")
        {

            var windowsCollecttion = Application.Current.Windows;

            var calcualtionReportOldView = new CalculationReportOldView(report, databaseDesingview)
            {
                Title = desingName + " " + "Calculation report"
            };

            if (windowsCollecttion.Cast<Window>().Any(x => x == specialDesignView))
            {
                specialDesignView.Close();
            }

            this.AttachAdorner();
            calcualtionReportOldView.ShowDialog();
        }

        if (msg.Notification == "Open special design window")
        {
            this.AttachAdorner();

            specialDesignView = new SpecialDesignView(designInfo, specDesignInfo)
            {
                Owner = GetWindow(this),



            };
            designInfo = null;
            specDesignInfo = null;

            specialDesignView.ShowDialog();
        }
    }

    private void AttachAdorner()
    {
        AdornerLayer parentAdorner = AdornerLayer.GetAdornerLayer(MainWindow);

            parentAdorner.Add(greyOutAdorner);


    }

    private void DettachAdorner()
    {
        AdornerLayer parentAdorner = AdornerLayer.GetAdornerLayer(MainWindow);
        parentAdorner.Remove(greyOutAdorner);
    }



}

Answer 1

I have no experience with mvvm light messenger so I don't know how exactly it is intended to be used properly. So here just some general things which sure could use improving:

1. Don't put a blanket catch all into the constructor of your view. If any part in there throws an exception your view is now in an inconsistent state - or even worse you entire application might be in an inconsistent state. At most the caller instantiating the view should catch the exception and handle it, but in most cases the application should just be terminated (logging the exception of course beforehand).

2. Messenger.Default looks like a static singleton instance - this creates an implicit dependency. Better would be to pass the Messenger in as a parameter of the constructor (ideally there would be an IMessenger interface of some sort). This might be tricky sometimes depending on how the view exactly is instantiated.

3. You should be able to use delegates for the message handlers. So instead of this:

   Messenger.Default.Register<string>(this, StringMessageRecived);
   

   you should be able to write this:

   Messenger.Default.Register<string>(this, m => { report = m });
   

   This would get rid of a bunch of small helper functions which just clutter the code.

4. In NotificationMessageRecived string literals are compared. This means they are also being generated somewhere else. Now you have the same string literal at least twice floating around in the code. Ideally they should be turned into enums. but if that is not possible then at least they should be public constants on the NotificationMessage class so you can reference the constant instead of using the actual string literal. This way you have to change it only in one place and the chance for bugs due to typos is greatly reduced.

5. In NotificationMessageRecived the if blocks should be turned into a switch statement. Alternatively you could take a more data driven approach and define a Dictionary<string, Action> which contains the actions to perform for each message. You then would only have to lookup the message as the key and execute the action if it's present.

6. A fair amount of typos are present. The ones I found on a brief glance are:

   • windowsCollecttion -> windowsCollection
   • calcualtionReportOldView -> calculationReportOldView
   • databaseDesingview -> databaseDesignView
   • desingName -> designName