Re[2]: Как не стоит писать код - .NET

Здравствуйте, Sinclair, Вы писали:

N_P>>1) При модификации кода сложнее 10 строк очень просто мазнуть взглядом и не заметить точки возврата из середины функции. Например, раньше возвращали lenght, теперь position. В конце сделали lenght+startpos, в середине пропустили.

S>Это если мы возвращаем разный length. В примере речь идёт о побочных ветках, когда должен возвращаться null, 0, или -1. Безо всякой дальнейшей математики.

И пример в статье и мой пример — всего лишь пример. Хотите ближе к ? Пожалуйста — была GetElement(id) с return null при "не найдено", стала GetElementOrDefault(id) с return new Element(). В конце поправили, в середине — забыли. В коде сложнее "пример в статье" фразу "return null" просмотреть — опять же легче легкого.

N_P>>2) При смене возвращаемого типа очень муторно и не прозрачно. Например, раньше возвращали lenght, теперь класс {Lenght, Position, Token}. А у вас первая точка возврата ДО вычисления токена. Придется курочить код, да еще вспоминать и разбираться — что он делал.
S>Ну, отловить проблемы нам поможет компилятор. Починить проблемы — следование другим рекомендациям из статьи. В частности, разбиение метода на более мелкие подметоды, так, чтобы основная логика стала понятной.

Именно в данном случае вместо рефакторинга 1 функции вам, скорее всего, придется рефакторить еще и ее под.функции потому как туда и оттуда новые типы принимать\передавать надо. Как бы в итоге еще и не больше провозиться придется.

P.S. Вычленение малых функций тоже не всегда хорошо. Особенно, если код линейный и не переиспользуемый. Потому как каждая функция — это какие-то условности и негласные договоренности (что входит, что выходит, как ошибается и что делает). Конечно, не стоит экономить на названиях функций, но их информативность тоже не всеобъемлюща. В качестве примера — как говоряще назвать такое:

foreach
{
   Element ElementB;
   if (InputStore.TryGet(ElementA.ID, out ElementB))
   {
      ElementA.Value = ElementB.Value;
   }
   else
   {
      ElementA.Value = ElementFormat.DefaultValue;
   }
   ElementA.ModificationTime = DateTime.UTCNow
}

Разбить на 3 функции с простыми названиями и foreach в каждой ?

Здравствуйте, Огинский Евгений, Бурда Роман, Вы писали:

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

Здравствуйте, Огинский Евгений, Бурда Роман, Вы писали:

ОЕБ>В статье рассматриваются некоторые ошибки, которые часто встречаются в коде программ. Даны рекомендации, как не стоит писать код, на какие этапы разработки кода нужно обращать внимание в первую очередь.

Кстати, вы не видите противоречий между этими двумя приемами упрощения?

Первый прием. Код:

// Не отправлено ли уведомление ранее и наступило ли время отправки уведомления.
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);
}

Что здесь по сути сделано? Сложные вызовы заменены на короткие псевдонимы.

Второй прием. Код:

public int Quantity
{
  get
  {
    int quantity;
    if(int.TryParse(TxtQuantity.Text, out quantity))
      return quantity;
    
    return 0;
  }
}

Заменен на:

public int Quantity
 {
   get
   {
     if (!int.Valid(TxtQuantity.Text))
       return 0;

     return int.Parse(TxtQuantity.Text);
   }
 }

Что здесь по сути сделано? Множественное использование короткого псевдонима заменено на множественное использование сложных вызовов.

Можно объяснить почему в первом примере хорош один прием, а во втором примере хорош прием обратный первому?

Здравствуйте, Nikolay_P_I, Вы писали:

N_P>И пример в статье и мой пример — всего лишь пример. Хотите ближе к ? Пожалуйста — была GetElement(id) с return null при "не найдено", стала GetElementOrDefault(id) с return new Element(). В конце поправили, в середине — забыли. В коде сложнее "пример в статье" фразу "return null" просмотреть — опять же легче легкого.
Ок, давайте обсудим новый пример.
Я правильно понимаю, что вы имеете в виду код типа:

public Element GetElement(int id)
{
  if (id < 0)
    return null;

  var index = this.Elements.IndexOf(id);
  if (index < 0)
    return null;

  return this.Elements[id];
}

И риск исправить его вот так:

public Element GetElementOrDefault(int id)
{
  if (id < 0)
    return new Element(); // Ok;

  var index = this.Elements.IndexOf(id);
  if (index < 0)
    return null; // ай, шайтан! проглядел.

  return this.Elements[id];
}

Да?
Тогда я соглашусь с вашей точкой зрения сразу же, как только вы объясните мне, чем ваш подход с "result variable" поможет в таком коде:

public Element GetElementOrDefault(int id)
{
  var result = default(Element);
  if (id >= 0)
  {
    var index = this.Elements.IndexOf(id);
    if (index >= 0)
      result = this.Elements[id];
    else
      result = new Element(); // Ok;
  }
  else
    result = null; // опять забыл!

  return result;
}

Если вы имеете в виду вот такое решение:

public Element GetElementOrDefault(int id)
{
  var result = new Element();
  if (id >= 0)
  {
    var index = this.Elements.IndexOf(id);
    if (index >= 0)
      result = this.Elements[id];
  }
  return result;
}

То оно тоже не шибко хорошо, т.к. провоцирует трудноотлаживаемые ошибки в логике, которая завязывается на определённое текущее значение этого result. Из-за большого количества присваиваний в result в теле длинной функции никаких выводов о реальном значении в месте возврата сделать нельзя. Точнее, можно, но сложность этого в точности равна сложности анализа множественных точек возврата. Скорее наоборот — в первоначальном варианте мы могли легко увидеть все места, где возвращается null (или new Element()). В новом варианте понять, в каких случаях возвращается new Element(), достаточно тяжко.
Недавно отлаживал код, переведённый с Delphi. Там вообще нет return statement, поэтому всё реализовано именно так, как вам нравится. Понять логику — чудовищно сложная задача. Result присваивается по полсотни раз на метод, да ещё и грешит штуками типа if (многоэтажка) Result = Result.Parent else Result = какая-то-ещё-локальнаяпеременная.Sibling

N_P>Именно в данном случае вместо рефакторинга 1 функции вам, скорее всего, придется рефакторить еще и ее под.функции потому как туда и оттуда новые типы принимать\передавать надо. Как бы в итоге еще и не больше провозиться придется.
Смотря в каком стиле написаны эти подфункции.

N_P>

N_P>foreach
N_P>{
N_P>   Element ElementB;
N_P>   if (InputStore.TryGet(ElementA.ID, out ElementB))
N_P>   {
N_P>      ElementA.Value = ElementB.Value;
N_P>   }
N_P>   else
N_P>   {
N_P>      ElementA.Value = ElementFormat.DefaultValue;
N_P>   }
N_P>   ElementA.ModificationTime = DateTime.UTCNow
N_P>}
N_P>

N_P>Разбить на 3 функции с простыми названиями и foreach в каждой ?

Ну, если вы не будете заниматься ерундой для искусственного
наращивания LOC, то получится простой и понятный код:

void ApplyValuesFromInputStore(IEnumerable elements, Store inputStore)
{
  Element source;
  foreach(target in elements)
  {
    target.Value 
      = inputStore.TryGet(target.ID, out source) 
        ? source.Value 
        : ElementFormat.DefaultValue;
    target.ModificationTime = DateTime.UTCNow; // кстати, вот это должно быть частью сеттера Value;
  }
}

Этот код уже можно понять без поллитры. В нём нет лишнего состояния.
Можно сделать наоборот — реализовать чинилку для отдельного элемента:

Action<Element> ApplyValuesFromInputStore(Store inputStore)
{
  return (Element target) => {
    Element source;
    target.Value 
      = inputStore.TryGet(target.ID, out source) 
        ? source.Value 
        : ElementFormat.DefaultValue;
    target.ModificationTime = DateTime.UTCNow; 
  }
}

И применять её по месту при помощи встроенного foreach:

Document.Elements.ForEach(ApplyValuesFromInputStore(inputStore));

Здравствуйте, Pavel Dvorkin, Вы писали:

PD>К одному известному композитору пришел начинающий и предложил прослушать его симфонию.
PD>После окончания симфонии маэстро сказал : "В Вашей симфонии действительно содержится много нового и интересного."
PD>-Вы думаете ? — расцвел дебютант.
PD>Маэстро продолжил : Жаль только, что интересное в ней не ново, а новое — не интересно.

Чесно говоря, мы не ставили перед собой цель написать о чем то новом и интересном в статье. На работе, часто встречаемся с одними и теми же проблемами (довольно таки неинтересными). Просто решили описать типичные недостатки на реальных примерах из наших проектов, и показать возможные пути устранения этих недостатков своим коллегам. И оформили все в виде статьи, может ещё кому то будет интересно.

Здравствуйте, Undying, Вы писали:

U>Кстати, вы не видите противоречий между этими двумя приемами упрощения?

Противоречий по-моему нет.

U>Первый прием. Код:

U>

U>// Не отправлено ли уведомление ранее и наступило ли время отправки уведомления.
U>if (!context.Order.Fields.ContainsKey("OrderMailSend") 
U>  && (context.Order.PaymentStatus == PaymentStatus.Paid 
U>      || (context.Order.PaymentStatus == PaymentStatus.InProgress 
U>        && GetPaymentMethod(context).StartsWith("banktransfer")
U>     )
U>   )
U>)
U>{
U>  IEnumerable<string> email = GetEmailAddress(context);
U>  MailNotification.SendOrderConfirmation(email, context.Order);
U>  // Отмечаем что уведомление о заказе отправлено
U>  context.Order.Fields.Add(
U>    new EntityField { Name = "OrderMailSend", Value = true });
U>}
U>

U>Заменен на:

U>

U>if (!OrderMailSent(context) && MailNotificationRequired(context))
U>{
U>  IEnumerable<string> email = GetEmailAddress(context);
U>  MailNotification.SendOrderConfirmation(email, context.Order);
U>  MarkOrderAsSent(context);
U>}
U>

U>Что здесь по сути сделано? Сложные вызовы заменены на короткие псевдонимы.

Не совсем. Здесь сложные вызовы разбиты на логически завершенные операции, которые выполняют одно действие и имеют понятные имена. Если бы мы просто хотели заменить сложные вызовы на короткие псевдонимы, то написали бы так:

if (OrderMailNotSentAndMailNotificationRequired(context))
{
  SendOrderConfirmationAndMarkOrderAsSent(context);
}

U>Второй прием. Код:

U>

U>public int Quantity
U>{
U>  get
U>  {
U>    int quantity;
U>    if(int.TryParse(TxtQuantity.Text, out quantity))
U>      return quantity;
    
U>    return 0;
U>  }
U>}
U>

U>Заменен на:

U>

U>public int Quantity
U> {
U>   get
U>   {
U>     if (!int.Valid(TxtQuantity.Text))
U>       return 0;

U>     return int.Parse(TxtQuantity.Text);
U>   }
U> }
U>

U>Что здесь по сути сделано? Множественное использование короткого псевдонима заменено на множественное использование сложных вызовов.

Здесь тоже попытались разбить операцию на две части, чтобы каждая часть выполняла свое логически завершенное действие и имела понятное имя.

Здравствуйте, Nuseraro, Вы писали:

N>Я бы сказал, что эти методы надо или поднимать до уровня Extension к Order с претензией на Reusable, или опускать до уровня переменной в использующем их методе, не усложняя внутренней структуры класса.
Или взять более выразительный язык, в котором микрометоды можно делать локальными функциями.
Таким образом, они не будут отсвечивать в классе.

N>Тут по моим представлениям в итоге получилась плохая практика, не знаю, как ее назвать правильно. Короче когда вроде и порефакторили, а в результате методы начали общаться на языке допущений "я возвращаю null, если линия предназначена к игнорированию".
Согласен. Но на C# лучше делать трудно.
Тут нужно что-то чуть более выразительное.
Например, так:
Кстати названия методов тоже плохие.

//отделяем логику обновления текущей корзины от собственно обновления корзины
public UpdateCurrentBasket(linesData : IEnumerable[IBasketLineData]) : void
{
    when (GetCurrentBasket() is Some(customerBasket))
    {
        UpdateBasket(customerBasket, linesData)
        SaveCurrentBasket(customerBasket);
    }
}
//Это возможно нужно сделать методом расширением
private UpdateBasket(customerBasket : IBasket, linesData : IEnumerable[IBasketLineData]) : void
{
    foreach (lineData in linesData)
        when (!IgnoreLine(lineData.BasketLineId))
        when (customerBasket.BasketLines.GetById(lineData.BasketLineId) is Some(line))
            if (lineData.Stock != 0)
                line.Quantity = lineData.Stock;
            else
                customerBasket.BasketLines.Remove(line);
}

Если несколько when идут подряд, то отступ после каждого делать не имеет смысла.

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

  public variant option[T]
  {
    | None
    | Some { val : T; }
  }

В случае если функции нечего возвращать она возвращает None.
Если есть чего то Some в который завернуто возвращаемое значение.

Это позволяет четко показать, какая функция всегда возвращает значение, а какая может и не вернуть.
При этом компилятор заставит проверить, что функция вернула то, что надо.
Конструкция вида

when (GetCurrentBasket() is Some(customerBasket))

проверяет что из функции вернули Some и достает из него customerBasket.
После is может идти любой паттерн.

... << RSDN@Home 1.2.0 alpha 4 rev. 1472>>

Ошибочка в 3 примере:
PO>

PO>public bool TryGetUserAddress(string username, out string userAddress)
PO>{
PO>....
PO>}
PO>

Здравствуйте, Sinclair, Вы писали:

S>Ну, если вы не будете заниматься ерундой для искусственного
S>наращивания LOC, то получится простой и понятный код:
S>

S>void ApplyValuesFromInputStore(IEnumerable elements, Store inputStore)
S>{
S>  Element source;
S>  foreach(target in elements)
S>  {
S>    target.Value 
S>      = inputStore.TryGet(target.ID, out source) 
S>        ? source.Value 
S>        : ElementFormat.DefaultValue;
S>    target.ModificationTime = DateTime.UTCNow; // кстати, вот это должно быть частью сеттера Value;
S>  }
S>}
S>

S>Этот код уже можно понять без поллитры. В нём нет лишнего состояния.

Сорри, но вот за такой код хочется что-нибудь нехорошее сделать

Такой иф разбивать на несколько строчек совершенно не допустимо. он должен быть в одну строчку, не помещается — пишим нормальный иф!
Я вообше не сразу въехал что target.ModificationTime = DateTime.UTCNow; не часть ифа.

	От:	blackhearted
	Дата:	01.03.12 08:23
	Оценка:

	От:	Nikolay_P_I
	Дата:	01.03.12 10:45
	Оценка:

	От:	fin_81
	Дата:	01.03.12 11:35
	Оценка:

	От:	Undying
	Дата:	01.03.12 11:39
	Оценка:

От:	Sinclair	https://github.com/evilguest/
Дата:	01.03.12 12:25
Оценка:	1 (1) +4

	От:	Огинский Евгений
	Дата:	01.03.12 20:51
	Оценка:	+2

	От:	WolfHound
	Дата:	02.03.12 10:39
	Оценка:

	От:	Pro100Oleh
	Дата:	05.03.12 16:52
	Оценка:

	От:	gwg-605
	Дата:	06.03.12 14:11
	Оценка: