Как Плохо этот Код?

Это не очень хорошо, но я видел гораздо хуже.

Предполагая, что это ASP.Net, есть ли причина, по которой вы используете этот подход вместо повторителя?

РЕДАКТИРОВАТЬ:

Если ретранслятор невозможен в этом случае, я бы рекомендовал использовать классы .Net HTML для генерации HTML вместо использования текста. Позже его будет немного легче читать / настраивать.

Вместо этого

        foreach (Post post in posts.Skip<Post>(i * PostsPerSlide))
        {
            // ...
            if (PostCount == PostsPerSlide)
                break;
        }

я бы переместил условие выхода в цикл следующим образом: (и исключил бы ненужный общий параметр, пока вы в нем)

        foreach (Post post in posts.Skip(i * PostsPerSlide).Take(PostsPerSlide))
        {
            // ...
        }

Что касается Фактически, ваши два вложенных цикла, вероятно, могут быть обработаны в одном цикле.

Кроме того, я бы предпочел использовать одинарные кавычки для атрибутов html, поскольку они допустимы и не требуют экранирования.

Было бы полезно, если бы определение для сообщений существовало, но в целом я бы сказал, что создание HTML в коде позади - это не путь в Asp.Net.

Поскольку это специально помечено как .Net ...

Для отображения списка элементов в коллекции вам лучше посмотреть на один из элементов управления данными (Repeater, Data List и т. Д.) И изучить как их правильно использовать.

http://quickstarts.asp.net/QuickStartv20/aspnet/doc/ctrlref/data/default.aspx

Я много видел хуже, но вы можете его немного улучшить.

1. Вместо StringBuilder.Append () со строкой.Format () внутри используйте StringBuilder.AppendFormat ()
2. Добавьте несколько модульных тестов, чтобы убедиться, что он производит желаемый результат, а затем реорганизуйте код внутри, чтобы он стал лучше. Тесты гарантируют, что у вас нет

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

Вот несколько советов:

Для обычных операций с плавающей запятой вы должны использовать double , а не Decimal . Однако в этом случае вам вообще не нужна арифметика с плавающей запятой, вы можете сделать это только с целыми числами:

int numberOfSlides = (posts.Count + PostsPerSlide - 1) / PostsPerSlide;

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

Для локальных переменных используется верблюжий регистр (postCoint), а не паскаль (PostCount). Постарайтесь сделать имена переменных осмысленными, а не использовать непонятные сокращения вроде «sb». Если область видимости переменной настолько мала, что вы не Для этого действительно нужно осмысленное имя, просто выберите простейшее из возможных и используйте одну букву вместо сокращения.

Вместо объединения строк «слайд-блок» и «первый» вы можете напрямую назначать буквальные строки. Таким образом вы заменяете вызов String.Concat простым назначением. (Это граничит с преждевременной оптимизацией, но, с другой стороны, конкатенация строк занимает примерно в 50 раз больше времени.)

Вы можете использовать .AppendFormat (...) вместо .Append (String .Format (...)) в StringBuilder. Я бы просто придерживался добавления в этом случае, поскольку на самом деле нет ничего, что нужно форматировать, вы просто объединяете строки.

Итак, я бы написал метод следующим образом:

public string PostsAsSlides(PostCollection posts, int postsPerSlide) {
   StringBuilder builder = new StringBuilder();
   int postCount = 0;
   foreach (Post post in posts) {
      postCount++;

      string cssClass;
      if (postCount == 1) {
         builder.Append("<div class=\"slide\">\n");
         cssClass = "slide-block first";
      } else if (postCount == postsPerSlide) {
         cssClass = "slide-block last";
         postCount = 0;
      } else {
         cssClass = "slide-block";
      }

      builder.Append("<div class=\"").Append(cssClass).Append("\">\n")
         .Append("<a href=\"").Append(post.Custom("Large Image"))
         .Append("\" rel=\"prettyPhoto[gallery]\" title=\"")
         .Append(post.MetaDescription).Append("\"><img src=\"")
         .Append(post.ImageUrl).Append("\" alt=\"").Append(post.Title)
         .Append("\" /></a>\n")
         .Append("<a class=\"button-launch-website\" href=\"")
         .Append(post.Custom("Website Url"))
         .Append("\" target=\"_blank\">Launch Website</a>\n")
         .Append("</div><!--.slide-block-->\n");

      if (postCount == 0) {
         builder.Append("</div><!--.slide-->\n");
      }

   }
   return builder.ToString();
}

Моя первая мысль, что это будет очень сложно провести модульное тестирование.

Я бы посоветовал использовать второй цикл for будет отдельной функцией, поэтому у вас будет что-то вроде:

for (int i = 0; i < NumberOfSlides; i++ )
        {
            int PostCount = 0;
            sb.Append("<div class=\"slide\">\n");
            LoopPosts(posts, i);
...

и LoopPosts:

private void LoopPosts(PostCollection posts, int i) {
...
}

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

Я видел намного хуже, но вы можете немного улучшить его.

1. Вместо StringBuilder.Append () со строкой.Format () внутри используйте StringBuilder.AppendFormat ()
2. Добавьте несколько модульных тестов, чтобы убедиться, что он производя желаемый результат, а затем реорганизуйте код внутри, чтобы он стал лучше. Тесты гарантируют, что вы ничего не сломали.