4
\$\begingroup\$

I am working with an object that needs to run an ExecuteEffect() method. Sometimes, the ExecuteEffect() only needs to run once, while in other cases, the ExecuteEffect() needs to keep running once / frame until some criteria is met (in my example, a real-time delay).

To solve this, I used an IEnumerator:

public override IEnumerator<bool> ExecuteEffect()
    {
        if(delay > 0)
        {
            if(executionStart == -1m)
            {
                executionStart = GetRealTime();
                yield return false;
            }
            if(GetRealTime() > executionStart + delay)
            {
                DealDamage();
                yield return true;
            }
            else
            {
                yield return false;
            }
        }
        else
        {
            DealDamage();
            yield return true;
        }
  }

Then, when I'm running ExecuteEffect(), if the IEnumerator<bool> returns false, it gets added to a list of objects that is continuously checked until their ExecuteEffect() method returns true.

GenericEffect ge = GetEffect();
IEnumerator<bool> temp = ge.ExecuteEffect();
temp.MoveNext();
if(!temp.Current)
{
    //Foo not finished, add to list of active Foo
    activeEffects.Add(ge);
}

and, elsewhere:

activeEffects.RemoveAll( s =>
                         {
                               IEnumerator<bool> temp = s.ExecuteEffect();
                               temp.MoveNext();
                               if(temp.Current)
                               {
                                   return true;
                               }
                               else
                               {
                                  return false;
                               }
                          });

This all feels very wordy, especially the temp variable to hold the IEnumerator<bool> and the explicit call to GetNext(). And, of course, I have no idea if this is an utter butchering of the intended uses of IEnumerator<T>.

Is there a better way to get this kind of effect? Or is this just an unconventional (albeit valid) use of IEnumerator<T>?

\$\endgroup\$
  • \$\begingroup\$ Could you add more context on how do you use this method? Because, lemme tell you, that's a wrong way to do it. It's quite unclear what's happening. But it's hard to propose something else without full context :) \$\endgroup\$ – IEatBagels Oct 19 '15 at 13:47
  • \$\begingroup\$ @TopinFrassi Sure. It's from a game. The player casts a "Spell", which has 1 to many GenericEffect objects. When the spell is cast, each GenericEffect has "ExecuteEffect" called. Some GenericEffects are "one and done", while others need to delay execution until some requirement and are added to a list of "ActiveEffects". Each frame, the game iterates over its list of ActiveEffects and calls ExecuteEffect again, removing any effects that have (self-reported) to have finished executing. That help? \$\endgroup\$ – Sable Dreamer Oct 19 '15 at 14:02
  • 1
    \$\begingroup\$ Well, it sure is an interesting use of IEnumerator<T> at least. \$\endgroup\$ – RubberDuck Oct 19 '15 at 16:19
2
\$\begingroup\$

You could make life slightly easier for yourself by using the iterator slightly more properly:

public override IEnumerator<bool> ExecuteEffect()
{
    if(delay < 0 || GetRealTime() > executionStart + delay)
    {
        DealDamage();
        yield break;
    }
    if(executionStart == -1m)
    {
        executionStart = GetRealTime();
    }
    yield return false;
}

Then you could actually use MoveNext 'correctly' as it will return false when you get to the end.

To be completely and brutally honest, I'd still rage erase all of this code if I found it in a project I worked on. Why is it an enumerator of booleans? None of it's particularly obvious.

As an aside:

if(temp.Current)
{
    return true;
}
else
{
    return false;
}

is 100% the same as

return temp.Current;
\$\endgroup\$
  • \$\begingroup\$ Thank you, valid points, all. I'm not sold on the IEnumerator<bool> -- I used it because I was under the (mistaken) impression that MoveNext() returned Current. \$\endgroup\$ – Sable Dreamer Oct 19 '15 at 14:32
  • 1
    \$\begingroup\$ @SableDreamer - As you're returning true or false why not just use a method IsActive()? \$\endgroup\$ – RobH Oct 19 '15 at 14:44
  • \$\begingroup\$ The goal is to make use of the yield statements. If I ignore the return value, the code would become ExecuteEffect(); if(IsActive() {activeEffects.Add(ge);}, and then the next time ExecuteEffect() is called, it'll pickup from where control was yield ed last time, right? Then I don't have to play around with iterators at all. (And you don't have to rage erase any code!) \$\endgroup\$ – Sable Dreamer Oct 19 '15 at 14:58
  • \$\begingroup\$ @SableDreamer what is the yield buying you? You're yielding, then turning around and getting the value that was yielded. What is the intent here? \$\endgroup\$ – moarboilerplate Oct 19 '15 at 20:07
  • \$\begingroup\$ For the record, I am accepting this answer for the solution of, "Don't use IEnumerators", as you've convinced me that the current code is worthy of rage-erasure. \$\endgroup\$ – Sable Dreamer Oct 26 '15 at 18:08
2
\$\begingroup\$

Your code is probably not working like you think it's working. I'd bet it works the same as code that just returns booleans directly.

Consider the fact that you're creating a new enumerator every time you call ExecuteEffect(). You take it and get the first item, and ignore the rest. Next time around, you get a completely new enumerator.

You're not using the enumerator in a for/foreach context and you're just using the first value and discarding the rest so your code should behave the exact same way as it would if you just returned a bool and inspected it.

As for comments about yielding control, you've only yielded control at your boolean which has already been calculated, so you haven't deferred your computation. Plus since you only use the first value and then your enumerator falls out of scope, you never resume anyway.

In fact, if you did manage to iterate over the results you'd see something interesting. Let's say executionStart is your magic value, -1 (please don't do that, by the way). You yield control at the corresponding false. Then let's say you MoveNext(). By the way, executionStart is still -1. Since you're picking back up at the next statement, does it no longer matter that executionStart is still -1? In fact, you might make your thing prematurely return true unless there is some hidden guarantee that the evaluation of GetRealTime() > executionStart + delay will always return the same as executionStart != -1m.

But don't even go there.

If what you're after is trying to defer a calculation, you should use a delegate or Action<bool> to define a method that returns your evaluation. But even if you did that, you're still synchronously iterating through a collection and evaluating the ExecuteEffect() method in every loop. So there's no gain there either.

So, return a boolean, because I think that's what your design is calling for.

\$\endgroup\$
  • \$\begingroup\$ I completely agree. I have no idea, why OP would want to use enumerator instead of returning bool value. \$\endgroup\$ – Nikita B Oct 20 '15 at 6:52
  • \$\begingroup\$ @NikitaBrizhak I think OP is trying to lazily evaluate the ExecuteEffect method, but since the code is running synchronously in a per-frame loop an iterator doesn't apply unless PLINQ is introduced and the per-frame checks the status of the running tasks. Also, I found this article which I think might have been an inspiration: twistedoakstudios.com/blog/… \$\endgroup\$ – moarboilerplate Oct 20 '15 at 16:17

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.