3
\$\begingroup\$

I'm new to OOP, so I tried to learn from tutorials how to create a class in JavaScript/jQuery. But tutorials don't give you feedback from experienced coders. Is the approach to create a pop-up OK (see below)? I know it works (it's tested), but is it prone to errors or does it have redundancies?

var Popup = function (){
    /// PROPERTIES
    var new_id="div"+Date.now();
    var defaults = {
        id: new_id,
        container: "body",
        height: 200,
        width: 200,
        text: "<div> created with \"id\" = "+new_id,
        x: 100,
        y: 100,
        position: "absolute",
        bgcolor: "red"
    };
    this.settings=defaults;
    /// METHODS
    this.create=function (options){
        $.extend(this.settings,options);
        var html= "<div id=\""+
            this.settings.id+"\">"+
                this.settings.text+
                "</div>";
        $(this.settings.container).append(html);
        this.update();
    };
    this.update=function (options) {
        $.extend(this.settings,options);
        $("#"+this.settings.id).css({
            "height":this.settings.height+"px",
            "width":this.settings.width+"px",
            "position":this.settings.position,
            "top":this.settings.y,
            "left":this.settings.x,
            "background-color":this.settings.bgcolor
        });
    };
};

/* Popup use:

var pop=new Popup(); // new popup is declared
pop.create({bgcolor:"green"}); // popup appears green instead of read
pop.update({bgcolor:"blue"}); // popup changes to blue

*/

(I know there are a tons of Popup plugins that I could use, but that's irrelevant for my learning goals)

For example, I've realized that this line will cause problems:

this.settings=defaults;

because when "settings" are changed, "defaults" change too (!). Thus, the better alternative is:

this.settings=$.extend(defaults);

or a JavaScript "clone" function like this.

\$\endgroup\$
2
\$\begingroup\$

Most of your code looks good The one improvement is do not overwrite the initial settings, instead extend from defaults and the options provided. Use $.extend for this

 popup.settings = $.extend(defaults, options);

Also store the reference of this into a variable so that it will be used inside event handlers and anonymous callbacks.

var Popup = function() {
    var popup = this,
        new_id = "div" + Date.now(),
        defaults = {
            id: new_id,
            container: "body",
            height: 200,
            width: 200,
            text: "<div> created with \"id\" = " + new_id,
            x: 100,
            y: 100,
            position: "absolute",
            bgcolor: "red"
        };

    popup.create = function(options) {
        popup.settings = $.extend(defaults, options);
        var html = "<div id=\"" +
            popup.settings.id + "\">" +
            popup.settings.text +
            "</div>";
        $(popup.settings.container).append(html);
        popup.update();
    };

    popup.update = function(options) {
        popup.settings = $.extend(defaults, options);
        $("#" + popup.settings.id).css({
            "height": popup.settings.height + "px",
            "width": popup.settings.width + "px",
            "position": popup.settings.position,
            "top": popup.settings.y,
            "left": popup.settings.x,
            "background-color": popup.settings.bgcolor
        });
    };
};



var pop = new Popup(); // new popup is declared

pop.create({
    bgcolor: "green"
}); // popup appears green instead of read

pop.update({
    bgcolor: "blue"
}); // popup changes to blue
\$\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.