Блог

Мне интересно, правильно ли я обрабатываю исключения в своем коде, поэтому я надеялся, что кто-нибудь даст мне несколько мыслей о моем коде ниже

  public IEnumerable<Job> GetAll() {
        try {
            return this._context.Jobs;
        } catch (SqlException ex) {
            //error so dispose of context
            this.Dispose();
            //wrap and rethrow back to caller
            throw new CentralRepositoryException("Error getting all jobs", ex);
        }

    }
  

этот метод является частью моей бизнес-логики и вызывает метод, описанный выше

     public IEnumerable<Job> GetAllJobs() {
        try {
            return this._jobsRepository.GetAll();
        } catch (CentralRepositoryException ex) {
            //logging code to go here
            //throw back to caller
            throw;
        } catch (Exception ex) {
            this._jobsRepository.Dispose();
            //logging code to go here
            //throw simple exception to caller
            throw new CentralRepositoryException("A general exception has occurred");
        }
    }
  

мое пользовательское исключение

 public class CentralRepositoryException : System.Exception {
    public CentralRepositoryException(string message, Exception innerException)
        : base(message, innerException) {
    }

    public CentralRepositoryException(string message){
    }
}
  

С этим подходом связано несколько проблем:

• Возврат IEnumerable<T> приведет к задержке выполнения кода. Даже если вы вернетесь IEnumerable<T> , в коде используйте this._jobsRepository.GetAll().ToList();
• Отложенное выполнение приводит к тому, что обработчик ошибок вообще не вызывается, поскольку код будет выполняться с задержкой и в контексте клиента (т. Е. Везде, Где вы его используете)
• Оберните все ваши IDisposable объекты в using блок

Таким образом, альтернативой было бы:

 public IEnumerable<Job> GetAllJobs() {
    try {
        using(var jobsRepository = new JobsRepository()) // !!! Use Dependency Injection, etc
        {
              return jobsRepository .GetAll().ToList(); // !! NOTE: ToList() avoids delayed execution

        }
    } catch (CentralRepositoryException ex) {
        //logging code to go here
        //throw back to caller
        throw;
    } catch (Exception ex) {
        //logging code to go here
        //throw simple exception to caller
        throw new CentralRepositoryException("A general exception has occurred", ex); // !!!!!! INCLUDE THE ORIGINAL ERROR !!!!!!!
    }
}
  

Вы не должны просто повторно создавать исключение при потере трассировки стека. Я вижу, что вы используете их для удаления объектов, это должно быть в блоке finally.

Если вы действительно не можете обработать исключение, вам не следует использовать catch, поскольку вы хотите, чтобы он всплывал в стеке вызовов, чтобы его можно было исправить, а не скрыть.

Это неправильное использование Dispose() . Если вы заметили, что в конечном итоге вы пишете:

 this.Dispose(); 
this._jobsRepository.Dispose(); 
  

Оба они ссылаются на один и тот же объект. Чтобы гарантировать, что вы удаляете только один раз, это возможность использования класса, объявляющего IDisposable для вызова dispose .

Это означает, что если вы создаете локальную переменную, вы делаете это в инструкции using:

 using(SomethingDisposable foo = new SomethingDisposable())
{
    //...
}
  

или явно утилизировать:

 SomethingDisposable foo = new SomethingDisposable();
try
{
    //...
}
finally
{
    ((IDisposable)foo).Dispose();
}
  

Если вы создаете поле, вы также делаете свой класс одноразовым:

 class MyDisposable : IDisposable
{
    private SomethingDisposable foo = new SomethingDisposable();

    void IDisposable.Dispose()
    {
        foo.Dispose();
    }
}
  

Если вы будете обращаться с вашими IDisposables таким образом, то ваша обработка исключений не будет путаться с вашим удалением.