1
\$\begingroup\$

I have the following javascript es6 class (along with some functions) that generates an area where you can draw in a page.

class Painter {
    constructor(selector, cols, rows, style) {
        this.looper = null;
        if (!selector)
            throw new TypeError("Provide a valid table selector");
        this.cols = cols || 20;
        this.rows = rows || 20;
        this.selector = selector;
        this.style = style;
        let markup = "";
        for (let height = 0; height < this.rows; height++) {
            markup += "<tr>";
            for (let width = 0; width < this.cols; width++)
                markup += "<td data-col=" + width + " data-row=" + height + "></td>";
            markup += "</tr>";
        }
        $(function() {
            $(selector).html(markup);
            $(selector).on("click", "td", function() {
                $(this).toggleClass("tile-active");
            });
        });
    }
    setStyleSheet(stylesheet) {
        this.style = stylesheet;
    }
    changeTileColor(stylesheet, newColor) {
        if (!newColor)
            return;
        var css = ".tile-active{\n\tbackground:" + newColor + ";\n}";
        try {
            changeCss((stylesheet || this.style), css);
        } catch (e) {
            console.log("Suppressed error");
        }
    }

    createChessBoard() {
        var id = this.selector;
        $(function() {
            $(id + " tr:nth-child(even) td:nth-child(even)").addClass("black-tile");
            $(id + " tr:nth-child(odd) td:nth-child(odd)").addClass("grey-tile");
        });
    }
    toggleBorder() {
        var bname = $(this.selector).css("border");
        $(this.selector).css("border", bname.indexOf("none") != -1 ? "2px solid" : "none");
    }
    clearBoard() {
        $(this.selector + " .tile-active").removeClass("tile-active");
    }
    createRandowDraw() {
        this.clearBoard();
        let mat = $(this.selector + " td");
        let max = mat.length;
        for (let i = 0; i < max; i++) {
            let num = Math.floor(Math.random(2) * 2);
            if (num && num <= 1)
                $(mat[i]).addClass("tile-active");
        }
    }
}

As I am new to classes in ECMAScript 6 I was wondering if there is anything I can do to improve the performance/readability of this code.

Here is a link to the following code: Painter

\$\endgroup\$
  • \$\begingroup\$ Your question says that this code "generates an area where you can draw in a page", but the code seems to have a lot to do with tiles and chessboard patterns. Could you clarify? Ideally, make a live demo (press Ctrl-M in the question editor) so that we can see exactly what is going on. \$\endgroup\$ – 200_success Nov 4 '16 at 19:20
1
\$\begingroup\$

The first weird thing I saw in your code is the use of the $(fun).

This jQuery function is used to execute code when the DOM is ready.

I think it should be better to remove all this calls and instead instantiate the class itself in a function executed after the DOM is ready.

That helps to follow the code properly.

// here you declare the class
class MyClassThatUseDOM {
    constructor(selector) {
        this.myElement = $(selector);
    }
}

$(function(){

    // here you instantiate the class 
    // when the DOM is ready and all selector
    // will properly match.
    const myObj = new MyClassThatUseDOM("#theselector");

});

As the element $(selector) or $(this.selector) are used may times it is best to save in an instance variable:

this.element = $(selector);

And then access always from this variable, instead of continue to search it with jQuery.

The following method have some issue for me:

createChessBoard() {
    var id = this.selector;
    $(function() {
        $(id + " tr:nth-child(even) td:nth-child(even)").addClass("black-tile");
        $(id + " tr:nth-child(odd) td:nth-child(odd)").addClass("grey-tile");
    });
}

Maybe if you remove the $(func) part as I suggested in my first point the code will be more clear.

But let's discuss what we have now.

You use a variable id to save the selector, but id is more specific term than selector. It could confuse some developer.

A selector for jQuery is anything is a valid css selector, like a tag name, a css class name or even and id.

Please pay attention to the names you use for your variables.

In this case you can do something like:

let _selector = this.selector

As you can see you understand that you just assign a private variable, here.

Use of var is not wrong, but we have let.

Similar considerations for:

var bname = $(this.selector).css("border");

About the method:

createRandowDraw() {
    this.clearBoard();
    let mat = $(this.selector + " td");
    let max = mat.length;
    for (let i = 0; i < max; i++) {
        let num = Math.floor(Math.random(2) * 2);
        if (num && num <= 1)
            $(mat[i]).addClass("tile-active");
    }
}

Here you could use jQuery API to simplify your code:

createRandowDraw() {
    this.clearBoard();
    $(this.selector + " td").each(function(i, mat){
        let num = Math.floor(Math.random(2) * 2);
        if (num && num <= 1) {
           $(mat).addClass("tile-active");
        }
    });
}

Or in a plain javascript fashion by using filter and forEach.

\$\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.