Quelle est la gravité de ce code?

Question

Ok, je suis un programmeur amateur et je viens de l'écrire. Cela fait le travail, mais je me demande à quel point c'est mauvais et quelles améliorations pourraient être apportées.

[S'il vous plaît noter que ceci est une extension pour Chalk 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

Utilisez un HtmlTextWriter au lieu d'un StringBuilder écrire en HTML:

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

Nous ne voulons pas utiliser un écrivain pour écrire des données non structurées structurées.

Briser le corps de la boucle intérieure dans une routine séparée:

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

//snip 

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

Cela rend testable et plus facilement modifiables.

Utilisez une structure de données pour gérer des éléments tels que les classes CSS.

au lieu d'ajouter les noms de classes supplémentaires avec un espace et en espérant que tout se aligne juste à la fin, garder un List<string> des noms de classe pour ajouter et supprimer au besoin, puis appelez:

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

//snip 

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

Answer 2

Ce n'est pas super mais j'ai vu bien bien pire.

En supposant que ce soit ASP.Net, existe-t-il une raison pour laquelle vous utilisez cette approche au lieu d'un répéteur?

EDIT:

Si un répéteur est impossible dans ce cas, je vous conseille d'utiliser les classes HTML .Net pour générer le code HTML au lieu d'utiliser le texte. C'est un peu plus facile à lire/ajuster plus tard.

Answer 3

Il serait utile si la définition pour les messages existait, mais en général, je dirais que générer du code HTML dans le code n'est pas le chemin à parcourir dans Asp.Net.

Depuis cela est spécifiquement étiqueté comme .Net ...

Pour afficher une liste d'éléments dans une collection, vous seriez mieux regarder l'un des contrôles de données (répéteur, liste de données, etc.) et apprendre à les utiliser correctement.

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

Answer 4

j'ai vu bien pire, mais vous pouvez l'améliorer un peu.

1. Au lieu de StringBuilder.Append() avec un string.Format() à l'intérieur, utilisez StringBuilder.AppendFormat()
2. Ajouter quelques tests unitaires autour d'elle pour faire en sorte que elle produit la sortie que vous voulez, alors la factoriser code à l'intérieur pour être meilleur. Les tests s'assureront que vous n'avez rien cassé.

Answer 5

Un autre serrage que vous pourriez faire: remplacer les appels sb.Append(string.Format(...)) par sb.AppendFormat(...).

Answer 6

Au lieu de cela,

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

Je rapprocheront l'état de sortie dans la boucle comme ceci: (et élimine paramètre générique inutile pendant que vous y êtes)

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

En En fait, vos deux boucles imbriquées peuvent probablement être traitées en une seule boucle.

En outre, je préférerais utiliser des guillemets simples pour les attributs html, car ceux-ci sont légaux et ne nécessitent pas d'échappement.

Answer 7

Ma première pensée est que cela serait très difficile à tester unitaire.

Je suggère d'avoir la deuxième boucle une fonction séparée, de sorte que vous auriez quelque chose comme:

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

et LoopPosts:

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

Si vous entrez dans une habitude de boucles faire comme ceci, alors quand vous devez faire un test unitaire, il sera plus facile de tester la boucle interne séparément de la boucle externe.

Answer 8

Je d disons que cela semble assez bon, il n'y a pas de problèmes sérieux avec le code, et cela fonctionnerait bien. Cela ne signifie pas qu'il ne peut pas être amélioré, cependant. :)

Voici quelques conseils:

Pour les opérations générales à virgule flottante, vous devez utiliser double plutôt que Decimal. Cependant, dans ce cas, vous ne avez pas besoin de l'arithmétique en virgule flottante du tout, vous pouvez le faire avec seulement entiers:

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

Mais, si vous venez d'utiliser une seule boucle sur tous les éléments au lieu d'une boucle dans un boucle, vous n'avez pas besoin du nombre de diapositives du tout.

La convention pour les variables locales est le cas chameau (postCoint) plutôt que le cas pascal (PostCount). Essayez de rendre les noms de variables significatifs plutôt que des abréviations obscures comme "sb". Si la portée d'une variable est si petite que vous n'avez pas vraiment besoin d'un nom significatif, optez pour le plus simple possible et juste une seule lettre au lieu d'une abréviation. Au lieu de concaténer les chaînes "bloc coulissant" et "premier", vous pouvez affecter des chaînes littérales directement. De cette façon, vous remplacez un appel à String.Concat avec une affectation simple. (Cela frise l'optimisation prématurée, mais d'un autre côté, la concaténation des chaînes prend environ 50 fois plus longtemps.)

Vous pouvez utiliser .AppendFormat(...) au lieu de .Append(String.Format(...)) sur un StringBuilder. Je voudrais juste coller à Append dans ce cas, car il n'y a pas vraiment tout ce qui a besoin de mise en forme, vous êtes juste en train de concaténer des chaînes.

Alors, je voudrais écrire la méthode comme ceci:

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