Wie schlecht ist dieser Code?

Question

Ok, ich bin ein Amateur-Programmierer und schrieb das einfach. Es erledigt die Arbeit, aber ich frage mich, wie schlecht es ist und welche Verbesserungen möglich sind.

[Bitte beachten Sie, dass dies für Graffiti CMS eine Chalk Erweiterung.]

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

Verwenden Sie ein HtmlTextWriter anstelle eines String HTML schreiben:

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

Wir wollen nicht eine unstrukturierte Schriftsteller verwenden, strukturierte Daten zu schreiben.

Brechen Sie den Körper Ihrer inneren Schleife in einer separaten Routine:

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

//snip 

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

Dieses es testbar und leichter modifizierbar macht.

Verwenden Sie eine Datenstruktur für die Verwaltung von Dingen wie CSS-Klassen.

Statt mit einem Raum Extraklasse Namen anhängen und alle Linie der Hoffnung, bis ganz am Ende, halten Sie ein List<string> von Klassennamen wie nötig hinzuzufügen und zu entfernen, und dann rufen:

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

//snip 

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

Answer 2

Es ist nicht groß, aber ich habe viel, viel Schlimmeres gesehen.

Angenommen, dies ist ASP.Net, gibt es einen Grund, warum Sie diesen Ansatz anstelle eines Repeaters verwenden?

EDIT:

Wenn ein Repeater in diesem Fall nicht möglich ist, würde ich mit .NET HTML Klassen empfiehlt die HTML zu erzeugen, anstatt Text zu verwenden. Es ist ein wenig leichter zu lesen/später anzupassen.

Answer 3

Es würde helfen, wenn die Definition für Beiträge existiert, aber im Allgemeinen würde ich sagen, dass das Generieren von HTML in Code nicht der Weg ist, in Asp.Net zu gehen.

Da dies markiert als .Net speziell ...

Für eine Liste von Elementen in einer Sammlung anzuzeigen, würden Sie besser dran an einen der Datenkontrollen (Repeater, Datenliste, usw.) auf der Suche sein und lernen, diese richtig zu benutzen.

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

Answer 4

Ich habe viel schlechter gesehen, aber man konnte es durchaus ein wenig verbessern.

1. Statt StringBuilder.Append() mit einem string.Format() nach innen, verwenden StringBuilder.AppendFormat()
2. einige Unit-Tests hinzufügen, um es zu gewährleisten, dass sie den Ausgang der Herstellung Sie wollen, Refactoring dann die Code innen, um besser zu sein. Die Tests stellen sicher, dass Sie nichts kaputt gemacht haben.

Answer 5

Ein weiteres Stück Verschärfung könnten Sie tun: Ersetzen Sie die sb.Append(string.Format(...)) Anrufe mit sb.AppendFormat(...).

Answer 6

Statt dem,

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

ich die Ausgangsbedingung in die Schleife wie so bewegen würde: (und unnötigen allgemeinen Parameter zu eliminieren, während Sie gerade dabei sind)

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

Als Tatsächlich können Ihre zwei verschachtelten Schleifen wahrscheinlich in einer einzigen Schleife behandelt werden.

Auch würde ich lieber einfache Anführungszeichen für die HTML-Attribute verwenden, da diese legal sind und keine Entkommen erfordern.

Answer 7

Mein erster Gedanke ist, dass dies sehr schwierig sein würde, Unit-Test.

würde ich vorschlagen, die zweite for-Schleife eine eigene Funktion sein, die, so dass Sie so etwas wie haben würde:

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

und LoopPosts:

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

Wenn Sie Schleifen zu tun bekommen in eine Gewohnheit Wenn Sie dann einen Komponententest durchführen müssen, ist es einfacher, die innere Schleife getrennt von der äußeren Schleife zu testen.

Answer 8

I‘ Ich sage, dass es gut genug aussieht, es gibt keine ernsthaften Probleme mit dem Code und es würde gut funktionieren. Es bedeutet jedoch nicht, dass es nicht verbessert werden kann. :)

Hier sind ein paar Tipps:

Für allgemeine Gleitkommaoperationen Sie double statt Decimal verwenden sollten. Sie jedoch in diesem Fall keine Gleitkomma-Arithmetik brauchen überhaupt, können Sie dies mit nur ganzen Zahlen:

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

Aber, wenn Sie nur eine einzige Schleife über alle Elemente verwenden statt einer Schleife in einem Schleife, brauchen Sie die Anzahl der Folien überhaupt nicht.

Die Konvention für lokale Variablen ist camel case (postCoint) und nicht pascal case (PostCount). Versuchen Sie, Variablennamen bedeutungsvoller zu machen als obskure Abkürzungen wie "sb". Wenn der Gültigkeitsbereich einer Variablen so klein ist, dass Sie keinen aussagekräftigen Namen dafür benötigen, wählen Sie einfach das Einfachste und nur einen Buchstaben statt einer Abkürzung.

Anstatt die Strings "slide block" und "first" zu verketten, können Sie Literal Strings direkt zuweisen. Auf diese Weise ersetzen Sie einen Aufruf von String.Concat durch eine einfache Zuweisung. (Dies grenzt an vorzeitige Optimierung, aber auf der anderen Seite dauert die Verkettung der Zeichenfolge etwa 50 mal länger.)

Sie können .AppendFormat(...) anstelle von .Append(String.Format(...)) auf einem StringBuilder verwenden. Ich würde in diesem Fall einfach bei Append bleiben, da es nicht wirklich etwas gibt, das formatiert werden muss, Sie verketten nur Strings.

So würde ich die Methode wie folgt schreiben:

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