CODEWITHSUNDEEP
javascriptclassdomobjectreference
3jfx
3jfx

Reputation: 75

Why is the wrong object removed?

I've written this simple class to create a div element with a message which should disappear after the given time. This works great, but when creating multiple messages with this the hide and destroy functions will only apply on the last created message.

This is my class:

function message(text, duration){
    var self = this;

    function init(){
        this.obj = document.createElement("div");
        this.obj.setAttribute("class", "message");
        this.obj.appendChild(document.createTextNode(text));
        document.body.appendChild(this.obj);
        setTimeout(function(){self.display.call(this);}, 20);
        setTimeout(function(){self.hide.call(this);}, duration);
        setTimeout(function(){self.destroy.call(this);}, duration+1000);
    }

    this.display = function(){
        this.obj.setAttribute("class", "message display");
    }

    this.hide = function(){
        this.obj.setAttribute("class", "message gone");
    }

    this.destroy = function(){
        document.body.removeChild(this.obj);
    }

    init();
}

This works:

new message("This will be removed in 5 seconds.", 5000);

This doesn't work as it should be:

new message("This will not be shown", 2000);
new message("This will be removed after 2 seconds", 5000);

There might be some reference error but i can't spot any.

Upvotes: 3

Views: 74

Answers (3)

user1693593
user1693593

Reputation:

I believe these this'es becomes window:

setTimeout(function(){self.display.call(this);}, 20);
setTimeout(function(){self.hide.call(this);}, duration);
setTimeout(function(){self.destroy.call(this);}, duration+1000);

So try with:

setTimeout(function(){self.display()}, 20);
setTimeout(function(){self.hide()}, duration);
setTimeout(function(){self.destroy()}, duration+1000);

Also, why not use self for the rest of the code:

function init(){
    self.obj = document.createElement("div");
    self.obj.setAttribute("class", "message");
    self.obj.appendChild(document.createTextNode(text));
    document.body.appendChild(self.obj);
    setTimeout(function(){self.display()}, 20);
    setTimeout(function(){self.hide()}, duration);
    setTimeout(function(){self.destroy()}, duration+1000);
}

this.display = function(){
    self.obj.setAttribute("class", "message display");
}

this.hide = function(){
    self.obj.setAttribute("class", "message gone");
}

this.destroy = function(){
    document.body.removeChild(self.obj);
}

(I think I got them all.. let me know if I too missed a spot)

Upvotes: 1

PatrikAkerstrand
PatrikAkerstrand

Reputation: 45721

You are pretty close to a working solution. I've fixed the code here: http://jsfiddle.net/GTpKV/

You (correctly) make self reference to this, but inside your code you still use this (this is lost when using inner functions, and you also call init directly, which means this will refer to window in that case. Had you used this.init() it would have created the references correctly on the object, but then the timeouts would have messed things up). The safest way (which I've taken), is to simply replace all occurrences of this with self. Another idea would be to analyze when the context is correct, and when it is not. But since you already have a correctly bound context in self, you might as well use it. Here is the corrected code found in the JSFiddle link as well:

function message(text, duration){
    var self = this;

    function init(){
        self.obj = document.createElement("div");
        self.obj.setAttribute("class", "message");
        self.obj.appendChild(document.createTextNode(text));
        document.body.appendChild(self.obj);
        setTimeout(function(){self.display();}, 20);
        setTimeout(function(){self.hide();}, duration);
        setTimeout(function(){self.destroy();}, duration+1000);
    }

    this.display = function(){
        self.obj.setAttribute("class", "message display");
    }

    this.hide = function(){
        self.obj.setAttribute("class", "message gone");
    }

    this.destroy = function(){
        document.body.removeChild(self.obj);
    }

    init();
}

new message("This will not be shown", 2000);
new message("This will be removed in 5 seconds.", 5000);

Upvotes: 2

Xotic750
Xotic750

Reputation: 23482

Perhaps you can try this as a solution. Get rid if the init function and change the this inside you setTimeouts to self, and do away with the unnecessary call.

Javascript

function message(text, duration) {
    var self = this;

    this.display = function () {
        this.obj.setAttribute("class", "message display");
    }

    this.hide = function () {
        this.obj.setAttribute("class", "message gone");
    }

    this.destroy = function () {
        document.body.removeChild(this.obj);
    }

    this.obj = document.createElement("div");
    this.obj.setAttribute("class", "message");
    this.obj.appendChild(document.createTextNode(text));
    document.body.appendChild(this.obj);
    setTimeout(function () {
        self.display(self);
    }, 20);
    setTimeout(function () {
        self.hide(self);
    }, duration);
    setTimeout(function () {
        self.destroy(self);
    }, duration + 1000);
}

new message("This will not be shown", 2000);
new message("This will be removed after 2 seconds", 5000);

On jsfiddle

Upvotes: 0

Related Questions