1

I noticed a pattern in my code, where im repeating a solid amount of code.

Let's say my application has 2 layers. UI and Services.

I have a FileService class, that has a CopyTo method. Now there are 2 things that can commonly go wrong. First the file already exists in the destination, second the file is already in use by another program.

If that happens, i would like to inform the user of this. So in my ViewModel i have something like this:

try 
   fileService.CopyTo(source, destination);
catch(FileAlreadyExistsException e)
   MessageBox.ShowDialog("File already exists!");
catch(FileInUseException e)
   MessageBox.ShowDialog("File is in use by another program!");
catch(Exception e)
   MessageBox.ShowDialog("Something went wrong, its file system it happens!");

Now any time i want to copy some file in any viewmodel, i need to 1. know about these exception and 2. handle them. Probably in exactly this way in every viewmodel.

I'm using a layered architecture, so i don't want to / can't just put this code in the FileService class.

My first idea is to create a UIService, in the WPF project, that contains the code above. Then my viewmodel calls this UIService and i can reuse the code everywhere.

This seems fine to me from an architectural standpoint. However from a pragmatic standpoint this looks quite silly. I create one Service to do the work, i create a different UIService that calls the first Service and all just to copy a file.

Also either the UIService gets filled with a lot of barely related code, OR i end up with a UI___Service for everyone of my actual Service classes.

I have the same issue with early return type statements. A lot of this kind of code:

if(DocumentDirectoryIsEmpty)
{
   MessageBox.Show("You have to configure the document directory first!");
   return;
}

How do i reuse this type of code properly?

1
  • 1
    You reuse code like this by simply copying it. There isn't sufficient reuse to warrant another abstraction. Oct 2, 2021 at 2:12

3 Answers 3

1

The code snippet above does not need a full-blown UIService class. This can be encapsulated by a simple, straightforward method (for which you need a static class in C# as a container, of course, but without any member variable access, not even static member variables). So regardless in which layer this code appears, just pick a static helper class in the same layer and put the code into a method like

void CopyFileAndHandleErrors(string source, string destination)
{
       //
}

and reuse that method inside the layer whereever you need it. In this specific case, a good place for the method could be a class FileServiceExtensions, where one implements it as an extension method of FileService. In C# this is a common way to extend components outside of the layer where they are coming from.

Your real confusion is probably here:

However from a pragmatic standpoint this looks quite silly. I create one Service to do the work, i create a different UIService that calls the first Service and all just to copy a file.

No, you create a simple method which calls the first Service and adds a specific form of error handling. That's far from being silly, that's the most basic form of reuse by extracting common code into a function, no less, no more.

0

First note that when implementing MVVM, your view model should never care about UI related business. Showing a dialog to the user is UI related business.

Since you are showing a message to the user, I assume the operation e.g. CopyTo is triggered by the user.
In this case the best solution to avoid the exception at all costs. You have several options to do this. You can limit the possible user input by providing only valid options, you can validate the input before using it or show the a (in case of file copy) FolderBrowserDialog.

The dialog is most powerful as it limits the possible options, while each option is valid at least in terms of an existing file path. Every picked path can never result in a FileNotFoundException. Don't worry about the fact, that this dialog is part of the WinForms framework when you are using a different framework like WPF.

Another way is to validate the user input by preferably implementing INotifyErrorInfoChanged (or IDataErrorInfo for WinForms). You then use API members e.g., DirectoryInfo.Exists or FileInfo.Exists to check if the path is valid or a file already exists (see: How to add validation to view model properties or how to implement INotifyDataErrorInfo):

private ValidationResult ValidateDestinationPath()
{
  return Directory.Exists(this.DestinationPath) 
    ? ValidationResuld.ValidResult 
    : new ValidationResult("Path not found", false);
}

In case of a file or folder picker dialog, you should validate the selection immediately before passing the value to the view model - in the view. This allows you to show the dialog again in case the user needs to correct his selection.

These solutions simply prevent exceptions by disallowing invalid data. This is the preferred strategy, as exceptions are generally quite expensive, especially when throwing frequently and should therefore be avoided.

Error prevention before error handling...

If you still need to throw the exception, then respect the best practices (Design Guidelines for Exceptions). Only catch exceptions you can handle and let all others crash your application.
In this context I have to point out that your current try-catch is quite dangerous, a bug magnet:

// This will silence every exception thrown in the current scope
catch(Exception e) {}

As you can also read in the design guidelines, catching Exception will silence exceptions. Showing a message to the user (which he will very likely ignore and try to continue) or logging the exception is not considered handling the exception.

Logged exceptions are lost exceptions, if the app doesn't crash to force the developer to take a look at the log files.
Exceptions are almost always a developer error. Only expected exceptions that can be handled must be caught. Every other exception should crash your application to help you to identify bugs.
Otherwise chances are high that the application continuous to execute, but in an unstable or unpredictable state, causing strange effects that are hard to replicate and to detect.

In this case you only want to handle FileAlreadyExistsException thrown by your FileService. To be able to distinguish the exception source, you should implement a custom exception (this is not necessary when the exception is caught locally), which wraps the original exception as inner exception:

class FileServiceException : Exception
{
  public FileServiceException(string message) : base(message)
  {
  }

  public FileServiceException(string message, Exception innerException) : base(message, innerException)
  {
  }
}

Since you want to show a dialog when this exception was thrown, you must catch it in the view. To do so you can subscribe to Application.Current.DispatcherUnhandledException in WPF or AppDomain.CurrentDomain.UnhandledException in WinForms and WPF:

The view you want to handle the exception by show a dialog to the user:

// WPF
partial class MainWindow : Window
{
  public MainWindow()
  {
    InitializeComponent();

    Application.Current.DispatcherUnhandledException += HandleFileServiceExceptions;
  }

  private void HandleFileServiceExceptions(object sender, DispatcherUnhandledExceptionEventArgs e)
  {
    if (e.Exception is FileServiceException _)
    {
      MessageBox.Show(e.Exception.Message);
    }
  }
}

The service that throws the exception of concern:

// Throws the FileServiceException
class FileService
{
  private void CopyTo(string source, string destination)
  {
    try 
    {
      fileService.CopyTo(source, destination);
    }
    catch(FileAlreadyExistsException e)
    {
      // Wrap original exception as inner exception to preserve the call stack
      throw new FileServiceException("File already exists!", e);
    }
  }
}
4
  • I wouldn't count showing an alert because something went wrong as "UI", as long as you would show the exact same alert, no matter what the real UI looks like.
    – gnasher729
    Sep 6, 2020 at 15:33
  • 1
    What exactly do you mean? Showing something to the user is always an interaction, is always UI. An error raised in the model would never be intended for the user, as those errors are related to internal business logic and not user input errors. " as long as you would show the exact same alert" Errors raised by the model are always internal of origin and intended to notify the developer. A user cannot understand or handle those errors. I am not sure if I understood your point correctly.
    – BionicCode
    Sep 6, 2020 at 15:42
  • You can never prevent all file system exceptions ahead of time. It's a global shared resource. Race conditions and other shit are gonna happen. May 29, 2022 at 21:12
  • @SebastianRedl You can prevent all user input related exceptions. You can prevent most application context related exceptions e.g., by querying system resources. Those other exceptions, that are hardware related, will generate OS level error messages anyway. I was not saying that a developer can prevent all exceptions in general. You can't predict or know all critical states of your machine, 3rd party library and even your framework. But you must always prevent all user input related exceptions. The only source of a potential faulty application state that you can predict.
    – BionicCode
    May 30, 2022 at 8:29
0

For your problem, look at Apples FileManager class. It has all kinds of file system related methods, for example a “CopyTo” (most likely a different name). That method performs the operation and returns success or error.

In addition you can install all kinds of options or delegates before the call. For example if you copy a folder there is a delegate that will figure out if the destination folder exists, ask you whether to cancel, replace the folder, or merge folders, ask which files to use, check if the drive is full etc. etc. etc.

As far as the caller is concerned, a UI is shown but it is absolutely standardised and goes away when the operation is finished. The caller checks for success or failure, that’s it. Of course the call is asynchronous.

That is the easiest way to avoid code duplication for error handling. Instead of calling a low level method and do all kinds of error checking you use or create and use a higher level method that does all the error handling in one place.

PS preflighting has often little chance to succeed, and with this approach it isn’t needed.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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