-1
\$\begingroup\$

I have to correct this code without changing the structure. Cannot seem to figure out what's wrong.

missiles is an array of missiles.

DeleteAtIndex() takes the last object in the array and replaces the object at the index with it. And decrease the count of the array with one.

pos and pos of the missiles are vectors. All operators are defined for them. Length works on a Vector and returns the magnitude of the vector.

public void Defense::Update()
{
 float dist;
 for(int i = 0; i < missiles.Count(); i++)
 {
      dist = (pos – missiles[i].pos).Length();
      if( dist = detectionRadius )
      {
           CreateDeathEffect(missiles[i].pos);
           missiles.DeleteAtIndex(i);
      }
 }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ What's the symptom of the things that go wrong? What are we looking for? \$\endgroup\$ – Alexandre Vaillancourt May 14 at 20:35
2
\$\begingroup\$

You're cutting off the branch on which you're standing.

You're changing the size of the container over which you're iterating. Even if you replace the item at index with the last one, you'll be skipping that item because you increase i at the end of each loop.

To fix that, you could iterate over your missiles starting with the end of the collection:

for(int i = missiles.Count() -1; i>= 0; i--)
\$\endgroup\$
  • 1
    \$\begingroup\$ Alternatively, you could decrement i after the missile is deleted. \$\endgroup\$ – Alex F May 15 at 4:04
  • 1
    \$\begingroup\$ @AlexF Yes, this would work too, but going back and forth with a loop index may look weird and unnatural and add confusion when reading the code. This would certainly require a comment (in the code). \$\endgroup\$ – Alexandre Vaillancourt May 15 at 15:26
  • 1
    \$\begingroup\$ I think that's just a stylistic choice - to me, the loop index decrement indicates that the thing I'm iterating over has changed, while the loop that counts down looks weird. \$\endgroup\$ – Alex F May 15 at 23:59
2
\$\begingroup\$

I cannot guess what language do you use so I answer about generic c-like language. Also since you didn't write explicitly what's wrong I can't be sure what's wrong so I will guess.

Comparison is ==, not =. = is assignment and it will rewrite your variable and because it's inside if it will be implicitly cast to boolean.

Also you should not compare two floats with == or !=. Floating point operations are not perfectly accurate because of limited precision of the floating point number type. You remember old calculators: when you divide 1 by 3 and then multiply 0.333... by 3 the result will be 0.999..., not 1. They are not equal. The same rule applies there.

That's how you should compare them in most cases: if (abs(a-b) < 0.001) {...}

But your case is about radius where missiles explode, you probably should handle case when they are not on circle border, but inside, so simply change = to <.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.