Jak zły jest ten kod?

Question

Ok, jestem amatorskim programistą i właśnie to napisałem. Wykonuje to zadanie, ale zastanawiam się, jakie to złe i jakie ulepszenia można wprowadzić.

[Proszę pamiętać, że jest to przedłużenie Kreda dla Graffiti CMS.]

public string PostsAsSlides(PostCollection posts, int PostsPerSlide) 
    { 
     StringBuilder sb = new StringBuilder(); 
     decimal slides = Math.Round((decimal)posts.Count/(decimal)PostsPerSlide, 3); 
     int NumberOfSlides = Convert.ToInt32(Math.Ceiling(slides)); 

     for (int i = 0; i < NumberOfSlides; i++) 
     { 
      int PostCount = 0; 
      sb.Append("<div class=\"slide\">\n"); 
      foreach (Post post in posts.Skip<Post>(i * PostsPerSlide)) 
      { 
       PostCount += 1; 
       string CssClass = "slide-block"; 

       if (PostCount == 1) 
        CssClass += " first"; 
       else if (PostCount == PostsPerSlide) 
        CssClass += " last"; 

       sb.Append(string.Format("<div class=\"{0}\">\n", CssClass)); 
       sb.Append(string.Format("<a href=\"{0}\" rel=\"prettyPhoto[gallery]\" title=\"{1}\"><img src=\"{2}\" alt=\"{3}\" /></a>\n", post.Custom("Large Image"), post.MetaDescription, post.ImageUrl, post.Title)); 
       sb.Append(string.Format("<a class=\"button-launch-website\" href=\"{0}\" target=\"_blank\">Launch Website</a>\n", post.Custom("Website Url"))); 
       sb.Append("</div><!--.slide-block-->\n"); 

       if (PostCount == PostsPerSlide) 
        break; 
      } 
      sb.Append("</div><!--.slide-->\n"); 
     } 

     return sb.ToString(); 
    } 

Answer 1

Użyć HtmlTextWriter zamiast StringBuilder napisać HTML:

StringBuilder sb = new StringBuilder(); 
using(HtmlTextWriter writer = new HtmlTextWriter(new StringWriter(sb))) 
{ 
    writer.WriteBeginTag("div"); 
    writer.WriteAttribute("class", "slide"); 
    //... 
} 
return sb.ToString(); 

nie chcemy użyć niestrukturalnych pisarza do napisania danych strukturalnych.

Przerwa ciało swojej wewnętrznej pętli do oddzielnego rutynowej:

foreach(...) 
{ 
    WritePost(post, writer); 
} 

//snip 

private void WritePost(Post post, HtmlTextWriter writer) 
{ 
    //write single post 
} 

To sprawia, że ​​łatwiej sprawdzalne i modyfikować.

Użyj struktury danych do zarządzania takimi obiektami, jak klasy CSS.

Zamiast dołączania dodatkowych nazw klas z przestrzeni i nadzieją linie wszystkiego na samym końcu, prowadź List<string> nazw klas, aby dodać lub usunąć w razie potrzeby, a następnie zadzwonić:

List<string> cssClasses = new List<string>(); 

//snip 

string.Join(" ", cssClasses.ToArray()); 

Answer 2

To nie jest wielki, ale widziałem wiele znacznie gorzej.

Zakładając, że jest to ASP.Net, czy istnieje powód, dla którego używasz tego podejścia zamiast repeatera?

EDIT:

Jeśli repeater nie jest możliwe w tym przypadku polecam użyciu klas HTML NET do generowania kodu HTML, a nie za pomocą tekstu. Jest to nieco łatwiejsze do odczytania/dostosowania później.

Answer 3

Pomógłby, gdyby istniała definicja postów, ale generalnie powiedziałbym, że generowanie kodu HTML za kodem nie jest rozwiązaniem w Asp.Net.

Ponieważ ta jest oznaczona jako Net konkretnie ...

do wyświetlania listy elementów w kolekcji, byłbyś lepiej patrząc na jednej z kontroli danych (Repeater, listy danych, etc) i nauczenie się, jak właściwie ich używać.

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

Answer 4

Widziałem dużo gorsze, ale można ją poprawić sporo.

1. Zamiast StringBuilder.Append() z String.Format() wewnątrz użyj StringBuilder.AppendFormat()
2. Dodaj kilka testów jednostkowych wokół niego, aby upewnić się, że produkujących wyjście chcesz, to byłaby wewnątrz kodu, aby być lepszym. Testy zapewnią, że niczego nie złamałeś.

Answer 5

Kolejny dokręcenie można wykonać: zastąpienie połączeń sb.Append(string.Format(...)) za pomocą sb.AppendFormat(...).

Answer 6

Zamiast tego

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

bym przenieść warunek wyjścia do pętli tak: (i wyeliminować niepotrzebne parametru rodzajowego gdy jesteś na nim)

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

jako Faktem jest, że twoje dwie zagnieżdżone pętle mogą prawdopodobnie być obsługiwane w jednej pętli.

Również wolałbym używać pojedynczych cudzysłowów dla atrybutów html, ponieważ są one legalne i nie wymagają ucieczki.

Answer 7

Moją pierwszą myślą jest to, że bardzo trudno byłoby przeprowadzić test jednostkowy.

Proponuję o drugiej pętli być oddzielną funkcję, więc trzeba będzie coś takiego:

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

i LoopPosts:

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

Jeśli masz w zwyczaju robić pętle tak jak wtedy, gdy trzeba wykonać test jednostkowy, łatwiej będzie przetestować wewnętrzną pętlę oddzielnie od zewnętrznej pętli.

Answer 8

I Powiedzmy, że wygląda wystarczająco dobrze, nie ma poważnych problemów z kodem i działałoby dobrze. Nie oznacza to jednak, że nie można go poprawić. :)

Oto kilka porad:

Dla ogólnych operacji zmiennoprzecinkowych należy użyć double zamiast Decimal. Jednak w tym przypadku nie trzeba żadnej arytmetyki zmiennoprzecinkowej w ogóle można to zrobić tylko z liczb całkowitych:

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

Ale jeśli wystarczy użyć jednej pętli nad wszystkich elementów zamiast pętli w sposób pętli, w ogóle nie potrzebujesz liczby slajdów.

Konwencja dla zmiennych lokalnych to wielbłąd (postCoint), a nie przypadek pascalowy (PostCount). Postaraj się, aby nazwy zmiennych były bardziej zrozumiałe niż ukryć skróty typu "sb". Jeśli zakres zmiennej jest tak mały, że tak naprawdę nie potrzebujesz do niej znaczącej nazwy, po prostu wybierz najprostszą możliwą i pojedynczą literę zamiast skrótu.

Zamiast łączenia łańcuchów "bloków slajdów" i "pierwszych" można bezpośrednio przypisać ciągi literowe. W ten sposób zastąpisz wywołanie String.Concat prostym zadaniem. (To graniczy z przedwczesną optymalizacją, ale z drugiej strony łączenie ciągów trwa około 50 razy dłużej.)

Możesz użyć .AppendFormat(...) zamiast .Append(String.Format(...)) na StringBuilderze. Po prostu trzymam się Append w tym przypadku, ponieważ nie ma naprawdę niczego, co wymaga formatowania, po prostu łączenie ciągów.

Tak, chciałbym napisać metodę tak:

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