3
\$\begingroup\$

Following is a code snippet from MVP Win Forms application.

My DAL doesn't handle exceptions and it will be propagated up to the calling method in the presenter classes where the exception will be handled.

I'm using a single handler called ExecutAction(Action action) so I'm catching exceptions in one place rather than repeating it in every method.

At the moment, I'm not logging errors just alert the user for an action, trying to keep the system alive.

When showing messages to users, Presenters will use static class MessagingService's ShowErrorMessage(). So that I can customize all massage boxes in one place.

    class BankAccountPresenter : BasePresenter
    {
        IBankAccount _Model;
        IBankAccountView _View;
        IDataService _DataService;

        public BankAccountPresenter(IBankAccount model, IBankAccountView view, IDataService dataService)
        {
            _DataService= dataService;
            _Model = model;
            _View = view;

            this.SetViewPropertiesFromModel(_Model, _View);

            this.WireUpViewEvents();
        }


        private void WireUpViewEvents()
        {
            this._view.Search += new EventHandler(_view_Search);
        }

        private void _view_Search(object sender, EventArgs e)
        {
            this.Search();
        }

        private void Search()
        {
            ExecutAction(() =>
            {
                var info = _DataService.GetByACNo(_model.AccountNumber);

                if (info != null)
                {
                    _Model = info ;
                    this.SetViewPropertiesFromModel(_Model, _View);
                }
                else
                {
                    MessageBox.Show ("Bank account not found");
                }
            });
        }



        private void ExecutAction(Action action)
        {
            try
            {
                action();
            }

            catch (NullReferenceException e) { MessagingService.ShowErrorMessage(e.Message); }
            catch (System.Data.SqlTypes.SqlTypeException e) { MessagingService.ShowErrorMessage(e.Message); }
            catch (System.Data.SqlClient.SqlException e) { MessagingService.ShowErrorMessage(e.Message); }
        }
    }

Could you please review the above code and let me know whether this approach is good?

Also could you show me a better way to handle showing user messages than using a static?

Does use of lambda statements in every method (ExecutAction(() =>) degrade code readability? or is it a good practice keep things in one place in this way?

\$\endgroup\$
  • 1
    \$\begingroup\$ Consider looking into an aspect-oriented programming approach to exception handling. Castle's dynamic proxy is one lightweight library for this \$\endgroup\$ – Ben Aaronson Jun 14 '14 at 13:00
  • 1
    \$\begingroup\$ I think this is a big improvement over your previous questions. You're no longer catching meaningful exceptions and replacing them with an incorrect stacktrace and vague exception message. You're handling them in the correct place. You're not catching Exception. The only thing I see is that you're creating a method named ExecuteAction that catches sql related exceptions. What if you have to catch some type of System.IO exceptions in the same presenter. Do you use the same ExecuteAction for that? \$\endgroup\$ – Smith.h.Neil Jun 16 '14 at 19:23
  • \$\begingroup\$ @Smith.h.Neil, Could you please show me how to keep the correct Stack trace? If I add general exception handler too (exception e), would be ok? Don't you recommend using a single ExecuteAction()? \$\endgroup\$ – CAD Jun 16 '14 at 19:40
  • 1
    \$\begingroup\$ You are now. When you caught Exception only to do throw new Exception("meaningless message"); you would see the exception originated from where you threw instead of the original exception you caught. You would want the actual exception thrown, not the new one you created and threw. It's usually considered bad to catch Exception. I'll write up an answer in a while about the single ExecuteAction. \$\endgroup\$ – Smith.h.Neil Jun 16 '14 at 19:46
1
\$\begingroup\$

To avoid a discussion in comments I'll go ahead and make my comments an answer.

I think this a much better than your original way. Instead of catching Exception in your DAL, you're allowing all exceptions to propagate up until you can properly handle them. Catching Exception can be a bad thing and is generally considered a bad practice. The problem is that whenever a critical exception such as OutOfMemoryException is thrown, you swallow it and throw a new exception with a message of "Bank account is empty" if I remember correctly. It's just not right because you were hiding what actually went wrong. Your stacktrace would show the exception being thrown from the wrong place and it wouldn't have a meaningful message.

But that was the past and this is the present. You're handling the exceptions where they should be handled and you're not catching Exception. Those were my only real concerns.

The only thing I don't really like (note it's not actually a problem with your example) is that you handle all exceptions in one method. If one method in your presenter does file IO and another does sql stuff - you'd have to include catches for both types of exceptions in one place. What if your presenter is big and can possibly catch dozens of different exceptions? Is this a problem for you? I don't think so because all you're really doing is displaying the exception message through your MessagingService. I would only see it as a problem if you had to uniquely react to exceptions.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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