_currentTier = 0;
while (_currentTier < _tierCount) //_tierCount will start at 1 but keep increasing because child objects will keep adding more tiers.
{
foreach (Widget w in _tiers[_currentTier])
{
w.DoMyThing();
}
_currentTier++;
}
_currentTier = 0;
while (_currentTier < _tierCount) //_tierCount will start at 1 but keep increasing because child objects will keep adding more tiers.
{
foreach (Widget w in _tiers[_currentTier])
{
w.DoMyThing();
}
_currentTier++;
}
You are iterating over a collection as it is changing. I mean the first iteration, not the second. You're obviously accounting for the change (hence the < _tierCount rather than a standard foreach) but it's still a smell, IMO.
Would I let it into production code? Possibly. Depends on the scenario. But I'd feel dirty about it.
Also: Your _tiers member could just as easily be a List<List<Widget>>.
The biggest potential issue here is that the Sweep method is iterating over a collection (_tiers) that could potentially change in a call to Widget.DoMyThing().
The .NET BCL classes do not allow collection to change while they are being iterated. The code the way it's structured, exposes a risk that this may happen.
Beyond that, the other problem is that the structure of the program makes it difficult to understand what happens in what order. Perhaps you could separate the phase of the program that recursively assembles the model from the portion that visits the model and performs computations.
However, I gave it some thought and I think I know why this smells. The pattern as I have implemented seems to be some kind of perversion of the Builder pattern. What would work better is creating a Composite out of a series of analytical steps which, taken as a whole, represents some kind of meaningful state. Each step of the analytical process (behavior) should be distinct from the output Composite (state). What I've implemented above meshes both the state and the behavior together. Since the state-holders and state-analyzers are one and the same object, this also violates the Single Responsibility Principle. The approach of having a composite "build itself" opens up the possibility of creating a vicious loop, even though my prototype above has a deterministic completion.
发布评论
评论（3）
是的，我将以下代码称为气味：
您正在迭代正在更改的集合。我的意思是第一次迭代，而不是第二次。显然，您正在考虑更改（因此是
<_tierCount，而不是标准的foreach），但在我看来，它仍然是一种味道。我会把它放入生产代码中吗？可能吧。取决于场景。但我会觉得很肮脏。
另外：您的
_tiers成员也可以轻松成为List> 。Yes, I call the following code smell:
You are iterating over a collection as it is changing. I mean the first iteration, not the second. You're obviously accounting for the change (hence the
< _tierCountrather than a standardforeach) but it's still a smell, IMO.Would I let it into production code? Possibly. Depends on the scenario. But I'd feel dirty about it.
Also: Your
_tiersmember could just as easily be aList<List<Widget>>.这里最大的潜在问题是
Sweep方法正在迭代集合 (_tiers)，该集合可能会在调用Widget.DoMyThing()..NET BCL 类不允许集合在迭代时发生更改。代码的结构方式暴露了发生这种情况的风险。
除此之外，另一个问题是程序的结构使得很难理解按什么顺序发生的事情。也许您可以将递归组装模型的程序阶段与访问模型并执行计算的部分分开。
The biggest potential issue here is that the
Sweepmethod is iterating over a collection (_tiers) that could potentially change in a call toWidget.DoMyThing().The .NET BCL classes do not allow collection to change while they are being iterated. The code the way it's structured, exposes a risk that this may happen.
Beyond that, the other problem is that the structure of the program makes it difficult to understand what happens in what order. Perhaps you could separate the phase of the program that recursively assembles the model from the portion that visits the model and performs computations.
兰多夫和LBushkin都+1。
不过，我想了想，我想我知道为什么会有这种味道。我所实现的模式似乎是 Builder 模式的某种变形。更好的方法是通过一系列分析步骤创建一个组合，这些步骤作为一个整体代表某种有意义的状态。分析过程（行为）的每个步骤都应与输出复合（状态）不同。我上面实现的将状态和行为结合在一起。由于状态持有者和状态分析者是同一个对象，这也违反了单一责任原则。尽管我上面的原型具有确定性的完成，但复合“构建自身”的方法开启了创建恶性循环的可能性。
链接：
构建器模式
复合模式
+1 to both Randolpho and LBushkin.
However, I gave it some thought and I think I know why this smells. The pattern as I have implemented seems to be some kind of perversion of the Builder pattern. What would work better is creating a Composite out of a series of analytical steps which, taken as a whole, represents some kind of meaningful state. Each step of the analytical process (behavior) should be distinct from the output Composite (state). What I've implemented above meshes both the state and the behavior together. Since the state-holders and state-analyzers are one and the same object, this also violates the Single Responsibility Principle. The approach of having a composite "build itself" opens up the possibility of creating a vicious loop, even though my prototype above has a deterministic completion.
Links:
Builder Pattern
Composite Pattern