Prevent errors if `destroy()` is called twice

[IQAndreas]

Add checks before accessing object properties to see if they hadn't already been set to null.

Not modified the package org.flixel.system.debug.*

A few classes are destroyed automatically by Flixel (which guarantees they are only destroyed once), but some foolish user (or derailed bug) may decide to destroy something anyway, and since if checks are so cheap, it's better to be safe than sorry.

[IQAndreas]

This code has been compiled and tested to make sure it works properly, but I'm not quite done with it yet.

There were two types of code that were repeated frequently. I'll begin with the first:

var i:int = _timers.length-1;
while(i >= 0)
{
    var timer:FlxTimer = _timers[i--] as FlxTimer;
    if(timer != null)
        timer.destroy();
}
_timers.length = 0;

In this case while adds between one and three extra variables which would not appear if a for each loop was used. Considering how you will only destroy an item once and irregularly (if you destroy 200 objects every frame, you may want to rethink your setup), for the sake of clarity, shall we make those into for each loops?

I assumed a while loop was faster by a hair's width, but after just doing some research, it varies quite a bit. So, my argument is less strong now than it was when I first started this pull request.

[IQAndreas]

The second repeated code looks like this:

removeChild(_open);
_open = null;
removeChild(_recordOff);
_recordOff = null;
removeChild(_recordOn);
_recordOn = null;
removeChild(_stop);
_stop = null;
// etc, etc, etc...

And that's before I added the if (_open) check before each removeChild(). I would like to separate that code off into a separate function.

But I was wondering on some "funny" syntax that isn't exactly "conventional". Would this look too odd?

_open &&= destroyChild(_open);
_recordOff &&= destroyChild(_recordOff);
_recordOn &&= destroyChild(_recordOn);
_stop &&= destroyChild(_stop);
// etc, etc, etc...

// Inherited so all debug classes have access to it
protected function destroyChild(child:DisplayObject):Boolean
{
    if (child && (child.parent == this))
    {
        //child.destroy();
        this.removeChid(child);
    }

    return null;
}

I just hate seeing repeated code, but perhaps this is overkill? It definitely shortens down the destroy() functions, though.

[Dovyski]

I think it is overkill and hard to understand :) I would stick with the if blocks, perhaps simplifying it a little bit:

if(_open) removeChild(_open);
_open = null;
if(_recordOff) removeChild(_recordOff);
_recordOff = null;
if(_recordOn) removeChild(_recordOn);
_recordOn = null;
if(_stop) removeChild(_stop);
_stop = null;

I know you advocate the use of brackets (I do it too), but for that code I would omit them or use something like:

if(_open) { removeChild(_open); }

There is no need to use a new line for every { and } in this case, it will only bloat the code.

[IQAndreas]

There is no need to use a new line for every { and } in this case, it will only bloat the code.

Of course, why didn't I think of that? I have used the more compact if in the org.flixel.system.debug.* package. Should I go back end shorten down the other destroy() functions that I already committed, or should I just leave them as they are?

Have you tested it calling destroy() more than once to check if everything is working?

Not yet, but I'll make sure to do that before the final commit.

I did notice a major flaw in in the debug classes which causes a #1009 error if destroy() is called even once. However, since Flash automatically garbage collects ALL objects when you close the application, there is no system set up in Flixel to run destroy() on everything when the SWF is closing.

I could fix that in this pull request, but it may involve tampering with the way the toolbar handles Mouse Events.

Also, there is a lot of repeated code inside of Vis and VCR which could be removed by giving them a common base class FlxToolbar (which would potentially allow for more toolbars to be able to be added in the future with ease). There are also a lot of hard-coded values which should be more dynamic.

Since there is little risk of anyone manually destroying the FlxDebugger (and it is never destroyed automatically) should I leave the bug and the "cleanup" of the two classes for the future release, and then fix them with the same code?

[Dovyski]

Should I go back end shorten down the other destroy() functions that I already committed, or should I just leave them as they are?

I looked at the code and everything seems ok to me. I think you should let them as they are.

About the flaws and the "cleanup", let's work on them in our future release. A FlxToolbar will be a great addition and a cleanup is necessary to remove any duplicated code. We can cleanup and optimize other classes in the future too, such as FlxSprite.

[IQAndreas]

I added a tiny last commit which just prevents those potential #1009 errors that should never really happen from ever happening. I can sleep safely now.

There still is no system inside every function to check if the item has already been destroyed; calling members after destruction may cause errors, but that is to be expected. And there is only so far we can go as to account for user error.

I also tested the code. I didn't test all objects, only FlxSprite, FlxGroup, and FlxDebugger, and aside from Flixel yelling at me for destroying the debugger in the middle of a game, there were no problems. Even without testing them all, I'm quite confident that the rest of the destroy() functions are now safe as well (at least from null errors).

If no one objects, I believe this code is safe to merge.

[Dovyski]

I agree, the code is good to go. Feel free to merge it anytime you want.