HiveBrain v1.2.0
Get Started
← Back to all entries
debugcsharpMinor

Catching exception inside model

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
catchingmodelexceptioninside

Problem

Is it normal to catch exception in concept of MVVM inside model? Or how should it be? How to improve this code?

This is simple application that allows user to drop .pfx certificate on the label. LoginModel checks the password for validity and check the publisher. if the validation fails throws an exception that is catching in the LoginViewModel;

Xaml:























Code:

```
namespace WpfApplication31
{
public partial class MainWindow : Window
{
public MainWindow()
{
InitializeComponent();
DataContext = new LoginViewModel();
}
}

public class LoginViewModel : ViewModelBase
{
LoginModel model = new LoginModel();

private string userCertificateResult = "place user certificate";

public string UserCertificateResult
{
get
{
return userCertificateResult;
}

set
{
userCertificateResult = value;
RaisePropertyChanged("UserCertificateResult");
}
}

public RelayCommand CreateConnectionCommand
{
get
{
return new RelayCommand(CreateConnection);
}
}

public RelayCommand DropUserCertificateCommand
{
get
{
return new RelayCommand(CheckUserCertificate);
}
}

private string userPassword = "123456";
public string UserPassword
{
get
{
return userPassword;
}

set
{
userPassword = value;
}
}

private X509Certificate2 UserCertificate;

private void CheckUserCertificate(DragEventArgs args)
{
try

Solution

There is nothing inherently wrong with catching exceptions in viewmodel. The rule of the thumb is that you catch exceptions at level where you can handle them. Sometimes this level is viewmodel level.

That being said, exceptions should not be used to control flow. This topic is covered on the Internet pretty well. For example, check this discussion or this MSDN guideline. Exceptions should be used for handling "exceptional" situations. There is nothing exceptional about certificate having a different issuer from the one you need. On the contrary, I would assume, this should be a pretty common situation.

So, instead of throwing exceptions - return a bool value. Might look like this:

var certificate = model.LoadCertificate(userCertificatePath, UserPassword);
var isValid = model.IsCertificateValid(certificate);


or like this

var isValid  = model.TryLoadCertificate(userCertificatePath, UserPassword, out certificate);


or w/e really, there is plenty of options. Just don't throw exceptions.

Also this smells:

catch (CryptographicException ex)
{
    throw new CryptographicException(ex.Message.ToString());
}


What exactly are you doing it for? Just let the original exception through, don't catch it, if you can't do anything about it.

Code Snippets

var certificate = model.LoadCertificate(userCertificatePath, UserPassword);
var isValid = model.IsCertificateValid(certificate);
var isValid  = model.TryLoadCertificate(userCertificatePath, UserPassword, out certificate);
catch (CryptographicException ex)
{
    throw new CryptographicException(ex.Message.ToString());
}

Context

StackExchange Code Review Q#144769, answer score: 3

Revisions (0)

No revisions yet.