Ответственный программист
0,0
рейтинг
27 августа 2013 в 13:00

Разработка → Защитное программирование перевод

Один из моих читателей, Барри Гайлз, недавно написал мне и задал достаточно интересный вопрос, который, по моему мнению, достоен обсуждения:

«Недавно я столкнулся с одной интересной ситуацией на работе: я производил ревью кода и вставил защитные проверки – одну для проверки аргумента конструктора на null, одну для проверки на null значения, возвращаемого из свойства. У меня также имелись утверждения для закрытых методов, которые я использовал для того, чтобы явно указать мои предположения.
«Похоже, что преобладающей практикой среди моих коллег по команде является опускание проверок и допущение падений. Если быть честным, я борюсь с этой концепцией, так как я уже привык разрабатывать посредством защитного программирования и считал это хорошей практикой. Я практически уверен, что дело обстоит так же в большей части руководств и записей в блогах.
«Вы не могли бы дать совет относительно того, почему лучше программировать в защитном стиле, вместо того, чтобы позволить коду провалиться и затем проверять трассировку стека?»

Вообще говоря, я не думаю, что защитное программирование хуже или лучше другого подхода. Как обычно, на решение проблемы влияют различные факторы, так что краткий ответ будет таков: всё зависит от обстоятельств.
Такой ответ, безусловно, бесполезен до тех пор, пока мы не можем ответить на вопрос: от каких обстоятельств проистекает эта зависимость? В данной статье, я рассмотрю некоторые факторы. Однако, я не собираюсь заявить, что эта статья явиться всеобъемлющим изложением темы.

Позволение коду упасть.

Какова ценность защитного программирования, если всё что вы собираетесь сделать – это немедленно выбросить исключение? Не будет ли таким же хорошим решением просто дать коду упасть? Давайте рассмотрим пример кода.

  public class ProductDetailsController {
            private readonly IUserRepository userRepository;
            private readonly IProductRepository productRepository;
            private readonly ICurrencyExchange exchange;

            public ProductDetailsController(
                IUserRepository userRepository,
                IProductRepository productRepository,
                ICurrencyExchange exchange) {
                this.userRepository = userRepository;
                this.productRepository = productRepository;
                this.exchange = exchange;
            }

            public ProductDetailsViewModel GetProductDetails(string userId, string productId) {
                var user = this.userRepository.Get(userId);
                var targetCurrency = user.PreferredCurrency;

                var product = this.productRepository.Get(productId);
                var convertedPrice =
                    this.exchange.Convert(product.UnitPrice, targetCurrency);

                return new ProductDetailsViewModel {
                                                       Name = product.Name,
                                                       Price = convertedPrice.ToString()
                                                   };
            }
        }

В этом простом классе ProductDetailsController, метод GetProductDetails создаёт экземпляр ProductDetailsViewModel, получая пользователя и информацию о продукте через инъекции репозиториев, конвертируя цену продукта в предпочитаемую пользователем валюту и возвращая данные о продукте, которые предполагается в дальнейшем отобразить на экране. Ради достижения цели статьи, давайте сконцентрируемся только на проблемах проверки на null. Как много вызовов могут пойти по провальному сценарию в методе GetProductDetails? Как много объектов могут быть нулевыми ссылками?
Довольно много, как выясняется. Даже отделённый от своих зависимостей, этот небольшой кусочек кода может выбросить NullReferenceException, как минимум в шести случаях. Представьте, что вы получаете отчёт об ошибках с вашей системы, находящейся в эксплуатации. Трассировка стека указывает на метод GetProductDetails и типом выброшенного исключения является NullReferenceException. Какой из шести возможных объектов с нулевой ссылкой стал причиной ошибки?
Давая системе просто упасть, нам будет трудно ответить на этот вопрос. И не забывайте, что это всего лишь учебный пример. Большая часть эксплуатируемого кода, который я встречал, имел более 5 или 6 строк кода, так что, выяснение причины возникшей ошибки может быстро стать чем-то вроде поиска иголки в стоге сена.
Просто позволить коду упасть – это не очень полезно. Очевидно, если вы пишете очень короткие методы (практика, которую я строго рекомендую), возникающая проблема не так насущна, но чем ваши методы длиннее, тем менее профессионально, на мой взгляд, выражение «просто позволить коду упасть» звучит.

Защитное программирование во спасение?

Посредством добавления явных сторожков на null, вы сможете выбросить более описательные сообщения об ошибке:

public class ProductDetailsController {
            private readonly IUserRepository userRepository;
            private readonly IProductRepository productRepository;
            private readonly ICurrencyExchange exchange;

            public ProductDetailsController(
                IUserRepository userRepository,
                IProductRepository productRepository,
                ICurrencyExchange exchange) {
                if (userRepository == null)
                    throw new ArgumentNullException("userRepository");
                if (productRepository == null)
                    throw new ArgumentNullException("productRepository");
                if (exchange == null)
                    throw new ArgumentNullException("exchange");

                this.userRepository = userRepository;
                this.productRepository = productRepository;
                this.exchange = exchange;
            }

            public ProductDetailsViewModel GetProductDetails(
                string userId,
                string productId) {
                var user = this.userRepository.Get(userId);
                if (user == null)
                    throw new InvalidOperationException("User was null.");
                var targetCurrency = user.PreferredCurrency;

                var product = this.productRepository.Get(productId);
                if (product == null)
                    throw new InvalidOperationException("Product was null.");

                var convertedPrice =
                    this.exchange.Convert(product.UnitPrice, targetCurrency);
                if (convertedPrice == null)
                    throw new InvalidOperationException("Converted price was null.");

                return new ProductDetailsViewModel {
                                                       Name = product.Name,
                                                       Price = convertedPrice.ToString()
                                                   };
            }
        }

Лучше ли этот код? С точки зрения анализа причин возникновения ошибок – да. В этой версии кода, если вы получаете сообщение об ошибке из вашей эксплуатируемой системы, сообщение в исключении подскажет с какой из шести возможных ситуаций с null-ссылкой столкнулась ваша программа. Если бы я находился в режиме поддержки, то я бы знал какую версию из представленных двух я бы предпочёл.
Однако, с точки зрения читаемости, всё сильно ухудшилось. Метод GetProductDetails удлиннился с пяти до одиннадцати строчек кода. Защитное программирование более чем удвоило количество строк! Проход по телу метода тонет в защитных конструкциях, таким образом, метод стал читаться хуже. Если вы типичный программист, то вы читаете код значительно больше, чем пишете его, так что практика, которая делает ваш код более трудночитаемым, должна вызвать в вас ощущение запашка. Нет сомнений в том, что многие программисты считают защитное программирование плохой практикой.

Устойчивость.

Возможно ли сбалансировать факторы, влияющие на проблему и её решение? Да, возможно, но для того, чтобы понять как, вы должны понимать корневую причину проблемы. Первоначальный пример кода не особенно сложен, но даже несмотря на это, присутствует множество случаев, когда этот код провалиться. Что касается нулевых ссылок, то причиной является ошибка в дизайне языка, но в целом, вопрос заключается в том, можете ли вы доверять входным данным или нет. Возвращаемые значения через вызов IUserRepository.Get это (косвенно) тоже входные данные.
В зависимости от того в какой среде функционирует ваша программа, у вас либо есть возможность доверять входным данным, либо у вас такой возможности нет. Представим, на минуту, ситуацию, при которой ваше ПО эксплуатируется в «дикой местности». Вашим ПО может быть повторно используемая библиотека или фреймворк. В таком случае, вы вообще не можете доверять входным данным. Если это так, то, возможно, вы захотите применить принцип устойчивости и оставаться убеждённым в том, что вы действуете в защитном стиле не только относительно входных, но и выходных данных. Другими словами, вы не хотите передавать нулевые ссылки (или другие дьявольские значения) и другому взаимодействующему коду.
Пример кода, приведённый ранее, может передавать нулевые ссылки своим зависимостям, например, если userId = null, или (более изощрённо), если user.PreferredCurrency = null. Таким образом, следуя принципу устойчивости, вы должны были бы добавить ещё больше защитных выражений:

 public class ProductDetailsController {
            private readonly IUserRepository userRepository;
            private readonly IProductRepository productRepository;
            private readonly ICurrencyExchange exchange;

            public ProductDetailsController(
                IUserRepository userRepository,
                IProductRepository productRepository,
                ICurrencyExchange exchange) {
                if (userRepository == null)
                    throw new ArgumentNullException("userRepository");
                if (productRepository == null)
                    throw new ArgumentNullException("productRepository");
                if (exchange == null)
                    throw new ArgumentNullException("exchange");

                this.userRepository = userRepository;
                this.productRepository = productRepository;
                this.exchange = exchange;
            }

            public ProductDetailsViewModel GetProductDetails(
                string userId,
                string productId) {
                if (userId == null)
                    throw new ArgumentNullException("userId");
                if (productId == null)
                    throw new ArgumentNullException("productId");

                var user = this.userRepository.Get(userId);
                if (user == null)
                    throw new InvalidOperationException("User was null.");
                if (user.PreferredCurrency == null)
                    throw new InvalidOperationException("Preferred currency was null.");
                var targetCurrency = user.PreferredCurrency;

                var product = this.productRepository.Get(productId);
                if (product == null)
                    throw new InvalidOperationException("Product was null.");
                if (product.Name == null)
                    throw new InvalidOperationException("Product name was null.");
                if (product.UnitPrice == null)
                    throw new InvalidOperationException("Unit price was null.");

                var convertedPrice =
                    this.exchange.Convert(product.UnitPrice, targetCurrency);
                if (convertedPrice == null)
                    throw new InvalidOperationException("Converted price was null.");

                return new ProductDetailsViewModel {
                                                       Name = product.Name,
                                                       Price = convertedPrice.ToString()
                                                   };
            }
        }

Это, очевидно, выглядит ещё более ужасным, чем предыдущий пример с использованием защитного стиля. Теперь вы защищаете не только себя, но и взаимодействующий код. Похвально, но абсолютно нечитаемо.
До сих пор, когда я пишу код, который живёт в «дикой местности», такое защитное программирование – это именно то, что я делаю, хотя я всё равно склоняюсь к тому, чтобы провести рефакторинг таким образом, что, сначала я бы собрал и проверил все входные данные и уже затем я бы передал эти данные в другой класс, который осуществляет всю логику.

Защищённая местность.

Что если ваш код живёт в защищённой местности? Что если ваш код работает в среде, весь взаимодействующий код является частью одной и той же кодовой базы, написанной вами и вашими коллегами? Если вы можете доверять друг другу, следуя определённым последовательным правилам, вы можете опустить большую часть защитных конструкций.
В большей части команд, в которых я работал, я всегда предполагал, что мы используем принцип устойчивости. На практике, это означает, что null никогда не является допустимым возвращаемым значением. Если член класса возвращает null, то баг находится в этом классе, не в потребителе этого класса. С учётом следования этому правилу, предыдущий код может быть сокращён до такого:

public class ProductDetailsController {
    private readonly IUserRepository userRepository;
    private readonly IProductRepository productRepository;
    private readonly ICurrencyExchange exchange;
 
    public ProductDetailsController(
        IUserRepository userRepository,
        IProductRepository productRepository,
        ICurrencyExchange exchange)
    {
        if (userRepository == null)
            throw new ArgumentNullException("userRepository");
        if (productRepository == null)
            throw new ArgumentNullException("productRepository");
        if (exchange == null)
            throw new ArgumentNullException("exchange"); 

        this.userRepository = userRepository;
        this.productRepository = productRepository;
        this.exchange = exchange;
    } 

    public ProductDetailsViewModel GetProductDetails(
        string userId,
        string productId)
    {
        if (userId == null)
            throw new ArgumentNullException("userId");
        if (productId == null)
            throw new ArgumentNullException("productId");
 
        var user = this.userRepository.Get(userId);
        if (user.PreferredCurrency == null)
            throw new InvalidOperationException("Preferred currency was null.");
        var targetCurrency = user.PreferredCurrency;
 
        var product = this.productRepository.Get(productId);
        if (product.Name == null)
            throw new InvalidOperationException("Product name was null.");
        if (product.UnitPrice == null)
            throw new InvalidOperationException("Unit price was null."); 

        var convertedPrice = 
            this.exchange.Convert(product.UnitPrice, targetCurrency);
 
        return new ProductDetailsViewModel
        {
            Name = product.Name,
            Price = convertedPrice.ToString()
        };
    }
}

Так-то лучше, но ещё не достаточно хорошо… но, подождите: свойства для чтения это тоже возвращемые значения, так что мы и их можем не проверять:

public class ProductDetailsController
{
    private readonly IUserRepository userRepository;
    private readonly IProductRepository productRepository;
    private readonly ICurrencyExchange exchange; 

    public ProductDetailsController(
        IUserRepository userRepository,
        IProductRepository productRepository,
        ICurrencyExchange exchange)
    {
        if (userRepository == null)
            throw new ArgumentNullException("userRepository");
        if (productRepository == null)
            throw new ArgumentNullException("productRepository");
        if (exchange == null)
            throw new ArgumentNullException("exchange");
 
        this.userRepository = userRepository;
        this.productRepository = productRepository;
        this.exchange = exchange;
    }
 
    public ProductDetailsViewModel GetProductDetails(
        string userId,
        string productId)
    {
        if (userId == null)
            throw new ArgumentNullException("userId");
        if (productId == null)
            throw new ArgumentNullException("productId"); 

        var user = this.userRepository.Get(userId);
        var targetCurrency = user.PreferredCurrency;
 
        var product = this.productRepository.Get(productId); 

        var convertedPrice = 
            this.exchange.Convert(product.UnitPrice, targetCurrency);
 
        return new ProductDetailsViewModel
        {
            Name = product.Name,
            Price = convertedPrice.ToString()
        };
    }
}

Вот теперь достаточно хорошо, поскольку теперь мы почти вернулись в исходное состояние кода. Единственной разницей является наличие сторожка в самом начале каждого члена. Когда следуешь принципу устойчивости, большая часть членов начинает выглядеть примерно также. Немного спустя, после того как привыкнете, вы их перестаёте замечать. Я считаю их некой преамбулой для каждого члена. Как читатель кода, вы можете проскакивать сторожки и концентрироваться на потоке логики, без прерывающих вас защитных проверок, мешающих чтению кода.

Инкапсуляция.

Если возвращение null-ссылки является ошибкой, то каким тогда образом класс User, или класс Product, могут следовать принципу устойчивости? Тем же самым путём:

public class User
{
    private string preferredCurrency;

    public User()
    {
        this.preferredCurrency = "DKK";
    } 

    public string PreferredCurrency
    {
        get { return this.preferredCurrency; }
        set
        {
            if (value == null)
                throw new ArgumentNullException("value"); 

            this.preferredCurrency = value;
        }
    }
}

Заметьте, как класс User защищает свои инварианты. Свойство PreferredCurrency никогда не может быть null. Этот принцип также известен под другим названием: инкапсуляция.

Заключение.

Как всегда, помогает понимание факторов, лежащих в основе проблемы или понимание вашей кодовой базы. Вам следует писать значительно больше защитных конструкций в случае, если ваш код живёт в «дикой местности», чем тогда, когда он живёт в защищённой среде. По сей день, я считаю заблуждением веру в то, что вы можете обойтись написанием неряшливого кода; мы все должны быть программистами-рэйнджерами.
Бесструктурный защитный код вредит читаемости. Защитный код – это всего лишь ещё одна отговорка для написания спагетти-кода. Напротив, структурированное защитное программирование являет собой инкапсуляцию. Я знаю, что мне предпочесть.
Перевод: Mark Seemann
Фофанов Илья @EngineerSpock
карма
21,0
рейтинг 0,0
Ответственный программист
Реклама помогает поддерживать и развивать наши сервисы

Подробнее
Спецпроект

Самое читаемое Разработка

Комментарии (72)

  • +7
    Ещё можно купить уже решарпер и разметить код аннотациями. Тогда тот сам будет подсказывать, откуда можно ждать null, и ругаться при попытке передать его не туда, куда нужно.
    • +6
      Можно также воспользоваться CodeContracts. Это средство более мощное.
      Но не стоит забывать, что и ваш способ и способ с CodeContracts — это дополнительные инвестиции в проект.
      • +1
        Решарпер своих денег стоит и окупается за пару месяцев возросшей за счёт него продуктивности.
        • +1
          Я и не говорил про Решарпер. Хотя его цену надо также учитывать. А ещё его эффективность надо доказать. Я его сам использую, купил лицензию. Вы замеряли рост производительности?
          Я, в целом, говорил о том, что и в описанном вами способе и мной появляется дополнительный код. Любой дополнительный код требует сопровождения и сам же является источником потенциальных ошибок (например, ослабление контракта кем-то из коллег).
        • –10
          У вас 2 сообщения подряд сияет посыл «купить решарпер», завязывайте с этим. Это удел кирби.
          • +1
            Не хотите покупать — спиратьте. Мне больно смотреть, как люди делают то, что должна быть занята автоматика.
            • –13
              Если не дошло говорю прямым текстом — хорош рекламировать платные продукты. Тут серьёзные статьи за такое банят и в соотв. хаб направляют. Дошло?
              А что покупать я как и любой другой хабражитель как нибудь без сопливых решим, и то что вам на что либо больно смотреть никак на подобные решения не повлияет — такие излияния в личном блоге пишите.
              • +3
                Простите, а Вы кто такой, собственно, чтобы говорить за хабражителей? Я вот тоже всеми руками и ногами за решарпер, например. И действительно, мнение «сопливых», как Вы выразились, меня не волнует.
          • 0
            Есть еще замечательный IntelliJ IDEA от того же самого JetBrains (есть и бесплатная версия, но не уверен, что оно там есть), для Java — тоже аннотации, которые спасают от ряда ситуаций, описанных в статье, хотя, конечно, не панацея. Плюс к этому можно обработать исходники в post-compile, добавив assert-подобные конструкции — всяко лучше, чем отхватить NPE.
            • 0
              В Eclipse, кстати, тоже есть такие аннотации; но чтобы была польза, их придется расставлять по всему проекту.
            • 0
              Для дотнета есть штука под названием PostSharp, которая встраивается как постсборочный таск и преобразует атрибуты аннотаций в исполняемый код. Помимо стандарных ассертов можно делать свои атрибуты с обработчиками, вызываемыми на входе и на выходе из функции, таким образом привнося в язык аспектно ориентированное программирование. Причём даже вносит правки в отладочные символы. Стоит, правда, как авианосец, так что в боевых проектах пока не пробовал.
              • 0
                Вы о стоимости PostSharp или о стоимости написания своей инфраструктуры для поддержки АОП?
                • 0
                  О стоимости PostSharp. АОП обычно прикручиваю с рантайм-кодогенерацией классов-врапперов. Дёшево, сердито, отладочные символы не портятся, готовые решения есть.
              • 0
                Я так понял, что PostSharp — это что-то вроде KindOfMagic, только с большими возможностями?
                • 0
                  Да, чем-то похоже. Но ради одного INotifyPropertyChanged городить целый post-build таск? У меня для этих целей Reflectiin.Emit-кодогенератор на коленке за пару часов написанный используется сто лет уже.
                  • 0
                    Код на гитхабе/пастебине имеется?
                    • 0
                      Сегодня освобожу от зависимости от инфраструктуры кодогенерации и выложу.
                    • +2
                      github.com/kekekeks/NotifyHelper — выложил тут. А, да. Оно сделано так, что дата биндинги не отваливаются после обфускации — если генератор видит атрибут PropertyName, то не только будет использовать его как имя для события, но сгенерирует в итоговом классе свойство с таким именем, которое в дальнейшем доступно через Reflection.
  • 0
    Представьте, что вы получаете отчёт об ошибках с вашей системы, находящейся в эксплуатации. Трассировка стека указывает на метод GetProductDetails и типом выброшенного исключения является NullReferenceException. Какой из шести возможных объектов с нулевой ссылкой стал причиной ошибки?

    Неужели по номеру строки, с которой вылетело исключение, этого никак нельзя понять?
    • 0
      Я бы не стал всерьёз ориентироваться на номер строки. Если номера строк совпадут — хорошо. А если с последней версии номера строк поменялись, то вам, как минимум, придётся лезть в историю через source control средство.
      • +3
        Не вижу проблемы слазить в историю. Мы же всегда достоверно знаем, что именно где сейчас стоит и как получить еще одну точно такую же сборку, не правда ли?
        • 0
          Хеш-суммы сборок, собранных из одних и тех же исходников имеет привычку различаться. Не думаю, что изменения затрагивают msil, но тем не менее
          • 0
            Если речь идет о Java, можно добавить шаг сборки проекта в дженкинсе, чтобы внутрь артефакта упаковывался текстовый файл с git SHA1, да и номером сборки до кучи.

            UPD: Прошу прощения, уже увидел, что о C#. Мне кажется, для его сборочницы можно тоже что-то подобное сообразить.
      • 0
        У меня вот с Android'a полный callstack приходит, с номерами строк, методами, текущей версией приложения, которое, собственно, крашнулось и прочей дополнительной информацией.
        Неужели на шарпе (desktop || ASP || на_что_оно_еще_делится?) так же нельзя?
    • +1
      Вы поставляете отладочные сборки в комплекте с отладочными символами?
      • +1
        Ну у нас же тут вроде бы управляемый код, всё это идет в комплекте и я не уверен, что отключаемо. Ошибаюсь?
        • +3
          1) сборка может быть отладочной и релизной, отличие релизной в том, что в результате применения оптимизаций строки могут не всегда корректно определяться.
          2) отладочная информация находится в лежащих рядом со сборками pdb(mdb в случае использования компилятора dmcs), которые с приложением обычно не поставляют, впрочем, в случае веб-приложений это не составляет проблем
      • 0
        Кстати, да, вы правы. Совсем об этом забыл)))
      • +2
        Не знаю, как с этим дела обстоят в шарпе, но в плюсовом проекте мы хранили отдельно символы для каждого из билдов, и тогда при крэше определенной сборки можно было просто подключить нужные символы и загнать в отладчик. Сами бинари в пакетах были, разумеется, стрипнутые.
  • 0
    Разве окончательный вариант — не то же самое, что валидация входных данных? Или это другое название одного и того же?
  • 0
    Узнал, что я приверженец защитного программирования :) На новой работе столкнулся с интересной практикой: код должен валиться только в действительно исключительных ситуациях типа невозможности отобразить UI. В подавляющем же большинстве остальных ситуациях, проблемный код должен быть просто проигнорирован, то есть исключения ловятся, но не обрабатываются вообще (sic!) — ни сообщений пользователю, ни записей в лог (на сервере, речь о веб-приложении).
    • –2
      Sic
      • +1
        Э-э-э… Я как-то затрудняюсь даже предположить, зачем вы мне этой ссылкой ответили?
        • –1
          Чтобы показать, как используется это слово. А еще в конце утвердительных предложений ставится точка.
          • +1
            Я именно так как по вашей ссылке написано его и использовал.

            А с пунктуацией у меня всегда проблемы были — последовательность с десяток-другой символов я ещё могу запомнить, но вот на знаки препинания памяти не хватает.
    • +2
      А вот отсутствие записей в логе вам точно аукнется. Типичная ситуация вида «ничего не работает, но ничего не известно». Боретесь как-нибудь с этой ситуацией? Ну, может, стиль кодирования какой-то особенно устойчивый?
      • 0
        Ну, собственно, я и пытаюсь продвинуть позицию «фиг с ним с пользователем, но хоть для нас самих давайте в логи писать об ошибках, обработку (или отсутствие её) которых мы не предусмотрели».
  • +1
    Один приверженец защитного программирования пытался ввести на предприятии стандарт кодирования, где нельзя была написать:
    if (условие)
    {
      код
    }
    

    По его мнению нужно было в обязательном порядке писать так:
    if (условие)
    {
      код
    }
    else
    {
      ;
    }
    

    Даже если else программисту не нужно. Во всех if.
    • 0
      и помогло? (а вдруг..)
    • 0
      Настолько я не заморачиваюсь, но постоянно пишу что-то вроде
      switch ($var) {
          case 'val1':
              // no break;
          case 'val2:
              doSmth();
              break;
          // no default
      }
      
      • +1
        Видел советы делать вот так:

        switch (...) {
            case ... :
                ....
                break;
            case ... :
                ....
                break;
            default:
                throw new UnsupportedOperationException();
        }
        
        • 0
          Не совсем-то. Я о ситуации когда необработка «левых» значений нормально, но нужно как-то в коде показать, что я о них тупо не забыл.
    • 0
    • –1
      Очень полезная, кстати, практика, и защитное програмирование тут не причем.
      А в else лучше писать короткий коммент, объясняющий почему else пустой.
      Исключением могут быть случаи коротких if, заканчивающихся return или throw,
      Например, при проверке аргументов.
      И да, нам помогает. На мой взгляд, это сопоставимо с требованием всегда использовать фигурные скобки после if и else даже если в блоке только одна строка.
      • +5
        Категорически с этим не согласен.
      • +2
        А в else лучше писать короткий коммент, объясняющий почему else пустой.

        if (!isset($a)) {
          $a = 1; // default value
        } else {
          ; // we have value and use it as is
        }
        

        Так что ли? По-моему бредово.
        • +1
          Или так:
          int maxval=int.MinValue;
          foreach(int val in array){
            if(val>maxval){
              maxval=val;  // it is new maximum
            }else{
              // it is not maximum, just skip it
            }
          }
      • +1
        Просмотрел первый попавшийся файл. Из 43 if-ов только в 6 был else, 10 кончались на return, break или continue, а остальные 27 — просто отработка возникшей ситуации, не требующей разбора случая, когда ситуация не возникла. Причём два else (и один if без else) можно убрать, заменив на switch. Так что else останется в 10% случаев. Если каждый из остальных снабдить else, да ещё и с фигурными скобками — разобраться в коде будет заметно сложнее.
  • 0
    В Google Guava есть метод T checkNotNull(T reference), который делает проверку на не null и возвращает сам этот объект. С его помощью такие вот защитные проверки становятся намного компактнее:

    this.userRepository = checkNotNull(userRepository);
    


    Наверняка в C# есть что-то аналогичное
    • +1
      Да, можно сделать такой ThrowIfNull Extension Method
      • 0
        Еще бы стек раскрутить до вызывающего метода
        • 0
          Можно и стек в исключение положить, не проблема.
        • 0
          В смысле?
    • 0
      Подобное можно и самому написать. Вопрос в том, что делать, если всё-таки окажется Null.
  • 0
    Странно, что утверждения упоминаются в открывающей цитате, но не в тексте. В шарпе утверждения, конечно, не самые красивые, но есть. Едва ли имеет смысл использовать if и throw, там где можно обойтись Assert. Собственно, они для сохранения инвариантов и нужны.
  • +2
    Однажды столкнувшись с такой проблемой, мы использовали другой подход. Для необходимых типов данных мы завели пустой объект с необходимой информацией. После этого даже визуально можно было понять что объекта нет, или каких либо его свойств.
    • +4
      Т.е., вы использовали паттерн NullObject?
      • 0
        Возможно, для системы это были полностью рабочие объекты. Очень помогало когда дизайнеры заводят огромное количество объектов а отображения и данных для них еще нет.
  • +1
    Иногда встречаю в чужом коде (и борюсь с желанием использовать в своем) анти-паттерн, проглатывающий ошибки. Что-то вроде такого:

    void Process(Container c, int x)
    {
        if(c == null) return;
        if(x <= 0) return;
    
        c.DoStuff(x);
    }
    

    Писать код в таком стиле подсказывает лень — непонятно, что делать с исключительной ситуацией, и разбираться в причинах ее возникновения тоже лень, поэтому давайте всё обмажем проверками, авось пронесет. В итоге баг неуловимым образом присутствует в программе годами.
    • 0
      А за такое нужно руки с корнем отрывать. Ни исключения, ни сообщения в логе — вообще ничего! И поди пойми, исполнился метод DoStuff() или нет.
      • 0
        Легче станет, если будет
        function Process(Container $c, SplInt $x)
        {
            if (is_null($c)) throw new NullArgumentException();
            if (x <=0) throw new NonPositiveArgumentException();
        
            $c->doStuff($x);
        }
        
        try {
          Process ($c, $x);
        } catch (Exception $e) {
        }
        
        ?
        • 0
          Нет, разумеется, с чего бы? По-моему это достаточно очевидно, что просто выкинуть исключение (без должной его обработки) — недостаточно.
          • 0
            Я считаю, что должно быть достаточно просто выкинуть исключение — необработанное само попадет в логи/stderr в известных мне средах. Но тут не отсутствие обработки, тут явное игнорирование, причем общее, любого исключения.
            • 0
              >Но тут не отсутствие обработки, тут явное игнорирование, причем общее, любого исключения.

              Ну так о том и речь. Кидаешь исключение — либо обрабатывай по-человечески, либо вообще не обрабатывай. А так ваш пример получается еще хуже оригинального: в оригинале хотя бы сразу видно, что кусок кода потенциально проблемный, а в вашем примере на первый взгляд кажется, что все нормально — мол, видите, исключения правильные кидаем!
              • +2
                Сильный аргумент в пользу такого поведения — популярные браузеры. Встречая, не то что не валидный, но даже не велл-формед html или xml код они тупо игнорируют проблемы. Знаете ли хоть один ещё такой популярный софт, как браузеры, заюивающие на синтаксис? Оси популярнее, но они обычно очень трепетно относятся к неправильному обращению с собой, выбрасывая бдосы, сегфолты и прочие классные штуки.
                • 0
                  Отсутствие внешних проявлений != отсутствию каких-либо проявлений. Логгирование все равно очень желательно, хотя бы в дев-версии (а из релизной оно при желании с легкостью вырезается простым #ifdef-ом).
                  • 0
                    Я полностью с вами согласен, правда в наших языках нет препроцессоров. Вот как бы мне коллег убедить?
  • +2
    Во-первых надо отличать вызов метода и конструктор. Дело в том, что конструктор создает объект, который «хранит состояние» между вызовами. Принципиально надо сводить всё к случаям, чтобы программа не могла в какое-то время находиться в несогласованном состоянии. Т.е. если в конструктор передаются ссылки null, а принципиально не может или не должен быть объект с null-cсылками — программа должна не создать объект, а упасть. Т.е. в конструкторе — да, проверки нужны.

    В методе не обязательны. Пусть падает.
    Но там говнокод в другом.
    var product = this.productRepository.Get(productId);
    if (product == null)
       throw new InvalidOperationException("Product was null.");
    

    И как думаете, после каждого вызова метода Get надо такие проверки делать? Может лучше внутри Get поместить эту проверку? В Linq есть методы, которые хорошо называются, например: FirstOrDefault — в названии заключено, что ждать от метода. Это очень плохая практика — взять, назвать метод Get, а потом, ожидать, что из него не только объект, но и null возвращается. Если в каких-то случаях нужен будет метод, который возвратит «пустоту» в случае «не нашел», то лучше для этих случаев писать отдельный метод. Но из метода Get ожидать объект. Договориться как-то с названиями методов. Например, позволять null — метод GetIfExists(). А метод Get (или Single, или как договоритесь), обязательно возвращает.

    И тогда и код чистый и DRY и смысла в коде больше, уверенности больше. Защитное программирование в общем-то не причем. Пусть падает и падает как можно быстрее. Это и мотив — написать проверки в конструкторе. В методах и так упадет, без разрыва по времени. Просто тяжелее искать, что упало. Но поддержка нормальных имен методов и по возможности вообще исключение null из всех мест, где по смыслу не может быть — это ключ к чистоте и надежности кода.
    • 0
      А, извините, невнимателен. В статье в конце о том же. Видимо у меня другая терминология. Не рассматриваю это как защитное программирование, а просто считаю, что null — это враг смысла в коде, который зачем-то всегда подается в коробке с классами и от которого нельзя избавиться. Поэтому по умолчанию у меня его нет. Методы не возвращают, если явно в имени это не указано и в этом явный смысл. Защита — это когда на вход методу подается «что-то не то» и мы защищаемся. Я же думаю — на выходе отдавать надо то, что просят, а если не можешь — кинуть исключение. Проверки только в местах сайд-эффектов — конструкторы или установки полей из методов. В общем, в статье с другой стороны это и описали.
    • 0
      Создавать надо два метода ради того, чтобы в ситуации, когда клиент точно знает, что значение точно вернётся из БД он вызывал Get, а если он не уверен, то GetIfExists, если я вас правильно понял?

      И, если я вас действительно правильно понял, то как часто встречается ситуация, когда клиент точно уверен, что что-то вернётся? Третье чувство подсказывает мне, что таких случаев 1%.
      • +2
        Если Ваш проект — всё приложение, а не библиотека на продажу, то создавать надо только то, что используется. Даже если 1% использует. Если Вы как клиент точно уверены, что Вам нужен объект и он должен быть — вызываете Get. Или даже если не уверены, а подозреваете, что так должно быть. Работа приложения покажет. Надо выбирать более ограниченный вариант.

        Потом, механически, в данном случае именно так и подозревалось: после вызова метода идет проверка на null. Как минимум, если в паре мест встречается такая проверка — DRY требует вынести в один метод. И можно даже писать такой метод (с выбрасыванием эксепшина) для поиска объектов по базе, которых заведомо может не быть. Создать свой эксепшин — NotFoundException. Тогда метод будет выглядеть так:

        Product Get(int productId)
        {
              var product = GetIfExist(productId);
        
              if (product == null)
              {
                    throw new NotFoundException(...);
              }
        
              return product;
        }
        

        Таким образом клиентский код не будет загрязняться проверками. А выброшенное исключение по сути является частью логики ПО, а не ошибкой.

        Вообще, null — это вещь плохая и ненужная. Оно по сути означает: ссылка на объект в куче не инициализирована. А то, что это «объект не найден к базе» — за уши притянуто. Null — это тип в типе. По смыслу — тип — это множество возможных значений. А когда метод может возвращать null, то это говорит, что метод может возвратить одно из двух: [Product | null]. Хотелось бы, чтобы если я подразумевал, что что-то не нашел в базе, например, то я бы сам указывал это в коде явно типом: [Product | NotFound]. А так, получается, что кишки устройства дотнет присутствуют везде в коде и везде ведется борьба в коде с возможными ненужными null.

        Вообще, мой посыл в комментарии был в том, что код должен выражать смысл задачи. Никакого защитного программирования. Если Вы ожидаете от метода что-то, то ровно это одно метод и должен возвращать, а не неявно подсовывать иногда null. Тогда код остается чистым автоматически. К сожалению, от ссылок null не избавиться языковыми средствами шарпа, поэтому иногда приходится в установках полей писать проверки. Не было бы этого уродства в шарпе, не было бы и там проверок. И отлично, надежно проверял бы корректность программы компилятор, а не появлялись ошибки в рантайме.

        А проверять входные параметры, как принято в защитном программировании — не нужно (не обязательно). Если создается объект или в нем меняются поля, то это состояние может сохраняться между вызовами и потом ошибку, кем она внесена, будет тяжело найти. А локальные переменные, в том числе и параметры, не отвечающие требованиям, заставят и так упасть программу сразу же. Проверки захламляют код. Иногда они разве что дают более внятное описание ошибки. Но это не особая проблема. Есть стек. А если бы не было null, так вообще все сообщения об ошибках были более менее читаемыми.
        Защитное программирование хорошо в языках, где ошибка может привести к неопределенному поведению, а не падению. В языках, где программно управляете памятью. Вот там, в С++/С, надо опасаться всего.

        В статье по сути пришли к этому же, что я написал, как бы убрав почти везде защитное программирование, хотя исходили из него вначале.
  • 0
    Эта тема прекрасно описана в книге «Совершенный код». Да и вообще, почему некоторые программисты не хотят её читать? :)

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