Дублирование проверки на null

303

29 июля 2017, 03:22

Один из методов бизнес-логики начинается с кода валидации:

public void Update(ProductDto dto)
{
    if (dto == null)
    {
        throw new ArgumentNullException(nameof(dto));
    }
    var validContext = new ValidationContext(dto);
    Validator.ValidateObject(dto, validContext);     
    //...
}

Если в метод передается null, то я выбрасываю стандартное исключение ArgumentNullException и никакой дополнительной информации как видите не отсылаю.

Мне кажется, что блок проверки на null излишен, потому что в случае dto = null следующее за ним выражение var validContext = new ValidationContext(dto) так и так выбросит то же самое исключение:

Будет ли ругаться "практика правильного дизайна", если я сокращу метод так?

public void Update(ProductDto dto)
{
    var validContext = new ValidationContext(dto);
    Validator.ValidateObject(dto, validContext);  
    //...
}

Answer 1

Да, такое изменение я бы не назвал правильным.

Дело в том, что вы тем самым вносите в ваш метод связность. Ваш код будет правильным лишь в том случае, если

конструктор ValidationContext проверяет входящий объект на null, и бросает исключение, и
у вас до вызова этого конструктора нету другого кода, который использует dto или делает какую-то другую полезную работу.

Каждое из этих предположений нужно будет держать в голове, работая с данным кодом. Причём в коде придётся ещё и оставлять комментарий, который будет объяснять, что null не является валидным входным значением, и почему именно в этом месте нету проверки на null.

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

Чем меньше нужно держать в голове, тем лучше, и тем меньше вероятность ошибки.

Ещё одно мелкое соображение в пользу ранней проверки: если вы видите stack trace упавшего на ArgumentNullException приложения, то ошибка скорее всего находится в методе на один фрейм выше метода, бросившего исключение. В случае «пропуска» плохого аргумента в глубину это соображение не работает.

Answer 2

Внесу краткое дополнение к ответу.

Не сложность заметить, что конструкции вида:

public void Update(ProductDto dto)
{
    if (dto == null)
    {
        throw new ArgumentNullException(nameof(dto));
    }
    //...
}

выглядят довольно громоздко и не эстетично, так и возникает желание избавиться от них. А потому одним из способов добиться этого, может стать использование контрактов (Code Contracts). Например, в случае их использования код выглядел бы так:

public void Update(ProductDto dto)
{
     // Определили Предусловие. Нарушение предусловия говорит о том, 
     // что клиент не прав. 
     Contract.Requires(dto != null);
    //...
}