HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Do I need to remove these event listeners?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
theseneedremovelistenersevent

Problem

The following ActionScript code just downloads and attempts to parse some JSON from an API service. Inside the method I have defined a cleanUp() function, just to remove 2 event listeners.

I think these event listeners will be garbage collected anyway though, so is it worth it? I would prefer to just use 2 anonymous functions for the listeners.

private function sendRequest(cmd:String, params:Object = null) :Promise{

    var d:Deferred = new Deferred(),
        vars:URLVariables = new URLVariables('cmd=' + cmd),
        req:URLRequest = new URLRequest(),
        loader:URLLoader = new URLLoader();

    if(params){
        for(var paramKey:String in params){
            vars[paramKey] = params[paramKey];
        }
    }

    req.url = url;
    req.requestHeaders = [new URLRequestHeader('Accept', 'application/json')];
    req.method = URLRequestMethod.POST;
    req.data = vars;

    function onComplete(e:Event) :void{
        var res:Object;

        try{
            d.resolve(JSON.parse(e.target.data));
        }
        catch(err:Error){
            d.reject(err);
        }
        finally{
            cleanUp();
        }
    }

    function onError(err) :void{
        d.reject(err);
        cleanUp();
    }

    function cleanUp() :void{
        loader.removeEventListener(Event.COMPLETE, onComplete);
        loader.removeEventListener(IOErrorEvent.IO_ERROR, onComplete);
    };

    loader.addEventListener(Event.COMPLETE, onComplete);
    loader.addEventListener(IOErrorEvent.IO_ERROR, onError);

    loader.load(req);

    return d.promise;
};

Solution

Quick answer:

Maybe yes, maybe no. Yes, they'll be garbage collected now, but cleaning up after yourself is not a bad habit to get into. It doesn't hurt to do so.

What you do need to be careful of, however, is that you clean it up right.

Compare:

loader.addEventListener(Event.COMPLETE, onComplete);
loader.addEventListener(IOErrorEvent.IO_ERROR, onError);


With:

loader.removeEventListener(Event.COMPLETE, onComplete);
loader.removeEventListener(IOErrorEvent.IO_ERROR, onComplete);


and you'll soon notice the mistake: IOErrorEvent.IO_ERROR is mapped to onError, but the removeEventListener assumes it's mapped to onComplete!

Additionally, you have a unnecessary variable here:

var res:Object;


It's not used by anything, so I'd remove it.

Code Snippets

loader.addEventListener(Event.COMPLETE, onComplete);
loader.addEventListener(IOErrorEvent.IO_ERROR, onError);
loader.removeEventListener(Event.COMPLETE, onComplete);
loader.removeEventListener(IOErrorEvent.IO_ERROR, onComplete);
var res:Object;

Context

StackExchange Code Review Q#52651, answer score: 2

Revisions (0)

No revisions yet.