17

Steve McConnell のチェックリスト項目の 1 つは、ループ インデックスを操作しないことです(第 16 章、25 ページ、ループ インデックス、PDF 形式)。

これは直感的に理解できるものであり、昔プログラミングの方法を学んだときを除いて、私がいつも守ってきた方法です。

最近のコード レビューで、私はこのぎこちないループを発見し、すぐに疑わしいとフラグを立てました。

        for ( int i=0 ; i < this.MyControl.TabPages.Count ; i++ )
        {
            this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] );
            i--;
        }

すべての TabPages が削除されるまでインデックスを 0 に保つことで機能するので、ほとんど面白いです。

このループは次のように記述できます。

        while(MyControl.TabPages.Count > 0)
            MyControl.TabPages.RemoveAt(0);

そして、コントロールは実際にはループとほぼ同時に書かれたので、次のように書かれた可能性さえあります

        MyControl.TabPages.Clear();

それ以来、私はコード レビューの問題について異議を唱えられてきましたが、なぜそれが悪い習慣なのかについての私の明確な説明は、私が望んでいたほど強力ではなかったことがわかりました。ループの流れを理解するのが難しく、そのため保守とデバッグが難しくなり、最終的にはコードの存続期間中により多くの費用がかかると言いました。

これが悪い習慣である理由をより明確に説明するものはありますか?

4

10 に答える 10

24

あなたのアーティキュレーションは素晴らしいと思います。多分それはそのように言うことができます：

ロジックはより明確に表現できるので、そうすべきです。

于 2009-01-19T09:47:30.870 に答える
23

まあ、これはほとんど目的のない混乱を招きます - あなたは同じように簡単に書くことができます:

while(MyControl.TabPages.Count > 0)
{
    MyControl.TabPages.Remove(MyControl.TabPages[0]);
}

または（簡単）

while(MyControl.TabPages.Count > 0)
{
    MyControl.TabPages.RemoveAt(0);
}

または（最も単純な）

MyControl.TabPages.Clear();

上記のすべてにおいて、極端なケースについて目を細めて考える必要はありません。いつ何が起こるかは明らかです。ループ インデックスを変更している場合は、一目で理解するのが非常に難しくなります。

于 2009-01-19T09:47:15.143 に答える
16

それはすべて期待についてです。

ループカウンターを使用する場合、ループの反復ごとに同じ量でインクリメント（デクリメント）されることが期待されます。

ループカウンターを台無しにした場合（または必要に応じてサル）、ループは期待どおりに動作しません。これは、理解が難しく、コードが誤って解釈される可能性が高くなることを意味し、これによりバグが発生します。または、賢明だが架空の人物を（誤って）引用するには：

complexity leads to misunderstanding

misunderstanding leads to bugs

bugs leads to the dark side.
于 2009-01-19T09:57:41.753 に答える
3

私はあなたの挑戦に同意します。forループを維持したい場合は、次のコードを使用します。

for ( int i=0 ; i < this.MyControl.TabPages.Count ; i++ ) {
    this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] );
    i--;
}

次のように減少します：

for ( int i=0 ; i < this.MyControl.TabPages.Count ; ) {
    this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] );
}

そして：

for ( ; 0 < this.MyControl.TabPages.Count ; ) {
    this.MyControl.TabPages.Remove ( this.MyControl.TabPages[0] );
}

ただし、whileループまたはClear（）メソッドが存在する場合は、それが明らかに望ましいです。

于 2009-01-19T10:12:37.790 に答える
2

クヌースの文芸的プログラミングの概念を呼び出すことで、より強力な議論を構築できると思います。プログラムはコンピューター用に作成するのではなく、他のプログラマーに概念を伝達するため、つまりループを単純化する必要があります。

 while (this.MyControl.TabPages.Count>0)
 {
            this.MyControl.TabPages.Remove ( this.MyControl.TabPages[0] );
 }

意図をより明確に示しています-残りがなくなるまで最初のタブページを削除します。ほとんどの人は、元の例よりもはるかに速く成長すると思います。

于 2009-01-19T09:50:08.493 に答える
1

これはより明確かもしれません：

while (this.MyControl.TabPages.Count > 0)
{
  this.MyControl.TabPages.Remove ( this.MyControl.TabPages[0] );
}
于 2009-01-19T09:51:52.140 に答える
1

使用できる議論の1つは、インデックスが2回変更されるようなコードをデバッグするのははるかに難しいということです。

于 2009-01-19T09:52:13.247 に答える
1

元のコードは、forループのアクションを必要なものに曲げるために非常に冗長です。インクリメントは不要で、デクリメントによってバランスが取られます。概念的にはポストインクリメントが間違っているため、これらはPOSTインクリメントではなく、PREインクリメントである必要があります。タブページ数との比較は、コンテナが空であることを確認するためのハックな方法であるため、半冗長です。

要するに、それは不必要な賢さであり、冗長性を取り除くのではなく追加します。それは明らかに単純であると同時に明らかに短い可能性があるので、それは間違っています。

于 2009-01-19T10:03:28.153 に答える
1

インデックスを気にする唯一の理由は、何かを選択的に消去する場合です。その場合でも、次のように言うのが望ましいと思います。

  i=0;
  while(i < MyControl.Tabpages.Count)
    if (wantToDelete(MyControl.Tabpages(i))
      MyControl.Tabpages.RemoveAt(i);
    そうしないと
      i++;
削除するたびにループ インデックスをジンクスするのではなく、または、さらに良いのは、アイテムが削除されたときに、削除が必要な将来のアイテムのインデックスに影響を与えないように、インデックス カウントを下げることです。多くのアイテムが削除された場合、これは、各削除後にアイテムを移動するのにかかる時間を最小限に抑えるのにも役立ちます。

于 2010-07-12T22:11:59.390 に答える
0

ループの反復が、誰もが予想するような「i ++」ではなく、クレイジーな「i--」セットアップによって制御されているという事実を指摘するだけで十分だったと思います。

また、カウントを評価して「i」の状態を変更してから、ループ内のカウントを変更すると、潜在的な問題が発生する可能性があると思います。forループには通常、「固定」の反復回数があり、forループ条件の唯一の部分がループ変数「i」に変わると思います。

于 2009-01-19T10:06:25.590 に答える