2016-08-29 4 views
0

このコードを書くためのより効率的な方法があり、ここで 私はタイトルはそれをすべて言うと思います

はこれと非常によく似たコード:)

private void listBoxComponents_MouseDoubleClick(object sender, MouseEventArgs e) 
    { 
     object item = listBoxComponents.SelectedItem; 
     string name = listBoxComponents.GetItemText(item); 

     if (e.Button == MouseButtons.Left) 
     { 
      for (int i = 0; i < terrains.Count; i++) 
      { 
       if (terrains[i].name == name) 
        terrains[i].EnableBoundingBox(true); 
       else 
        terrains[i].EnableBoundingBox(false); 
      } 
     } 
    } 

私はその全体のコードを割り当てるきているのです私はちょうど彼らのことをもっと素早く同じことをacomplishするためにもっと早くやりたいと思っています。たぶんLINQと?

ありがとうございました。

+0

'地形 '? Enumerableですか? –

+0

これは一般的なリストです – tdkr80

+0

@ tdkr80ここにこの質問を投稿することを検討してください:http://codereview.stackexchange.com/ – random

答えて

1

コード自体は問題ありません。 LINQは、コード文字が少なくても、効率的であるとは限りません(何か追加のオーバーヘッドがありますが、通常はILコンパイル時に取り除かれ、従来のブロック/ループより読みにくくなりがちです) 。

LINQを使用している場合は、とにかく書いたものに似たものにコンパイルされてしまいます。実際は、それは単なる砂糖/コードキャンディです。

コードは読み込み可能で、ループ内でリソースを無駄にしないでください。他に何が欲しいですか?

しかし、私は個人的には、イベントハンドラからそのUIロジックをそれ自身のメソッドに分解します。

3

あなたの質問は、動作するようにcodereview SOサイトに適しているかもしれませんが、最適化してください。

terrainsの別のデータ構造を調べることをお勧めします。これを配列として使用することはあまり効率的ではありません。 nameプロパティで辞書を作成すると、特定の名前の地形に対して一定の繰り返しを避けることができます。

更新:私の最初の提案には欠陥があるため、別のものを思いついています。

私はデータ構造を変更することを考えています。 TerrainListを作成して、この例ではバウンディングボックスの有効化機能を公開し、アプリケーション全体に地形処理コードを広げることを避ける必要があります。

if (e.Button == MouseButtons.Left) 
    { 
     terrains.highlight(name); 
    } 

highlight機能の実装は、あなたのコードの他の部分への副作用なしで最適化することができます。

あなたのコードは次のようになります。

+0

私はあなたの意見に同意します。 彼のコメントでは、彼は配列ではなく一般的なリストだと言った。 – Marv

+0

なぜ辞書はここでうまくいくのですか?それ以外のすべての場合には 'false'が設定されます。 – Maarten

+0

@Maartenあなたは本当に正しいです、私は明らかに 'for'ループで読むことを止めました。私はその提案を言い換えるつもりです。 – thst

6

あなたは「より効率的」の意味を正確に述べていないので、私はただ、「読みやすいとタイプ」としてこれを解釈する自由を取るだろう。

あなたのループを置き換えることができます。

一般的に、 foreachループが少しおそらく以下であることを

foreach (var terrain in terrains) 
{ 
    terrain.EnableBoundingBox(terrain.name == name); 
} 

注:このいずれかで

for (int i = 0; i < terrains.Count; i++) 
{ 
    if (terrains[i].name == name) 
     terrains[i].EnableBoundingBox(true); 
    else 
     terrains[i].EnableBoundingBox(false); 
} 

実行時に効率的にループをforループよりも有効にします。しかし、その違いはおそらく、極端なリアルタイム/高性能シナリオを除いては、ごくわずかなことであり、心配することはまったくありません。肯定的な面では、はるかに単純なソースコードで終わります。

ちなみに、string.Equals(string, string, StringComparison)メソッドを使用して文字列を比較する方が良いかもしれません。これは比較のために文化と大文字と小文字の区別をどのように使うべきかをより明確にするためです。

別の言い方をすると、LINQについて言及しました。 LINQはここではうまくフィットしません。理由を簡単に説明しましょう:理論的には、デリゲートを受け入れるカスタムLINQ演算子ForEachを記述することは可能です。ソースシーケンス内の各項目のデリゲートを呼び出すforeachループとして実装できます。それは、コードの行以上を占めるにもかかわらず、平野foreachループが実際より読みやすいです、私の意見では

terrains.ForEach(terrain => terrain.EnableBoundingBox(terrain.name == name)); 

:あなたは、その後、次のワンライナーを書くことができます。しかし、最も重要なのは、LINQの略語である「言語統合クエリ」を覚えておいてください。 LINQ式はクエリのデータではなく、を変更してです(副作用がある)。しかし、カスタムオペレータはすべて副作用に関するものです。それがLINQのパラダイムに適合せず、Frameworkに含まれていない理由です。のデータ型が何であるかを

(最終発言として、私の答えは基本的にコードレビューである。これは、あなたが希望するものであれば、おそらくCode Review SEサイトは、あなたの質問を投稿するために、より適切な場所だったかもしれない。)

+1

これはおそらく、ブランチを含まないため、低レベルのパフォーマンスの観点からはさらに効率的です。しかし、EnableBoundingBoxの背後にあるコードは、とにかくランタイムを支配する可能性が高い:D – stefan

+0

名前は文字列であり、EnableBoundingBoxはboolをとる。 – tdkr80

+0

@ tdkr80:文字列を '=='や 'string.Equals'と比較すると' bool'が返されますが、もちろんこれは動作します。 – stakx

関連する問題