9

わかりました、私はアマチュア プログラマーで、これを書きました。それは仕事を成し遂げますが、それがどれほど悪いのか、どのような改善ができるのか疑問に思っています.

[これは 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();
    }
4

8 に答える 8

12

StringBuilder の代わりにHtmlTextWriterを使用して 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))
        {
            // ...
        }

実際のところ、ネストされた 2 つのループは、おそらく 1 つのループで処理できます。

また、HTML 属性には一重引用符を使用することをお勧めします。これは正当であり、エスケープする必要がないためです。

于 2009-10-15T04:41:37.387 に答える
2

私はもっ​​と悪いことを見てきましたが、あなたはそれをかなり改善することができます.

  1. 内部に string.Format() を含む StringBuilder.Append() の代わりに、StringBuilder.AppendFormat() を使用します。
  2. その周りにいくつかの単体テストを追加して、必要な出力が生成されていることを確認してから、内部のコードをリファクタリングして改善します。テストにより、何も壊れていないことが確認されます。
于 2009-10-15T04:38:45.870 に答える
2

私の最初の考えは、これは単体テストが非常に難しいということです。

2番目の for ループを別の関数にすることをお勧めします。したがって、次のようになります。

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

およびループポスト:

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

このようなループを実行する習慣を身につけると、単体テストを実行する必要があるときに、外側のループとは別に内側のループをテストする方が簡単になります。

于 2009-10-15T04:42:39.427 に答える
1

見た目は十分で、コードに重大な問題はなく、うまく機能すると思います。とはいえ、改善できないわけではありません。:)

いくつかのヒントを次に示します。

一般的な浮動小数点演算では、doubleではなくを使用する必要がありますDecimal。ただし、この場合、浮動小数点演算はまったく必要ありません。整数だけでこれを行うことができます。

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

ただし、ループ内のループではなく、すべてのアイテムに対して単一のループを使用するだけの場合は、スライド カウントはまったく必要ありません。

ローカル変数の規則は、パスカル ケース (PostCount) ではなくキャメル ケース (postCoint) です。「sb」のようなあいまいな省略形ではなく、変数名を意味のあるものにするようにしてください。変数のスコープが非常に小さいため、意味のある名前を実際に必要としない場合は、省略形ではなく、可能な限り単純な 1 文字だけを使用してください。

文字列 "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 など) の 1 つを見て、それらを適切に使用する方法を学習することをお勧めします。

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 に答える