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

It's not great but I've seen much much worse.

Assuming this is ASP.Net, is there a reason you're using this approach instead of a repeater?

EDIT:

If a repeater isn't possible in this instance I would recommend using the .Net HTML classes to generate the HTML instead of using text. It's a little easier to read/adjust later on.

Answer 2

It would help if the definition for posts existed, but in general, I would say that generating HTML in code behind is not the way to go in Asp.Net.

Since this is tagged as .Net specifically...

For displaying a list of items in a collection, you'd be better off looking at one of the data controls (Repeater, Data List, etc) and learning how to use those properly.

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

Answer 3

I've seen much worse, but you could improve it quite a bit.

1. Instead of StringBuilder.Append() with a string.Format() inside, use StringBuilder.AppendFormat()
2. Add some unit tests around it to ensure that it's producing the output you want, then refactor the code inside to be better. The tests will ensure that you haven't broken anything.

Answer 4

Another piece of tightening you could do: replace the sb.Append(string.Format(...)) calls with sb.AppendFormat(...).

Answer 5

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.

Answer 6

My first thought is that this would be very difficult to unit test.

I would suggest having the second for loop be a separate function, so you would have something like:

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

and LoopPosts:

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

If you get into a habit of doing loops like this then when you need to do a unit test it will be easier to test the inner loop separate from the outer loop.

Answer 7

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 8

I'd say that it looks good enough, there is no serious problems with the code, and it would work well. It doesn't mean that it can't be improved, though. :)

Here are a few tips:

For general floating point operations you should use double rather than Decimal. However, in this case you don't need any floating point arithmetic at all, you can do this with just integers:

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

But, if you just use a single loop over all items instead of a loop in a loop, you don't need the slide count at all.

The convention for local variables is camel case (postCoint) rather than pascal case (PostCount). Try to make variable names meaningdful rather than obscure abbreviations like "sb". If the scope of a variable is so small that you don't really need a meaningful name for it, just go for the simplest possible and just a single letter instead of an abbreviation.

Instead of concatenating the strings "slide block" and " first" you can assign literal strings directly. That way you replace a call to String.Concat with a simple assignment. (This borders on premature optimisation, but on the other hand the string concatenation takes about 50 times longer.)

You can use .AppendFormat(...) instead of .Append(String.Format(...)) on a StringBuilder. I would just stick to Append in this case, as there isn't really anything that needs formatting, you are just concatenating strings.

So, I would write the method like this:

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