How Bad is this Code?

Question

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

Answer 1

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

Answer 2

Instead of this,

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

I'd move the exit condition into the loop like so: (and eliminate unnecessary generic parameter while you're at it)

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

As a matter of fact, your two nested loops can probably be handled in one single loop.

Also, I'd prefer to use single quotes for the html attributes, since those are legal and don't require escaping.