Quanto è pericoloso questo codice?

Question

Ok, sono un programmatore dilettante e ho appena scritto questo. Ottiene il lavoro fatto, ma mi chiedo quanto sia grave e che tipo di miglioramenti potrebbero essere fatti.

[Si prega di notare che questa è un'estensione del gesso a 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

Utilizzare un HtmlTextWriter invece di uno StringBuilder per scrivere HTML:

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

Noi non vogliamo usare uno scrittore non strutturato per scrivere i dati strutturati.

Rompere il corpo del vostro ciclo interno in una separata routine di:

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

//snip 

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

Questo rende controllabile e più facilmente modificabili.

Utilizzare una struttura dati per la gestione di cose come le classi CSS.

Invece di aggiungendo nomi delle classi in più con uno spazio e sperando linee tutto fino proprio alla fine, mantenere un List<string> di nomi di classe per aggiungere e rimuovere, se necessario, e quindi chiamare:

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

//snip 

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

Answer 2

Non è grande, ma ho visto molto molto peggio.

Supponendo che questo sia ASP.Net, c'è un motivo per cui stai usando questo approccio al posto di un ripetitore?

EDIT:

Se un ripetitore non è possibile in questo caso mi consiglia di utilizzare le classi HTML Net per generare il codice HTML invece di utilizzare il testo. È un po 'più facile da leggere/regolare in seguito.

Answer 3

Sarebbe utile se esistesse la definizione di post, ma in generale, direi che generare codice HTML nel codice non è la strada da percorrere in Asp.Net.

Dal momento che questo è etichettato come .Net specificamente ...

Per visualizzare un elenco di elementi in una collezione, si sarebbe meglio guardare uno dei controlli di dati (ripetitore, Lista dei dati, ecc) e imparare come usarli correttamente.

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

Answer 4

ho visto molto peggio, ma si potrebbe migliorare un po '.

1. Invece di StringBuilder.Append() con uno string.Format() all'interno, utilizzare StringBuilder.AppendFormat()
2. aggiungere alcuni test di unità intorno ad esso per garantire che essa sta producendo l'output desiderato, quindi refactoring del codice interno per essere migliore. I test garantiranno che non hai rotto nulla.

Answer 5

Un altro elemento di serraggio che è possibile eseguire: sostituire le chiamate sb.Append(string.Format(...)) con sb.AppendFormat(...).

Answer 6

Invece di questo,

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

mi piacerebbe spostare la condizione di uscita nel circuito in questo modo: (ed eliminare parametro generico inutili mentre si è in esso)

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

Come In pratica, i tuoi due cicli annidati possono probabilmente essere gestiti in un unico ciclo.

Inoltre, preferirei utilizzare le virgolette singole per gli attributi html, poiché sono legali e non richiedono l'escape.

Answer 7

Il mio primo pensiero è che questo sarebbe molto difficile da testare.

vi suggerisco di avere il secondo ciclo for essere una funzione separata, così si avrebbe qualcosa come:

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

e LoopPosts:

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

Se si entra in un abitudine di fare loop in questo modo, quando è necessario eseguire un test dell'unità, sarà più facile testare il ciclo interno separato dal circuito esterno.

Answer 8

I' D dire che sembra abbastanza buono, non ci sono problemi seri con il codice, e funzionerebbe bene. Ciò non significa che non possa essere migliorato, però. :)

Ecco alcuni suggerimenti:

Per le operazioni in virgola mobile generali si dovrebbe usare double piuttosto che Decimal. Tuttavia, in questo caso non è necessario alcun aritmetica in virgola mobile a tutti, si può fare questo con solo numeri interi:

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

Ma, se si utilizza un singolo ciclo su tutti gli elementi al posto di un ciclo in un loop, non è necessario il conteggio delle diapositive.

La convenzione per le variabili locali è il caso cammello (postCoint) piuttosto che il caso Pascal (PostCount). Cerca di rendere i nomi variabili significati piuttosto che le abbreviazioni oscure come "sb". Se l'ambito di una variabile è così piccolo che non hai davvero bisogno di un nome significativo, vai semplicemente per il più semplice possibile e solo una singola lettera invece di un'abbreviazione.

Invece di concatenare le stringhe "slide block" e "first" è possibile assegnare direttamente le stringhe letterali. In questo modo sostituisci una chiamata a String.Concat con un compito semplice. (Ciò confina con l'ottimizzazione prematura, ma d'altra parte la concatenazione di stringhe richiede circa 50 volte di più.)

È possibile utilizzare .AppendFormat(...) anziché .Append(String.Format(...)) su un oggetto StringBuilder. Mi limiterò a Append in questo caso, poiché non c'è davvero nulla che abbia bisogno di formattazione, si stanno semplicemente concatenando stringhe.

Quindi, vorrei scrivere il metodo come questo:

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