понедельник, 17 сентября 2012 г.

Как не стоит писать код


Как не стоит писать код

Авторы: Огинский Евгений Владимирович
Бурда Роман Вадимович
Опубликовано: 20.02.2012
Версия текста: 1.1
Вступление
Есть ли жизнь после написания программы?
Не пишите непонятный код
Не маскируйте ошибочные ситуации
Не оптимизируйте раньше времени
Заключение
Список литературы

Вступление

Правильный код – понятие относительное. Одну и ту же функциональность можно написать по-разному одинаково хорошо. Поэтому довольно сложно обучить написанию правильного кода. На порядок легче показать, как код писать не стоит. В этой статье хочется поговорить о некоторых распространенных ошибках, которые встречаются в коде программных продуктов. При этом преследуется следующая цель – обучить обнаруживать такие ошибки. Статья, в некоторой мере, является развитием работ, опубликованных RSDN ранее [1-2]. Основным достоинством данной статьи является то, что все ошибки излагаются на фрагментах кода из реальных программных продуктов.

Есть ли жизнь после написания программы?

Каким должен быть продукт, чтобы считаться успешным? Ответ “чтобы приносил много денег” рассматривать не будем, поскольку он слишком общий и не подсказывает направление движения. Рассмотрим только то, на что может повлиять программист.
Во-первых, продукт должен быть рабочим. Это значит, что вся функциональность должна работать именно так, как это заявлено. Конечно, если востребованность продукта высока, то люди будут пользоваться программами с “багами”. Но в этом случае конкурентам легче вытеснить продукт с рынка. Зачем, спрашивается, упрощать им жизнь? Во-вторых, скорость работы продукта должна быть приемлемой.
ПРИМЕЧАНИЕ
Скорость работы программы идет вторым пунктом потому, что сделать быстрой корректно работающую программу, как правило, проще, чем быструю – корректной. Вместе с этим, приемлемая скорость работы продукта является одной из важнейших характеристик успешности продукта.
В-третьих, немаловажным фактором является поддерживаемость продукта. Если со временем возникает необходимость добавления новой функциональности, хотелось бы вносить изменения без чрезмерных усилий и без добавления новых ошибок. Иначе эта функциональность обойдется слишком дорого. В результате продукт перестанет развиваться и потеряет позиции на рынке. Давайте сосредоточимся на этих пунктах.

Не пишите непонятный код

Начнем с пункта “рабочий продукт”, поскольку он имеет самый высокий приоритет. Давайте подумаем над вопросом – почему в продукте появляются баги? Причин тому существует огромное количество.
ПРИМЕЧАНИЕ
Авторы этой статьи даже не пробуют претендовать на знание всех этих причин. Рассматриваются наиболее простые и очевидные случаи.
Напомним, что большой программный продукт разрабатывается командой. Любая часть кода может быть дописана, исправлена или изменена любым членом команды. Поэтому важно, чтобы код был простым и понятным. Рассмотрим пример.

Слишком сложный код

В онлайн-магазине необходимо написать функциональность, которая позволяет получать скидку по карточке покупателя. Эта скидка начисляется на заданное количество продуктов (groupSize) и может быть применена несколько раз для одной корзины. Например, если в корзине 5 продуктов, и значение groupSize равняется двум, то скидка будет начислена на две группы продуктов – {первый, второй} и {третий, четвертый}. Пятый продукт не попал ни в одну группу, и поэтому на него скидка не полагается. Количество скидок (или групп), которые применяются для корзины, ограничено числом maximumGroupCount.
При реализации системы скидок появилась возможность удалить не использованные при начислении скидок продукты. А если количество групп больше maximumGroupCount, то удаляет и лишние группы. Так появился следующий код:
public interface IProduct
{
  Guid Id { get; set; }
  string Name { get; set; }
}

public interface IProductHandler
{
  void HandleUnusedProduct(IProduct product);
}

public class ProductsRemover
{
  IProductHandler handler;

  public ProductsRemover(IProductHandler handler)
  {
    this.handler = handler;
  }

  public void RemoveItems(List<IProduct> products, int groupSize, 
    int maximumGroupCount)
  {
    while (products.Count > groupSize * maximumGroupCount)
    {
      handler.HandleUnusedProduct(products[products.Count - 1]);
      products.RemoveAt(products.Count - 1);
    }

    while (products.Count % groupSize != 0)
    {
      handler.HandleUnusedProduct(products[products.Count - 1]);
      products.RemoveAt(products.Count - 1);
    }
  }
}
Функция RemoveItems работает правильно, хотя и выглядит достаточно странно. Действительно, не с первого раза видно, что циклы взаимоисключают друг друга. Также неочевидна функциональность, за которую отвечает каждый цикл. Мы уже не говорим о повторении кода. У человека, который видит это класс впервые, может появиться жуткое желание удалить второй цикл. Даже сам автор функции RemoveItems удалял второй while через некоторое время после написания. Если первый цикл хотя бы раз исполнился, то второй исполняться не будет. Увидеть сценарий, в котором products.Count <= groupSize * maximumGroupCount, и при этом остаток от деления products.Count на groupSize не равняется нулю, сложнее. Учитывая человеческий фактор, можно предположить, что рано или поздно в ходе доработки появятся баги.
Проблемой этого примера является непонятность, усложненность и неочевидность кода. А добавление новой функциональности может привести к еще более трудночитаемому коду. На основании только одного примера тяжело сделать правильные выводы. Но давайте вспомним работы таких классиков, как Фаулер [3] и Макконнелл [4]. Из них следует, что сложность кода повышает вероятность существования в нем багов и появления багов в будущем.
Мы умолчим о том, что дальнейшая работа с этим фрагментом может привести к нервному срыву. Только представьте мысли программиста, которому приходится в пятницу во второй половине дня разбираться в этом коде.
Чтобы предыдущий пример выглядел законченным, приведем более очевидный и простой вариант его реализации:
public class ProductsRemover
{
  IProductHandler handler;

  public ProductsRemover(IProductHandler handler)
  {
    this.handler = handler;
  }

  public void RemoveItems(List<IProduct> products,
    int groupSize, int maximumGroupCount)
  {
    int usedProductCount = GetUsedGroupCount(products.Count, groupSize, 
      maximumGroupCount) * groupSize;

    List<IProduct> productsToRemove = products.GetRange(usedProductCount, 
        products.Count – usedProductCount);

    foreach (IProduct product in productsToRemove)
    {
      handler.HandleUnusedProduct(product);
      products.Remove(product);
    }
  }

  private int GetUsedGroupCount(int productCount, int groupSize, 
    int maximumGroupCount)
  {
    return Math.Min(productCount / groupSize, maximumGroupCount);
  }
}
Конечно, этот пример можно сделать еще лучше. Как говорится, совершенству нет предела. Но мы достигли главной цели – код стал более понятным. Нам приводили аргументы, что качество ухудшилось, ведь выросло количество строчек. На самом деле код должен быть понятным при минимально возможном размере. Это удобно тем, что его можно быстрее прочитать. Если размер кода уменьшается и при этом теряется понятность – это недопустимо.
Приведем сравнение с технической книгой. Если она хорошая – все тезисы излагаются кратко и понятно. В сложных местах есть необходимые объяснения. Плохую книгу сложно понять, и один абзац приходиться перечитывать десятки раз, пропускать непонятные места и потом опять возвращаться к ним.
ПРИМЕЧАНИЕ
Код должен быть понятен каждому члену команды. Аргумент “он мне понятен, этого достаточно”, – неправильный, так как обычно за качество отвечает вся команда, а не отдельный человек. Кроме этого, если код непонятен другому программисту, существует большая вероятность того, что через некоторое время его и не поймет автор.
Давайте продолжим исследовать вопрос понятности кода и выделим наиболее очевидные ошибки, которые ухудшают его понимание.

"Индусский" код

Многие говорять об "индусском" коде, в основном, чтобы посмеяться. Мы хотим подчеркнуть то, что "индусский" код усложняет понимание программы. Рассмотрим классический пример:
public bool IsArrayEmpty(string[] array)
{
  if (array.Length > 0)
    return false;
  else
    return true;
}
К огромному сожалению, такой код встречается слишком часто в наших проектах. Когда мы просили автора его улучшить, в лучшем случае он приобретал следующий вид:
public bool IsArrayEmpty(string[] array)
{
  if (array.Length > 0)
    return false;
 
  return true;
}
Улучшение есть, но очень небольшое – все еще надо анализировать условный оператор. Если вспомнить, что выражение в условном операторе совпадает с возвращаемым типом функции, этот код можно записать следующим образом:
public bool IsArrayEmpty(string[] array)
{
  return array.Length == 0;
}
ПРИМЕЧАНИЕ
Отметим, что в C# 3.0 появилась библиотека Linq to Objeсt, в которой имеется метод-расширение Any(). Не следует “изобретать” функции, которые уже написаны.
В таком варианте функция читается легко и быстро. Часто может получиться так, что и сама функция уже не нужна. Но здесь ее можно оставить, поскольку IsArrayEmpty понимается легче в сравнении с array.Length == 0.
ПРИМЕЧАНИЕ
Повторимся, читабельность кода является единственной причиной такого улучшения. При этом часто, но не всегда, размер кода уменьшается. Но и увеличенный размер не является плохим, если это делает код более понятным.
Перечислить все возможные варианты "индусского" кода невозможно, поскольку человеческой фантазии нет предела (особенно если ее “включают” программисты). Но это не является нашей целью. Главное, что мы хотели показать этим примером – если код непонятен с первого взгляда, то, скорее всего, он нуждается в улучшении.

Код-“гармошка“

Рассмотрим функцию, которая содержит один условный оператор if…else. Есть два возможных сценария выполнения этой функции. При добавлении нових условных операторов добавляются новые сценарии выполнения. Согласно Гради Бучу [5], человек может одновременно воспринимать около 7 единиц информации. Именно поэтому с увеличением количества сценариев сложно помнить все возможные случаи поведения кода. В результате могут появиться ошибки или непредсказуемое поведение программы в определенных сценариях.
Рассмотрим разновидность этой проблемы на примере с большой вложенностью операторов, которую мы называем кодом-“гармошкой” из-за характерного вида блоков с отступами.
public void UpdateBasket(IEnumerable<IBasketLineData> linesData)
{
    IBasket customerBasket = GetCurrentBasket();

    if (customerBasket != null)
    {
        foreach (IBasketLineData lineData in linesData)
        {
            if (!IgnoreLine(lineData.BasketLineId))
            {
                BasketLine line = customerBasket.BasketLines.GetById(
               lineData.BasketLineId);
                if (line != null)
                {
                    if (lineData.Stock != 0)
                        line.Quantity = lineData.Stock;
                    else
                        customerBasket.BasketLines.Remove(line);
                }
            }
        }

        SaveBasket(customerBasket);
    }
}
Этот пример содержит большую вложенность операторов (пять), что усложняет чтение и модификацию кода.
СОВЕТ
Старайтесь использовать не больше 2-3 вложенных операторов в пределах одной функции.
Попробуем улучшить.
public void UpdateBasket(IEnumerable<IBasketLineData> linesData)
{
    IBasket customerBasket = GetCurrentBasket();
    if (customerBasket == null)
        return;

    foreach (IBasketLineData lineData in linesData)
    {
        BasketLine line = GetLine(customerBasket.BasketLines, lineData);

        if (line != null)
        {
            ProcessLine(customerBasket, lineData.Stock, line);
        }
    }

    SaveBasket(customerBasket);
}

private void ProcessLine(IBasket customerBasket, int quantity, BasketLine line)
{
    if (quantity != 0)
        line.Quantity = quantity;
    else
        customerBasket.BasketLines.Remove(line);
}

private BasketLine GetLine(BasketLines basketLines, IBasketLineData lineData)
{
    if (!IgnoreLine(lineData.BasketLineId))
        return null;

    return basketLines.GetById(lineData.BasketLineId);
}
Стало симпатичнее, не так ли? Давайте разберемся в том, что у нас получилось. В каждой функции имеется не больше двух вложенных операторов. Несмотря на то, что общее количество сценариев осталось прежним, они разделены по разным функциям. В нужных местах добавлена необходимая абстракция (например, функции GetLine, ProcessLine). Некоторые сценарии разделены и идут последовательно, например проверка на null переменной customerBasket (Фаулер называет это преобразование заменой вложенных условных операторов граничным оператором [3]). Это облегчает дальнейший рефакторинг. Рекомендуется описывать в первую очередь те сценарии, которые проще. Этот момент носит чисто психологический характер. Если программист видит функцию, которая начинается сложно, у него будет меньше желания разбираться в ней. Привлекайте простотой, по крайней мере, в начале функции.
СОВЕТ
Расположение сценариев в одной функции лучше сортировать по возрастанию их сложности

Излишние комментарии

Иногда программисты стараются написать комментарии для того, чтобы облегчить понимание кода в будущем. Это, в общем-то, неплохо. Но всегда ли применение комментариев оправдано? Часто (но не всегда) желание написать комментарий говорит о том, что код не очень хорошо читается. Возможно, стоит лишь поменять название переменной или класса, упростить условные конструкции, вынести блок кода в отдельную функцию с понятным названием, и комментарии перестанут быть необходимыми. Код должен быть достаточно выразительным и объяснять читателю все намерения автора. Понятный код без комментариев обладает большими преимуществами в сравнении с непонятным кодом и обильными комментариями. Его всё-таки легче читать и поддерживать.
СОВЕТ
Если попадаются блоки кода, которые хочется пояснить при помощи комментариев, то, возможно, стоит улучшить код.
Рассмотрим пример:
// Не отправлено ли уведомление ранее и наступило ли время отправки уведомления.
if (!context.Order.Fields.ContainsKey("OrderMailSend") 
  && (context.Order.PaymentStatus == PaymentStatus.Paid 
      || (context.Order.PaymentStatus == PaymentStatus.InProgress 
        && GetPaymentMethod(context).StartsWith("banktransfer")
     )
   )
)
{
  IEnumerable<string> email = GetEmailAddress(context);
  MailNotification.SendOrderConfirmation(email, context.Order);
  // Отмечаем что уведомление о заказе отправлено
  context.Order.Fields.Add(
    new EntityField { Name = "OrderMailSend", Value = true });
}
Из-за громоздких выражений код дополнительно поясняется комментариями. Они немного упрощают задачу чтения кода. Но при попытке модификации разработчику всё равно придётся вникать и детально разбирать конструкции языка программирования. Можно ли ему чем-то помочь? Давайте попробуем:
if (!OrderMailSent(context) && MailNotificationRequired(context))
{
  IEnumerable<string> email = GetEmailAddress(context);
  MailNotification.SendOrderConfirmation(email, context.Order);
  MarkOrderAsSent(context);
}

private bool MailNotificationRequired(IPaymentContext context)
{
  return OrderPaid(context) || BankTransferInProgress(context);
}

private bool BankTransferInProgress(IPaymentContext context)
{
  return context.Order.PaymentStatus == PaymentStatus.InProgress && 
       GetPaymentMethod(context).StartsWith("banktransfer");
}

private bool OrderPaid(IPaymentContext context)
{
  return context.Order.PaymentStatus == PaymentStatus.Paid;
}

private bool OrderMailSent(IPaymentContext context)
{
  return context.Order.Fields.ContainsKey("OrderMailSend");
}

private void MarkOrderAsSent(IPaymentContext context)
{
  context.Order.Fields.Add(new EntityField { Name = "OrderMailSend", Value = 
    true });
}
После улучшения код хорошо читается при отсутствии комментариев, и при изменении кода вероятность внести ошибку уменьшается.
Другой пример “вредных” комментариев – пояснение очевидных конструкций. В хорошо читаемом коде комментарии дублируют информацию и затрудняют его поддержку. Тому, кто будет просматривать код, нужно прочитать больше текста. При модификации нужно проверять актуальность комментариев и изменять их при необходимости. А это увеличивает потраченное время.
Комментарии стоит писать в тех случаях, когда малопонятный код не получается улучшить. Такое часто случается при взаимодействии с внешними компонентами, у которых есть свои особенности.

Не вводите ненужные состояния

Для реализации сложной логики программы часто вводится состояние. Это делает код сложнее, что увеличивает вероятность появления ошибок. Но что же делать? Существуют практики, которые разрешают писать сложную функциональность почти без состояний (функциональный подход в программировании [6]). Но в этой статье мы хотим обратить внимание на случаи, когда состояние является лишним. Рассмотрим пример.
protected string GetClass(object url)
{
  string result = string.Empty;

  if (SiteMap.CurrentNode != null && SiteMap.CurrentNode.Url == url.ToString())
    result = "class=\"active\"";

  return result;
}
Первое, что хочется спросить – зачем переменной result присваивать значение, которое позже может измениться? А второе – зачем здесь переменная в принципе? Предлагаем удалить переменную result и написать так:
protected string GetClass(object url)
{
  if (SiteMap.CurrentNode != null && SiteMap.CurrentNode.Url == url.ToString())
    return "class=\"active\"";

  return string.Empty;
}
Переменные можно вводить для упрощения чтения кода, но не стоит этого делать только ради улучшения производительности.
ПРИМЕЧАНИЕ
Помните, в начале статьи мы говорили о том, что правильно работающий код имеет приоритет выше, чем быстро работающий код с багами?
Любая оптимизация усложняет понимание кода и подразумевает под собой использование более “хитрого” подхода, который будет работать быстрее. Но он не такой очевидный, как неоптимизированный подход. Рассмотрим следующий пример:
public IList<Website> AllWebsites
{
   get
   {
     var websites = RequestState.GetValue<List<Website>>("Backoffice", 
       "AllWebsites");
     if (websites != null)
      return websites;

     string userName = Membership.GetUser().UserName;

     if (Roles.GetRolesForUser(userName).Contains("Dealer"))
       websites = BOUtils.GetDealerSites(userName).ToList();
     else
       websites = new List<Website>(Framework.Content.GetWebsites());

     RequestState.SetValue<List<Website>>("Backoffice", "AllWebsites", 
       websites);

     return websites;
   }
}
Как видим, здесь есть две переменные: websites и userName. В начале функции переменная websites используется для упрощения чтения кода (ведь в правой части стоит достаточно громоздкая конструкция), а дальше – для возвращаемого значения. Но userName затрудняет дальнейшее улучшение. Поэтому давайте от неё избавимся.
public IList<Website> AllWebsites
{
  get
  {
    var websites = RequestState.GetValue<List<Website>>("Backoffice",
          "AllWebsites");

    if (websites!= null)
     return websites;

    if (Roles.GetRolesForUser(Membership. GetUser().UserName).
      Contains("Dealer"))
    {
      websites = BOUtils.GetDealerSites(
        Membership.GetUser().UserName).ToList();
    }
    else
    {
     websites = new List<Website>(Framework.Content.GetWebsites());
    }

    RequestState.SetValue<List<Website>>(
      "Backoffice", "AllWebsites", websites);

    return websites;
  }
}
Уберем в начале функции переменную websites и сделаем код более читаемым. Например, с помощью дополнительных свойств. Посмотрите, как изменился код:
public IList<Website> AllWebsites
{
  get
  {
    if (CaсhedWebsites != null)
      return CaсhedWebsites;

    List<Website> websites = IsRoleDealer ? DealerWebsites 
      : new List<Website>(Framework.Content.GetWebsites());

    SetCache(websites);

    return websites;
  }
}

private List<Website> DealerWebsites
{
  get
  {
    return BOUtils.GetDealerSites(Membership.GetUser().UserName).ToList();
  }
}

private bool IsRoleDealer
{
  get
  {
    return Roles.GetRolesForUser(Membership.GetUser().UserName).
      Contains("Dealer");
  }
}

private List<Website> CaсhedWebsites
{
  get
  {
    return RequestState.GetValue<List<Website>>("Backoffice", "AllWebsites");
  }
}

private void SetCache(List<Website> websites)
{
  RequestState.SetValue<List<Website>>("Backoffice", "AllWebsites", websites);
}
Части сложного и трудночитаемого кода были вынесены в свойства с понятными именами. Поскольку нам удалось удалить состояние websites в начале функции, получилось отделить сценарий с кэшированными Web-сайтами от остальных. Согласитесь, в предыдущем коде это было видно не сразу. Простым переименованием переменной websites нельзя обойтись, поскольку она использовалась одновременно в двух ролях. Можно было бы добавить еще одну переменную (например, сaсhedWebsites вместе с websites), но это неверный вектор движения, поскольку так код только усложнится.
В принципе, в последнем примере можно было бы удалить переменную websites вообще. Причем сделать это можно разными способами. Мы оставили как есть, потому что не хотим слишком увлекаться. Нас полностью удовлетворяет понятность данного фрагмента кода. Повторимся – совершенству нет предела. Поэтому надо чувствовать, когда пора остановиться.
В случае, когда переменную целесообразно оставить, не используйте её для разных целей (как это было с переменной websites до улучшения). По возможности, переменные должны быть неизменяемыми (значение устанавливается один раз при объявлении). Часто это позволяет сделать код более понятным. Рассмотрим пример:
void CalculateLines()
{
  IEnumerable<IBasketLine> lines = BasketLines.Where(
      item => item.ProductId == productId);

  if (!string.IsNullOrEmpty(variantId))
  {
    if (string.IsNullOrEmpty(prepackCode))
      lines = BasketLines.Where(item => item.ProductId == productId &&
                  item.VariationId == variantId);
    else
      lines = BasketLines.Where(item => item.ProductId == productId && 
                    item.VariationId == variantId && 
                    item.PrepackCode == prepackCode);
  }
...
}
Здесь переменная lines инициализируется при объявлении, но потом её значение может измениться. Давайте подумаем, насколько такой подход оправдан. Есть вероятность того, что в ходе модификации функции переменная lines будет использована раньше окончательной инициализации. Это приведет к ошибке. Небольшое изменение позволит устранить этот недостаток:
void CalculateLines()
{
  IEnumerable<IBasketLine> lines = GetLines();
  ...
}

private IEnumerable<IBasketLine> GetLines()
{
  if (!string.IsNullOrEmpty(variantId))
  {
    if (string.IsNullOrEmpty(prepackCode))
      return BasketLines.Where(item => item.ProductId == productId &&
                   item.VariationId == variantId);
    else
      return BasketLines.Where(item => item.ProductId == productId &&
                   item.VariationId == variantId && 
                   item.PrepackCode == prepackCode);
  }

  return BasketLines.Where(item => item.ProductId == productId);
}
Следующим шагом к улучшению данного фрагмента может стать удаление дублирования кода в функции GetLines. Но поскольку это уже другая проблема, мы этого делать не будем.

Не маскируйте ошибочные ситуации

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

Генерируйте исключения в момент обнаружения ошибки

Для начала рассмотрим пример:
public string GetUserAddress(string username)
{
  if (string.IsNullOrEmpty(username) || username.Length > maxUserNameLength)
    return string.Empty;

  IShopAccount account =
      ShopContext.CustomerManager.GetShopAccountByEmail(username, true);

  return account.Address;
}
Разработчик попытался замаскировать ошибочную ситуацию следующим образом: в случае передачи функции недопустимого параметра возвращается пустая строка. Код работает без исключений, и, на первый взгляд, хорошо. Но как программисту, который вызывает данную функцию, узнать о том, что пустая строка это не адрес, а признак ошибки? Такие неявные ошибки нужно явно обрабатывать после каждого вызова функции. А если не обработать, то возврат пустой строки может привести к другой ошибке, причину которой сложнее выявить. Ведь ошибочный сценарий не обрабатывается, а всего лишь маскируется. Отсутствие исключений в данной ситуации совсем не гарантирует правильную работу программы в целом.
Поскольку сама функция не может обработать ошибочную ситуацию, то предпочтительнее сгенерировать исключение сразу же после ее обнаружения. Если длина username может оказаться неправильной (например, при вводе на форме пользователем), то лучше её проверить перед вызовом функции GetUserAddress и выдать сообщение пользователю. Сгенерированное исключение только поможет обнаружить необработанную ошибочную ситуацию в программе и исправить её. Предлагается следующий вариант функции:
public string GetUserAddress(string username)
{
  if (string.IsNullOrEmpty(username) || 
                username.Length > maxUserNameLength)
    throw new ArgumentException("Invalid username length", "username");

  IShopAccount account =
     ShopContext.CustomerManager.GetShopAccountByEmail(username, true);

  return account.Address;
}

Злоупотребление оператором as

К проблеме маскирования ошибочных ситуаций относится и злоупотребление оператором as. Он отличается от обычного преобразования типов тем, что в случае несоответствия типов возвращает null, а не генерирует InvalidCastException. И, возможно, этим он показался более привлекательным автору следующей функции:
private void ProductAddedHandler(object source, ItemAddedEventArgs e)
{
  string title = (e.Item as IProduct).Title;
  ...
}
Рассмотрим два сценария, в которых разработчик ожидает, что e.Item обладает следующими свойствами:
  1. Всегда ссылается на объект класса, реализующий IProduct.
  2. Может ссылаться на разнородные объекты (как реализующие IProduct, так и не реализующие).
Заметим, приведенный пример неправилен в обоих случаях.
Рассмотрим первый сценарий. Предположим, что в e.Item ошибочно попал объект, не реализующий IProduct. Тогда выражение в скобках вернет null, и обращение к свойству Title приведет к NullReferenceException. Маскируется одна ошибочная ситуация (неправильный тип в e.Item), что сразу же приводит к другой ошибочной ситуации (обращение к свойству по null ссылке). Программисту, обнаружившему NullReferenceException, придется лишний раз подумать, почему результатом выражения является null. Если в e.Item ожидаются только объекты, реализующие IProduct, лучше получить InvalidCastException, что более точно описывает ошибочную ситуацию:
        string title = ((IProduct)e.Item).Title;
Теперь при чтении кода сразу видно, что в e.Item должен находиться объект, реализующий IProduct.
Во втором сценарии обязательна проверка на null после приведения типа:
IProduct product = e.Item as IProduct;
if (product != null)
{
  string title = product.Title;
  ...
}
Подведем итог. Почти всегда лучше получить исключение сейчас, чем неправильное поведение потом. Сэкономленное время на изучении последствий замаскированной ошибки можно потратить на что-то полезное, например, почитать “Коллеги, улыбнитесь” на RSDN.
СОВЕТ
Избегайте проверок “на всякий случай”, которые не обрабатывают ошибки, а всего лишь маскируют их и откладывают последствия “на потом”.

Неправильная обработка исключений

Рассмотрим простой пример, который содержит часто встречающуюся ошибку:
public IErpResponse DeserializeResponse(string response)
{
  using (MemoryStream ms = new MemoryStream(Encoding.UTF8.GetBytes(response)))
  {
    Type responseType = ObjectManager.GetType<IErpResponse>();
    XmlSerializer serializer = new XmlSerializer(responseType);

    try
    {
      return (IErpResponse)serializer.Deserialize(ms);
    }
    catch (Exception) 
    {
      return null; 
    }
  }
}
Функция десериализирует ответ ERP-системы в соответствующий объект. При возникновении любой ошибки функция возвращает null. Но так лучше не делать.
ПРИМЕЧАНИЕ
Вам не кажется, что этот catch слишком много на себя берет?
Здесь автор хотел обработать невалидный xml в response. Но сделал это неудачно. Например, в случае возникновения исключения при создании десериализируемого объекта, ситуация будет обработана так же, как и невалидный response. То есть, последствия ошибки опять откладываются на потом.
Рекомендуется обрабатывать наиболее конкретный тип ожидаемого исключения. Поэтому данный пример лучше переписать так:
public IErpResponse DeserializeResponse(string response)
{
  using (MemoryStream ms = new MemoryStream(Encoding.UTF8.GetBytes(response)))
  {
    Type responseType = ObjectManager.GetType<IErpResponse>();
    XmlSerializer serializer = new XmlSerializer(responseType);

    try
    {
      return (IErpResponse)serializer.Deserialize(ms);
    }
    catch (InvalidOperationException e)
    {
      if (e.InnerException is XmlException)
      {
        return null;
      }

      throw;
    }  
  }
}
В документации сказано, что метод XmlSerializer.Deserialize генерирует InvalidOperationException, причем исходное исключение находится в InnerException. Чтобы выделить XmlException, дополнительно проверяется InnerException. А в других случаях исключение передается дальше.
Рассмотрим второй пример:
public int Quantity
{
  get
  {
    try
    {
      return int.Parse(TxtQuantity.Text);
    }
    catch (Exception)
    {
      return 0;
    }
  }
}
Свойство возвращает значение, введенное пользователем на Web-форме. В случае неправильного ввода данных возвращается ноль. Такой подход содержит проблему, описанную выше (обрабатываются все исключения вместо FormatException и OverflowException). Но здесь мы хотим обратить внимание на другой момент. Конструкция try…catch является достаточно трудночитаемой. Поэтому, если можно, лучше обойтись без нее. Например, это можно сделать так:
public int Quantity
{
  get
  {
    int quantity;
    if(int.TryParse(TxtQuantity.Text, out quantity))
      return quantity;
    
    return 0;
  }
}
Заметим, функция TryParse делает одновременно две вещи: проверяет строку и преобразует ее в целое число. Если можно было бы разделить эти два действия (читай: если существовала бы функция int.Valid), то код стал бы еще нагляднее:
 public int Quantity
 {
   get
   {
     if (!int.Valid(TxtQuantity.Text))
       return 0;

     return int.Parse(TxtQuantity.Text);
   }
 }
Конечно, производительность немного уменьшилась. Но это далеко не всегда является проблемой Изучим этот вопрос в следующем разделе.

Не оптимизируйте раньше времени

К вопросу оптимизации кода надо подходить очень осторожно. Нужно всегда помнить, что оптимизированный код часто бывает неочевидным и трудночитаемым в сравнении с неоптимизированным. Поэтому в разработке мы используем следующий подход:
Вначале пишем простой и понятный, рабочий код, не оптимизированный по скорости исполнения. Это позволяет избежать багов и выработать четкое понимание того, как система должна работать.
Проводится оптимизация медленно работающих частей программы.
Рассмотрим второй пункт более детально.
Если некоторая функциональность программы работает недостаточно быстро, надо определить максимальное допустимое время работы. Подход – “сделать так, чтобы работало как можно быстрее” – неправильный, поскольку оптимизировать можно все. Вопрос в том, действительно ли это нужно? Если максимально допустимое время работы равняется одной секунде, то функциональность, работающая 0,5 секунды и 0,1 секунды считается приемлемой. Часто выбирается тот вариант, который обойдется дешевле.
Оптимизируется, как правило, наиболее долго работающая часть кода. Почему? Потому что вероятность значительно улучшить производительность выше, чем при оптимизации остальных частей. Перед тем, как оптимизировать, желательно исследовать проблему. Рассмотрим пример.
Нужно оптимизировать скорость загрузки домашней страницы некоторого сайта. Она грузится 4 секунды. Допустимое время загрузки: 2 секунды. Был произведен замер производительности. Результаты измерений приведены в следующей таблице:
Загрузка меню
3 секунды
Загрузка Master Page без меню
0.5 секунды
Загрузка страницы
0.5 секунды
Если считать, что второй и третий вариант удастся оптимизировать на 50%, мы получим ускорение на 0,5 секунды, А этого недостаточно, чтобы получить допустимое время. В случае с первым вариантом 50% уменьшение времени улучшит результат на 1,5 секунды. Поэтому логичнее начать оптимизировать именно меню, несмотря на то, что остальные два пункта могут быть написаны неоптимально по производительности. После изучения работы меню выяснилось, что оно медленно работает из-за множества картинок продуктов, которые в нем присутствуют.
СОВЕТ
Проблема появилась не из-за неоптимального по производительности кода, а из-за большого объема передаваемых данных. Очень часто случается, что код “не виноват”. Поэтому не оптимизируйте ничего, пока не убедитесь, что это действительно нужно.
Было принято решение убрать картинки с меню. В результате время загрузки уменьшилось до 0.5 секунд. Таким образом, страница начала в среднем загружаться за 1.5 секунды, что является допустимым временем.
Бывает, но редко, что все части функциональности работают приблизительно одинаковое время, тогда выбор надо делать по обстоятельствам. Но этот вопрос выходит за рамки этой статьи.

Заключение

Эта статья была написана, чтобы помочь обнаруживать ошибки, которые часто встречаются в коде программных продуктов. Все вышеизложенное не претендует на абсолютную истину. Воспринимайте это как рекомендации, как пример хороших практик.
Код стоит писать оптимально. Но при этом всегда нужно определиться с критерием оптимальности. При командной разработке понятность кода является одним из главных критериев, а производительность – далеко не всегда. Инвестиции времени на написание качественного кода часто окупаются. Поэтому давайте сделаем наш код лучше!

Список литературы

  1. “Почему ваш код – отстой”. http://rsdn.ru/article/philosophy/whyyourcode.xml
  2. “Оптимизация - ваш злейший враг”. http://rsdn.ru/article/philosophy/Optimization.xml
  3. М.Фаулер. Рефакторинг: улучшение существующего кода. СПб: Символ-Плюс, 2003.
  4. С. Макконнелл. Совершенный код. М.: Издательско-торговый дом “Русская Редакция” СПб.: Питер, 2005.
  5. Гради Буч. Объектно-ориентированный анализ и проектирование с примерами приложений на С++. 2 изд.
  6. Функциональное программирование для всех. http://rsdn.ru/article/funcprog/fp.xml

Комментариев нет:

Отправить комментарий

Подписаться на: Комментарии к сообщению (Atom)