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

Здравствуйте, Аноним, Вы писали:

А>в общем какого-нибудь универсального решения нет судя по всему.

А>если закончилось место можно отправить почту или чего-нибудь еще придумать все зависит от приложения и фантазии
А>главное если почта не отправилась при этом опять не писать а файл

А>хотелось именно поянять как кто поступает в таких случаях.

Ты сам все правильно написал. Универсальных решений не бывает. Твой подход кажется вполне разумным. Пробуй, а там будет видно. Это единственное универсальное решение.

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

VD>Через какой домен входишь?

Как всегда — httр://rsdn.ru.

Клик по ссылке на статью ведет на адрес httр://rsdn.ru/article/?1111, а затем
выполняется редирект на httр://rsdn.ru/article/net/BadCode.xml, где пустой экран.

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

O>Как всегда — httр://rsdn.ru.

Странно.

O>Клик по ссылке на статью ведет на адрес httр://rsdn.ru/article/?1111, а затем
O>выполняется редирект на httр://rsdn.ru/article/net/BadCode.xml, где пустой экран.

Кликаю. И все ОК.

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

O>выполняется редирект на httр://rsdn.ru/article/net/BadCode.xml, где пустой экран.

Может просто кэш? Попробуй эту страничку по Ctrl+F5 обновить.

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

VD>Какие-то абстрактные наезды.
1)

protected string GetClass(object url)
{
  string result = string.Empty;

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

  return result;
}

protected string GetClass(object url)
{
  if (SiteMap.CurrentNode != null && SiteMap.CurrentNode.Url == url.ToString())
    return "class=\"active\"";

  return string.Empty;
}

Дело личного вкуса. Я для себя практически взял за правило сохранять результат функции в локальную переменную перед ретурном. Сложности чтения не добавляет, но дает возможность ставить бряку на ретурн и смотреть результат. Сильно помогает при отладке. Стопроцентное покрытие автотестами бывает только в сказках.

2) В примере из той же главы в самом "кошерном" варианте есть такой код:

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

Избавились от промежуточного стостояния, но какой ценой. На ровном месте. Имхо — не приемлимо.

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

VD>Какие-то абстрактные наезды. Читал статью, и не припомню таких мест. Хотелось бы более развренутой критики. Типа вот вам цитата, а вот мои мысли по ней.

Вообще статья хорошая, а цели у нее и вовсе отличные. Я за больше таких статей, но иногда конкретика в этой статье подкачивает:

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 });
}

Очень неожиданные сигнатуры методов, как параметр OrderPaid я ожидаю что-нибудь типа Order, а никак не IPaymentContext context.
Вообще внутренние "микрометоды" — это очень часто зло(кстати вроде у того же Макконела есть такая идея). Я бы сказал, что эти методы надо или поднимать до уровня Extension к Order с претензией на Reusable, или опускать до уровня переменной в использующем их методе, не усложняя внутренней структуры класса.

        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) //<--Bad
                {
                    ProcessLine(customerBasket, lineData.Stock, line);
                }
            }

            SaveBasket(customerBasket);
        }

        private void ProcessLine(IBasket customerBasket, int quantity, BasketLine line)
        {            
        //...
        }

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

            return basketLines.GetById(lineData.BasketLineId);
        }

Тут по моим представлениям в итоге получилась плохая практика, не знаю, как ее назвать правильно. Короче когда вроде и порефакторили, а в результате методы начали общаться на языке допущений "я возвращаю null, если линия предназначена к игнорированию". Это вообще плохой свойство кода такие допущения, сложно отлаживать и передавать эту информацию, кроме как явно, комментарием — типа "я возвращаю null если нет в базе". Такое уместно, только если basketLines.GetById тоже может вернуть null, и такое допущение уже есть. Да и название GetLine слишком простое, чтобы ожидать какую-то проверку на игнор. Вынос Ignore спорен, я бы вернул обратно наверх, например в виде if (Ignore) continue; или даже .Where в foreach.

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

AK>Дело личного вкуса. Я для себя практически взял за правило сохранять результат функции в локальную переменную перед ретурном. Сложности чтения не добавляет, но дает возможность ставить бряку на ретурн и смотреть результат. Сильно помогает при отладке. Стопроцентное покрытие автотестами бывает только в сказках.
Дело вкуса.
Но в этом вопрос автор по-моему прав:
"зачем переменной result присваивать значение, которое позже может измениться"
Заполнение result честнее заполнять в else.

AK>2) В примере из той же главы в самом "кошерном" варианте есть такой код:
AK>

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

AK>Избавились от промежуточного стостояния, но какой ценой. На ровном месте. Имхо — не приемлимо.

По моим представлениям, тут вопрос проходит ли CaсhedWebsites некие "Критерии свойства", т.е.
1) определенность (не зависит от времени, состояния побочных объектов, рандома)
2) достаточно быстро работает
Иначе его честнее назвать GetCaсhedWebsites() и общаться с ним в другом формате, типа
AK>

AK>{
AK>  var cachedWebsites = GetCaсhedWebsites();
AK>  if (cachedWebsites != null)
AK>      return cachedWebsites ;
AK>}
AK>...
AK>

Конкретно в этом случае мб и удовлетворяет — RequestState врядли измениться до какого-нибудь рефреша, да и операция GetValue наврядли такая уж тормозная — скорее всего все исполняется в памяти.

В примере говорится, что код

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);
   }
 }

Это плохой подход, так как Вы заменили одну "атомарную" операцию на несколько. Что будет, если свойство Text поменяется между вызовами?

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

AK>>В примере из той же главы в самом "кошерном" варианте есть такой код:
AK>>

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

AK>>Избавились от промежуточного стостояния, но какой ценой. На ровном месте. Имхо — не приемлимо.

N>По моим представлениям, тут вопрос проходит ли CaсhedWebsites некие "Критерии свойства", т.е.
N>1) определенность (не зависит от времени, состояния побочных объектов, рандома)
N>2) достаточно быстро работает
Другими словами, оверхед по ресурсам и вообще работоспособность метода зависит от реализации этого самого свойства.
В замен мы получаем чуть более понятный код (по мнению автора).

N>Иначе его честнее назвать GetCaсhedWebsites() и общаться с ним в другом формате, типа
AK>>

AK>>{
AK>>  var cachedWebsites = GetCaсhedWebsites();
AK>>  if (cachedWebsites != null)
AK>>      return cachedWebsites ;
AK>>}
AK>>...
AK>>

С подобного кода и начиналась история в статье, но автор говорит что это лишнее стостояние.

Пример с кодом:

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;
}

Идея верна: "Не маскируйте ошибочные ситуации". Я считаю, что это очень важно, из-за этого у нас часто возникают ошибки (остался код в наследство). Но Вы плохо развили идею. Не всегда нужен такое жесткое поведение как выкидывание исключения. Лично я использую три варианта:
1) при поведении как выше имя метода должно заканчиваться на OrDefault:

public string GetUserAddressOrDefault(string username)
{
  if (string.IsNullOrEmpty(username) || username.Length > maxUserNameLength)
    return string.Empty;

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

  return account.Address;
}

2) Этот повторяет Ваш:

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;
}

3) Этот заставляет вызывающего явно проверять результат:

public bool TryGetUserAddressOrDefault(string username, out string userAddress)
{
....
}

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

N_P>С лишней точкой возврата — точно вредный совет. При переделках кода (например, рефакторинге) влететь с ней — проще простого.

А какие именно проблемы могут возникнуть с этим? Приходит в голову разве что освобождение ресурсов, но если использовать using то все должно быть нормально.

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

N>Очень неожиданные сигнатуры методов, как параметр OrderPaid я ожидаю что-нибудь типа Order, а никак не IPaymentContext context.

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

N>Вообще внутренние "микрометоды" — это очень часто зло(кстати вроде у того же Макконела есть такая идея). Я бы сказал, что эти методы надо или поднимать до уровня Extension к Order с претензией на Reusable, или опускать до уровня переменной в использующем их методе, не усложняя внутренней структуры класса.

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

N>Тут по моим представлениям в итоге получилась плохая практика, не знаю, как ее назвать правильно. Короче когда вроде и порефакторили, а в результате методы начали общаться на языке допущений "я возвращаю null, если линия предназначена к игнорированию". Это вообще плохой свойство кода такие допущения, сложно отлаживать и передавать эту информацию, кроме как явно, комментарием — типа "я возвращаю null если нет в базе". Такое уместно, только если basketLines.GetById тоже может вернуть null, и такое допущение уже есть. Да и название GetLine слишком простое, чтобы ожидать какую-то проверку на игнор. Вынос Ignore спорен, я бы вернул обратно наверх, например в виде if (Ignore) continue; или даже .Where в foreach.

Полностью согласен, все таки лучше было бы не выносить Ignore.

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

VD>Из реальной жизни. РСДН падал просто потому, что на диске куда записывались логи Веб-сервера кончилось место.

Это security-фича, т.к. место на диске вполне могло закончиться из-за осознанных действий атакующего, направленных именно на то, чтобы нейтрализовать логирование и осуществить некие действия, не оставив за собой следов. С этой точки зрения, безусловное вываливание IIS в 500-ую ошибку при невозможности записи логов понятно. Непонятны две вещи:

1. Почему разработчики IIS сочли, что за ними должно быть последнее слово в принятии решения о подобном поведении сервера? IMHO, это нужно было сделать опцией, включенной по умолчанию.
2. Падать при первой же ошибке записи логов в место их хранения, с учетом того, что рядом простаивает полупустой террабайтный диск, а в оперативной памяти хватит места для хранения логов за несколько дней (в сжатом без потерь виде) — по меньшей мере странно. Разработчикам IIS следовало бы продумать альтернативные конфигурируемые варианты временного сохранения логов, в случае невозможности их записи в основное хранилище.

Т.е. сама идея отказа сервиса из-за невозможности журналирования правильная и здравая, но вот конкретно в IIS реализована через традиционное место

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

PO>1) при поведении как выше имя метода должно заканчиваться на OrDefault:
Понравился подход, возьму на вооружение.

Здравствуйте, Аноним, Вы писали:

N_P>>С лишней точкой возврата — точно вредный совет. При переделках кода (например, рефакторинге) влететь с ней — проще простого.

А>А какие именно проблемы могут возникнуть с этим? Приходит в голову разве что освобождение ресурсов, но если использовать using то все должно быть нормально.

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

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

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

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

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

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

PO>Это плохой подход, так как Вы заменили одну "атомарную" операцию на несколько. Что будет, если свойство Text поменяется между вызовами?[/c#]

Атомарность тут даже вторична. Главная проблема, что код оказался замусорен дублирующимися длинными вызовами, соответственно кроме технических характеристик (время работы, атомарность) резко ухудшилась и читабельность.

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

U>Атомарность тут даже вторична. Главная проблема, что код оказался замусорен дублирующимися длинными вызовами, соответственно кроме технических характеристик (время работы, атомарность) резко ухудшилась и читабельность.

Т.е. такая замена это однозначно антипаттерн.

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

AK>

AK>protected string GetClass(object url)
AK>{
AK>  string result = string.Empty;

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

AK>  return result;
AK>}
AK>

AK>vs
AK>

AK>protected string GetClass(object url)
AK>{
AK>  if (SiteMap.CurrentNode != null && SiteMap.CurrentNode.Url == url.ToString())
AK>    return "class=\"active\"";

AK>  return string.Empty;
AK>}
AK>

AK>Дело личного вкуса. Я для себя практически взял за правило сохранять результат функции в локальную переменную перед ретурном. Сложности чтения не добавляет, но дает возможность ставить бряку на ретурн и смотреть результат. Сильно помогает при отладке. Стопроцентное покрытие автотестами бывает только в сказках.

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

protected string GetClass(object url)
{
  string result = string.Empty;

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

  return result;
}

Как можно описать состояние переменной после первого if-а? Как недоинициализированное что ли

А использование переменной с таким состоянием ещё больше усложняет код.

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

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

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

От:	VladD2	www.nemerle.org
Дата:	28.02.12 17:56
Оценка:	+1

От:	okman	https://searchinform.ru/
Дата:	28.02.12 18:20
Оценка:

От:	VladD2	www.nemerle.org
Дата:	28.02.12 18:21
Оценка:

От:	VladD2	www.nemerle.org
Дата:	28.02.12 22:04
Оценка:

	От:	AK85
	Дата:	29.02.12 07:09
	Оценка:	4 (2) +1

	От:	Pro100Oleh
	Дата:	29.02.12 11:07
	Оценка:	1 (1) +2

От:	kochetkov.vladimir	https://kochetkov.github.io
Дата:	29.02.12 20:47
Оценка:	9 (1)

	От:	Огинский Евгений
	Дата:	29.02.12 21:04
	Оценка:

	От:	Nikolay_P_I
	Дата:	01.03.12 05:35
	Оценка:	4 (2) +1

От:	Sinclair	https://github.com/evilguest/
Дата:	01.03.12 08:12
Оценка: