9

好的，我是一个业余程序员，刚刚写了这个。它完成了工作，但我想知道它有多糟糕以及可以做出什么样的改进。

[请注意，这是 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();
    }
4

8 回答 8

12

使用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());
于 2009-10-15T04:48:52.417 回答
5

这不是很好，但我见过更糟糕的情况。

假设这是 ASP.Net，您是否有理由使用这种方法而不是中继器？

编辑：

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

于 2009-10-15T04:37:25.780 回答
5

取而代之的是，

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

我会像这样将退出条件移动到循环中：（并在您使用时消除不必要的通用参数）

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

事实上，您的两个嵌套循环可能可以在一个循环中处理。

另外，我更喜欢对 html 属性使用单引号，因为这些是合法的并且不需要转义。

于 2009-10-15T04:41:37.387 回答
2

我见过更糟糕的情况，但你可以改进很多。

  1. 使用 StringBuilder.AppendFormat() 而不是内部带有 string.Format() 的 StringBuilder.Append()
  2. 围绕它添加一些单元测试以确保它产生您想要的输出，然后重构内部代码以使其更好。测试将确保您没有破坏任何东西。
于 2009-10-15T04:38:45.870 回答
2

我的第一个想法是这将很难进行单元测试。

我建议让第二个 for 循环是一个单独的函数，所以你会有类似的东西：

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

和 LoopPosts：

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

如果你养成了这样循环的习惯，那么当你需要进行单元测试时，将内循环与外循环分开测试会更容易。

于 2009-10-15T04:42:39.427 回答
1

我会说它看起来足够好，代码没有严重的问题，而且效果很好。但这并不意味着它无法改进。:)

这里有一些提示：

对于一般浮点运算，您应该使用double而不是Decimal. 但是，在这种情况下，您根本不需要任何浮点运算，只需使用整数即可：

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

但是，如果您只对所有项目使用单个循环而不是循环中的循环，则根本不需要幻灯片计数。

局部变量的约定是驼峰式 (postCoint) 而不是帕斯卡式 (PostCount)。尝试使变量名有意义，而不是像“sb”这样晦涩难懂的缩写。如果一个变量的范围很小，以至于你真的不需要一个有意义的名字，就选择最简单的，一个字母而不是缩写。

您可以直接分配文字字符串，而不是连接字符串“slide block”和“first”。这样你就可以用一个简单的赋值替换对 String.Concat 的调用。（这接近于过早的优化，但另一方面，字符串连接需要大约 50 倍的时间。）

您可以在 StringBuilder 上使用.AppendFormat(...)而不是。.Append(String.Format(...))在这种情况下我会坚持 Append ，因为实际上没有任何需要格式化的东西，你只是连接字符串。

所以，我会写这样的方法：

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();
}
于 2009-10-15T06:58:44.100 回答
0

如果帖子的定义存在会有所帮助，但总的来说，我会说在后面的代码中生成 HTML 不是在 Asp.Net 中的方式。

由于这被标记为 .Net 特别...

要显示集合中的项目列表，您最好查看其中一个数据控件（Repeater、Data List 等）并学习如何正确使用它们。

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

于 2009-10-15T04:38:35.573 回答
0

您可以做的另一件事是：将sb.Append(string.Format(...))调用替换为sb.AppendFormat(...).

于 2009-10-15T04:39:57.163 回答