How Bad is this Code?

.net, asp.net, c#, graffiticms

Solution

Use an HtmlTextWriter instead of a StringBuilder to write HTML:

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

We don't want to use an unstructured writer to write structured data.

Break the body of your inner loop into a separate routine:

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

//snip

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

This makes it testable and more easily modifiable.

Use a data structure for managing things like CSS classes.

Instead of appending extra class names with a space and hoping everything lines up right at the end, keep a `List<string>` of class names to add and remove as necessary, and then call:

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

//snip

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

Problem

Ok, I am an amateur programmer and just wrote this. It gets the job done, but I am wondering how bad it is and what kind of improvements could be made. [Please note that this is a Chalk extension for 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(); } ```

Original source