6
\$\begingroup\$

I have been learning HTML/JavaScript and CSS. I have gone through some of the online tutorials, PDFs and books as well. Though, now I am familiar with syntactical stuff, I wasn't sure how I should design or structure code, so I started with a small ToDo application. I didn't want to use any frameworks. As I am still learning, I thought I should spend sometime learning the basics before using any frameworks.

GitHub

{
(function (){

    //Create a modal, which contains an array to hold list of todos
    var modal = {
        init: function () {
            this.todolist = [];
            this.todolist = JSON.parse(localStorage.getItem("tododata"));
            if (this.todolist === null)
                this.todolist = [];
        },
        getTodoList: function () {
            return this.todolist;
        },
        setTodoState: function (intodoid) {
            //Set status
            this.todolist[intodoid].status === "closed" ?
                this.todolist[intodoid].status = "open" : this.todolist[intodoid].status = "closed";
            localStorage.setItem("tododata", JSON.stringify(this.todolist));
        },
        setNewTodo: function (inval) {
            this.todolist.push({
                id: this.todolist.length,
                name: inval,
                status: "open"
            });
            localStorage.setItem("tododata", JSON.stringify(this.todolist));
            this.myFirebaseRef.push({id: this.todolist.length, name: inval, status: "open"});
        },
        clearall: function () {
            this.todolist = [];
        }
    }

    //Create a View, which interacts with Controller and alters the display in HTML
    var view = {
        init: function () {
            this.todolst = $('#todolistelm');
            this.todolst.html(' ');
            view.render();
            $('#clearall').click(function () {
                localStorage.clear();
                controller.clearall();
                view.render();
            });
        },
        render: function () {
            var htmlStr = '';
            var clr = true;
            var todoelem = $('#todolistelm');
            todoelem.html('');

            if (controller.getTodoList().length === 0) {
                $("#cleanmsg").css("visibility", "visible");
                $("#displaysection").css("visibility", "hidden")
            } else {
                $("#cleanmsg").css("visibility", "hidden");
                $("#displaysection").css("visibility", "visible")
            }

            controller.getTodoList().forEach(function (todo) {
                //DOM elements

                var elemId = ' ';
                var lielem = $('<li></li>');
                var spanelem = $('<span></span>');
                var taskid = "#task" + todo.id;

                //Create <li> element and add required classes
                lielem.addClass("list-group-item");
                //Set status of the todo item
                todo.status === "closed" ? lielem.addClass("completed") : lielem.addClass("");
                //Set alternate color
                clr === true ? lielem.addClass("colored") : lielem.addClass("");
                clr = !clr;

                //Create <span> and add required classes and IDs
                spanelem.addClass("glyphicon");
                spanelem.addClass("glyphicon-ok pull-right");
                spanelem.addClass("todocomplete");
                spanelem.attr('id', "task" + todo.id);

                //Append span and name to <li>
                lielem.append(spanelem);
                lielem.append(todo.name);

                //Append <li> to main list
                $(todoelem).append(lielem);

                //Add event handlers
                $(taskid).click(function (todoid) {
                    return function () {
                        controller.setTodoState(todoid);
                        view.render();
                    };
                }(todo.id));
            });

            //Set focus on input textbox by default
            $('#intodo').focus();
        }
    };

    //Create a controller, which acts as a mediator between View and Modal
    var controller = {
        init: function () {
            //Initialize view and modal
            modal.init();
            view.init();
        },
        getTodoList: function () {
            return modal.getTodoList();
        },
        setTodoState: function (intodoid) {
            modal.setTodoState(intodoid);
        },
        setNewTodo: function (inval) {
            modal.setNewTodo(inval);
        },
        clearall: function () {
            modal.clearall();
        }
    };

    //Set ENTER key on the input text box
    $('#intodo').keypress(function (e) {
        if (((e.which && e.which == 13) ||
                (e.keyCode && e.KeyCode == 13)) &&
            $("#intodo").val().trim().length > 0) {
            controller.setNewTodo($("#intodo").val());
            view.render();
            $("#intodo").val('');
        }
    });

    //Set the controller to start the work
    controller.init();
})();
}

Can someone go through this code and let me know if this follows the best practices or not, as well as any areas that I should probably be focusing on?

\$\endgroup\$
3
\$\begingroup\$

From a quick once over:

  • Your first line creates an anonymous function, it offers you the opportunity to pick a great function name that conveys to the reader what he is about to be exposed to like (function todoApp(){
  • This

    init: function () {
        this.todolist = [];
        this.todolist = JSON.parse(localStorage.getItem("tododata"));
        if (this.todolist === null)
            this.todolist = [];
    },
    

    could be

    init: function () {
        this.todolist = JSON.parse(localStorage.getItem("tododata")) || [];
    }
    

    essentially you were overwriting this.todolist = [] with
    this.todolist = JSON.parse(localStorage.getItem("tododata"));
    furthermore you should never drop the curly braces after an if block

  • You are copy pasting this line of code all over the place, you should have a dedicated function for this:
    localStorage.setItem("tododata", JSON.stringify(this.todolist));
  • This:

    setTodoState: function (intodoid) {
        //Set status
        this.todolist[intodoid].status === "closed" ?
            this.todolist[intodoid].status = "open" : this.todolist[intodoid].status = "closed";
        localStorage.setItem("tododata", JSON.stringify(this.todolist));
    },
    

    has a few problems, intodoid is an unfortunate name because it is not lowerCamelCase and because it smells like Hungarian notation. Also the repetition of this.todolist[intodoid].status is too much. I would suggest something like this:

    toggleTodoState: function (todoId) {
        //Toggle status
        var status = this.todolist[todoId].status;
        this.todolist[todoId].status = status == "closed" ? "open" : "closed";
        updateTodoData();
    },
    
\$\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.