Какой код проще, лучше и почему - Философия программирования

Просьба вникнуть в код и найти потенциальные ошибки в каждом случае

3.5

    private void Collect(IService sr, ObjectCategories protection)
    {
            sr.WorkingSegments.Cast<ServiceSegment>()
            .Concat(sr.ProtectionSegments.Cast<ServiceSegment>())
            .SelectMany(s => s.SegmentParcels.Cast<SegmentParcel>())
            .Select(p => p.NetworkConnection)
            .Where(nc => (nc != null))
            .Execute(nc => Collect(nc, protection));
     }

2.0

    private void Collect(IService sr, ObjectCategories protection)
    {
      List<ServiceSegment> segments = new List<ServiceSegment>();

      foreach (ServiceSegment item in sr.WorkingSegments)
        segments.Add(item);
      foreach (ServiceSegment item in sr.ProtectionSegments)
        segments.Add(item);

      foreach (ServiceSegment segment in segments)
      {
        foreach (SegmentParcel parcel in segment.SegmentParcels)
        {
          if (parcel.NetworkConnection == null)
            continue;

          Collect(parcel.NetworkConnection, protection);
        }
      }
    }

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

I>Просьба вникнуть в код и найти потенциальные ошибки в каждом случае

Вот такой лучше:

var segments = sr.WorkingSegments.Cast<ServiceSegment>().Concat(sr.ProtectionSegments.Cast<ServiceSegment>());
var connections = from s in segments
                  from p in s.SegmentParcels.Cast<SegmentParcel>
                  let nc = p.NetworkConnection
                  where nc != null
                  select nc;
foreach(var connection in connections)
    Collect(connection, protection);

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

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

I>>Просьба вникнуть в код и найти потенциальные ошибки в каждом случае

L>Вот такой лучше:

L>var connections = from s in segments
L>                  from p in s.SegmentParcels.Cast<SegmentParcel>
L>                  let nc = p.NetworkConnection
L>                  where nc != null
L>                  select nc;
L>

let здесь по-моему лишний. Не тот случай, чтобы запоминать промежуточные значения нужно было.

var connections = from s in segments
                  from p in s.SegmentParcels.Cast<SegmentParcel>
                  where p.NetworkConnection != null
                  select p.NetworkConnection;

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

S>let здесь по-моему лишний. Не тот случай, чтобы запоминать промежуточные значения нужно было.

Это уже вопрос личных предпочтений. Я в свою очередь не вижу причин, почему бы не вставить тут let.

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

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

S>>let здесь по-моему лишний. Не тот случай, чтобы запоминать промежуточные значения нужно было.

L>Это уже вопрос личных предпочтений. Я в свою очередь не вижу причин, почему бы не вставить тут let.

Это лишний анонимный тип с лишними экземплярами. Они оправданы, когда за let-ом нужны наборы значений s, p и nc, либо когда вычисленное значение нужно закэшировать. А т.к. нужно лишь значение nc, притом что p.NetworkConnection вряд ли нужно кэшировать, то на мой взгяд let не оправдан.

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

L>>Это уже вопрос личных предпочтений. Я в свою очередь не вижу причин, почему бы не вставить тут let.

S>Это лишний анонимный тип с лишними экземплярами.

Честно говоря, меня вся эта кухня мало волнует.

S>Они оправданы, когда за let-ом нужны наборы значений s, p и nc, либо когда вычисленное значение нужно закэшировать. А т.к. нужно лишь значение nc, притом что p.NetworkConnection вряд ли нужно кэшировать, то на мой взгяд let не оправдан.

На мой вкус вариант с let читабельнее, но я не настаиваю.

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

L>Вот такой лучше:

L>var segments = sr.WorkingSegments.Cast<ServiceSegment>().Concat(sr.ProtectionSegments.Cast<ServiceSegment>());
L>var connections = from s in segments
L>                  from p in s.SegmentParcels.Cast<SegmentParcel>
L>                  let nc = p.NetworkConnection
L>                  where nc != null
L>                  select nc;
L>foreach(var connection in connections)
L>    Collect(connection, protection);
L>

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

Один из хелперов ты скипнул почему то

и заменил на цикл.

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

I>смотрится покрасивше, но это плохой способ.

Это хороший способ. Два плохих я не стал квотить.

I>Потому что например есть кое какиех хелперы и их пристроить сюда никак нельзя.

I>Один из хелперов ты скипнул почему то

и заменил на цикл.

Их пристраивать просто не нужно. Не надо в одну кучу валить получение данных и обработку, потом вам за это спасибо скажут.

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

I>Просьба вникнуть в код и найти потенциальные ошибки в каждом случае

Лучше всего так:

    private void Collect(IService sr, ObjectCategories protection)
    {
      IEnumerable<ServiceSegment> segments = _.Combine(sr.WorkingSegments, sr.ProtectionSegments);

      foreach (ServiceSegment segment in segments)
      {
        foreach (SegmentParcel parcel in segment.SegmentParcels)
        {
          if (parcel.NetworkConnection != null)
            Collect(parcel.NetworkConnection, protection);
        }
      }
    }

В этом варианте и лишних сущностей не появляется (в отличие от первого варианта) и по лаконичности он первому варианту не уступает.

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

I>Просьба вникнуть в код и найти потенциальные ошибки в каждом случае

I>3.5
I>

I>    private void Collect(IService sr, ObjectCategories protection)
I>    {
I>            sr.WorkingSegments.Cast<ServiceSegment>()
I>            .Concat(sr.ProtectionSegments.Cast<ServiceSegment>())
I>            .SelectMany(s => s.SegmentParcels.Cast<SegmentParcel>())
I>            .Select(p => p.NetworkConnection)
I>            .Where(nc => (nc != null))
I>            .Execute(nc => Collect(nc, protection));
I>     }
I>

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

      foreach (var ss in new[] { sr.WorkingSegments, sr.ProtectionSegments })
        foreach (ServiceSegment s in ss)
          foreach (SegmentParcel p in s.SegmentParcels)
          {
            if (p.NetworkConnection != null)
              Collect(p.NetworkConnection, protection);
          }

В функциональном варианте 250 символов, в императивном 220, т.е. даже меньше.

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

U>Кстати, и лаконичность функционального варианта мнимая и достигается только за счет использования однобуквенных переменных и отказа от явного указания типов. Если также делать в чисто императивном варианте, то код получится столь же компактным.

U>

U>      foreach (var ss in new[] { sr.WorkingSegments, sr.ProtectionSegments })
U>        foreach (ServiceSegment s in ss)
U>          foreach (SegmentParcel p in s.SegmentParcels)
U>          {
U>            if (p.NetworkConnection != null)
U>              Collect(p.NetworkConnection, protection);
U>          }
U>

U>В функциональном варианте 250 символов, в императивном 220, т.е. даже меньше.

Твой вариант падает в рантайме на втором foreach-е

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

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

I>>Просьба вникнуть в код и найти потенциальные ошибки в каждом случае

I>>3.5
I>>

I>>    private void Collect(IService sr, ObjectCategories protection)
I>>    {
I>>            sr.WorkingSegments.Cast<ServiceSegment>()
I>>            .Concat(sr.ProtectionSegments.Cast<ServiceSegment>())
I>>            .SelectMany(s => s.SegmentParcels.Cast<SegmentParcel>())
I>>            .Select(p => p.NetworkConnection)
I>>            .Where(nc => (nc != null))
I>>            .Execute(nc => Collect(nc, protection));
I>>     }
I>>

U>Кстати, и лаконичность функционального варианта мнимая и достигается только за счет использования однобуквенных переменных и отказа от явного указания типов. Если также делать в чисто императивном варианте, то код получится столь же компактным.

U>

U>      foreach (var ss in new[] { sr.WorkingSegments, sr.ProtectionSegments })
U>        foreach (ServiceSegment s in ss)
U>          foreach (SegmentParcel p in s.SegmentParcels)
U>          {
U>            if (p.NetworkConnection != null)
U>              Collect(p.NetworkConnection, protection);
U>          }
U>

U>В функциональном варианте 250 символов, в императивном 220, т.е. даже меньше.

Тебе повезло с примером. Вот получше пример.

      IEnumerable<ServiceSegment> segments = _.Combine(sr.WorkingSegments, sr.ProtectionSegments);

а что это за конструкция такая?

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

M>

M>      IEnumerable<ServiceSegment> segments = _.Combine(sr.WorkingSegments, sr.ProtectionSegments);
M>

M>а что это за конструкция такая?

Объединение произвольного количества коллекций.

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

S>Твой вариант падает в рантайме на втором foreach-е

Да, new[] лучше не использовать, надо так:

      foreach (var ss in new IEnumerable[] { sr.WorkingSegments, sr.ProtectionSegments })
        foreach (ServiceSegment s in ss)
          foreach (SegmentParcel p in s.SegmentParcels)
          {
            if (p.NetworkConnection != null)
              Collect(p.NetworkConnection, protection);
          }

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

G>Тебе повезло с примером. Вот получше пример.

И чем это:

var matches = from item in items 
              where criteria.All(
                criterion=>criterion.IsMetBy(item)) 
              select item;
match = matches.FirstOrDefault();

лучше чем?

      foreach (var item in items)
      {
        if (criteria.All(criterion => criterion.IsMetBy(item)))
        {
          match = item;
          break;
        }
      }

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

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

G>>Тебе повезло с примером. Вот получше пример.

U>И чем это:

U>

U>var matches = from item in items 
U>              where criteria.All(
U>                criterion=>criterion.IsMetBy(item)) 
U>              select item;
U>match = matches.FirstOrDefault();
U>

U>лучше чем?

U>

U>      foreach (var item in items)
U>      {
U>        if (criteria.All(criterion => criterion.IsMetBy(item)))
U>        {
U>          match = item;
U>          break;
U>        }
U>      }
U>

intention лучше выражен.

U>var matches = from item in items 
U>              where criteria.All(
U>                criterion=>criterion.IsMetBy(item)) 
U>              select item;
U>match = matches.FirstOrDefault();
U>

U>лучше чем?

U>

U>      foreach (var item in items)
U>      {
U>        if (criteria.All(criterion => criterion.IsMetBy(item)))
U>        {
U>          match = item;
U>          break;
U>        }
U>      }
U>

1)Количеством строк.
2)FirstOrDefault гораздо понятнее, чем использование break. FirstOrDefault лучше отражает суть того что происходит.
3)В случае Linq matches это объект, который можно куда-то передать или как-то обработать, в случае с циклом такого не выйдет.

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

G>1)Количеством строк.

И с каких это пор скобочки стали учитываться при подсчете строк?

G>2)FirstOrDefault гораздо понятнее, чем использование break. FirstOrDefault лучше отражает суть того что происходит.

Издеваешься? FirstOrDefault это вообще крайне опасная операция, т.к. если match у нас, к примеру, int, то в случае пустого массива получим 0, который можно очень долго по коду отлавливать.

G>3)В случае Linq matches это объект, который можно куда-то передать или как-то обработать, в случае с циклом такого не выйдет.

С каких это пор создание лишних сущностей стало достоинством кода?

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

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

G>>1)Количеством строк.

U>И с каких это пор скобочки стали учитываться при подсчете строк?

С тех пор как текст программ начали читать

G>>2)FirstOrDefault гораздо понятнее, чем использование break. FirstOrDefault лучше отражает суть того что происходит.

U>Издеваешься? FirstOrDefault это вообще крайне опасная операция, т.к. если match у нас, к примеру, int, то в случае пустого массива получим 0, который можно очень долго по коду отлавливать.
А с break что получишь? Тот же самый 0 при наивной реализации или придется более сложные конструкции городить.

G>>3)В случае Linq matches это объект, который можно куда-то передать или как-то обработать, в случае с циклом такого не выйдет.
U>С каких это пор создание лишних сущностей стало достоинством кода?
Это называется страшным словом composition. Чем выше возможности композиции тем лучше, это всегда так было.

От:	Ikemefula	http://blogs.rsdn.org/ikemefula
Дата:	01.04.10 18:58
Оценка:

	От:	Lloyd
	Дата:	01.04.10 20:02
	Оценка:	11 (2) +3

От:	samius	http://sams-tricks.blogspot.com
Дата:	01.04.10 20:07
Оценка:	1 (1)

	От:	Lloyd
	Дата:	01.04.10 20:17
	Оценка:

От:	samius	http://sams-tricks.blogspot.com
Дата:	01.04.10 20:24
Оценка:	11 (1)

От:	gandjustas	http://blog.gandjustas.ru/
Дата:	02.04.10 06:22
Оценка: