这个代码有多糟糕？

Question

好吧,我是业余程序员,只是写了这个.它完成了工作,但我想知道它有多糟糕,可以做出哪些改进.

[请注意,这是Graffiti CMS的Chalk扩展.]

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

使用HtmlTextWriter而不是StringBuilder来编写HTML:

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

我们不想使用非结构化编写器来编写结构化数据.

将内循环的主体分解为单独的例程:

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

//snip

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

这使得它可以测试并且更容易修改.

使用数据结构来管理CSS类之类的东西.

而不是使用空格附加额外的类名并希望所有内容在最后排成一行,保留一个List<string>类名以根据需要添加和删除,然后调用:

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

//snip

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

Answer 2

这不是很好但我看到的情况要糟糕得多.

假设这是ASP.Net,你有没有理由使用这种方法而不是转发器？

编辑:

如果在这种情况下无法使用转发器,我建议使用.Net HTML类来生成HTML而不是使用文本.稍后阅读/调整会更容易一些.

Answer 3

而不是这个,

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

我将退出条件移动到循环中,如下所示:(并在您使用时删除不必要的泛型参数)

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

事实上,您的两个嵌套循环可能在一个循环中处理.

此外,我更喜欢使用单引号作为html属性,因为这些是合法的,不需要转义.