1
\$\begingroup\$

I am working on an HTML5/jQuery todo list app and here is my code so far.

As you might notice, there are some functions called at the beginning of the JS code. These functions work perfectly right now. Is it OK if I declare these functions inside the object literal and then call it using the object? In my opinion, the code would look consistent to the other programmers.

What is the ideal way of doing this?

jsFiddle

$(function(){

    // check if browser supports localStorage, if not, notify and exit
    checkLocalStorageBrowserSupport();

    //check localStorage for existing data
    checkLocalStorageExistingData();

    tweakMinorUIStuff();

    // object literal todo_item
    var todo_item = {
        content: "",        // default content
        starred: false,      // default star value
        addItem: function(){
            // collect new item data
            todo_item.content = $("#new_todo_item").val();
            todo_item.starred = $("#star").prop('checked');
            addItem(todo_item);
        },
        toggleStar: function(){
            // update localStorage
            // update UI
            alert("working!");
        },
        editItem: function(){
            // toggle textarea
            // update localStorage
            // update UI
        }
    }

    $("#add").click(function(){ todo_item.addItem(); });
    $(".star").click(function(){ todo_item.toggleStar(); });



})

function checkLocalStorageBrowserSupport(){
    if(!window.localStorage) {
        // notify user
        $("#info").text("Your browser does not support the HTML5 feature 'localStorage'. Please use an updated browser for this application to work.");
        // stop
        return false;
    } else
        return true;
}

function checkLocalStorageExistingData(){
    if(localStorage.getItem('todo_items')){
        var todo_items_json = localStorage.getItem('todo_items');

        var todo_items = todo_items_json ? JSON.parse(todo_items_json) : [];

        todo_items.forEach(function(item){
            var item_content = item[0];
            var star_check = item[1];

            var checked = "";
            if(star_check) checked = "checked";

            $("ul").append("<li><button class='done'>Done</button>"+
                           "<p class='item_content'>"+item_content+"</p>"+
                           "<input type='checkbox' class='star' "+checked+" />"+ 
                           "<label>Star</label>");

        });
    }    
}

function addItem(todo_item){

    // fetch current items from localStorage
    var todo_items_json = localStorage.getItem('todo_items');
    var todo_items = todo_items_json ? JSON.parse(todo_items_json) : [];

    // check for duplicate item
    var duplicate = false;
    $.each(todo_items,function(i,item){
        //console.log(item[0]);
        if(todo_item.content === item[0]){
            duplicate = true;
            //console.log("found!");
            // cut the loop
            return false;
        }
    });

    // if new item is a duplicate of an existing item, do not add it
    if(duplicate){
        // highlight the original item
        $( ".item_content:contains('"+todo_item.content+"')" ).parent().effect("highlight", {}, 10000);

        // notify the user
        $("#info").text("You already have '"+todo_item.content+"' in your list!");

        // set focus on textbox again
        $("#new_todo_item").focus();

        return false;
    }

    // compose and add new todo item to localStorage
    var new_todo_item = [todo_item.content,todo_item.starred];
    todo_items.push(new_todo_item);
    localStorage.setItem('todo_items', JSON.stringify(todo_items));

    // update UI with new todo item
    var checked = "";
    if(todo_item.starred) checked = "checked";
    $("ul").append("<li><button class='done'>Done</button>"+
                   $("#new_todo_item").val()+
                   "<input type='checkbox' class='star' "+checked+" />"+
                   "<label for='star'>Star</label>");

    // reset the content of textarea
    $("#new_todo_item").val("");

    // uncheck the add star checkbox
    $("#star").prop('checked', false);

    // set focus on textbox again
    $("#new_todo_item").focus();
}

function tweakMinorUIStuff(){
    // make teaxtarea height auto-increate
    $('#new_todo_item').elastic();

    // set focus on textbox
    $("#new_todo_item").focus();

    $("#new_todo_item").on("input propertychange paste", function(){
        if($("#new_todo_item").val().length != 0)
            $("#add").removeAttr('disabled');
        else
            $("#add").attr('disabled','disabled');
    });

    $("#undo, #redo").attr('disabled','disabled');
}
\$\endgroup\$

5 Answers 5

3
\$\begingroup\$

Two things:

  • Don't use functions before you define them. Yes, this works fine with function declarations, but still it's a bad practise that disrupts the reading flow.
  • Don't use global variables. All these functions are declared globally even though you don't seem to need this, and pollute the global scope. Instead, you should move them into the the module function.

$(function(){
    // first declare everything (locally):
    function checkLocalStorageBrowserSupport(){
        …
    }
    function checkLocalStorageExistingData(){
        …
    }
    function addItem(todo_item){
        …
    }
    function tweakMinorUIStuff(){
        …
    }
    var todo_item = {
        …
    }

    // then call it:
    checkLocalStorageBrowserSupport();
    checkLocalStorageExistingData();
    tweakMinorUIStuff();
    $("#add").click(function(){ todo_item.addItem(); });
    $(".star").click(function(){ todo_item.toggleStar(); });
})

Disclaimer: I've only commented on the code layout, I haven't actually read what the functions are doing

\$\endgroup\$
7
  • \$\begingroup\$ Why should I not use functions before defining them? EDIT: By module function, do you mean the object literal? \$\endgroup\$ Commented Jul 8, 2014 at 16:45
  • \$\begingroup\$ It's more natural to define them first and after that use them. A reader will read your file from top to bottom. \$\endgroup\$
    – Bergi
    Commented Jul 8, 2014 at 16:48
  • \$\begingroup\$ By module, I mean the function's scope. It's very similar to an IEFE which is used in the module pattern, only that it is executed by jQuery (and you're not exporting anything). \$\endgroup\$
    – Bergi
    Commented Jul 8, 2014 at 16:49
  • \$\begingroup\$ IMO, the reader would like to see the main function first [in this code it is $(function(){ ... })] and then read the other functions as they are popping up in the main. Edit: That's why I have coded it like that. \$\endgroup\$ Commented Jul 8, 2014 at 16:52
  • \$\begingroup\$ Hm, probably it's just a coding convention. Many other programming languages enforce this, and I'm accustomed to it :-) \$\endgroup\$
    – Bergi
    Commented Jul 8, 2014 at 16:58
2
\$\begingroup\$

First a once over:

  • // check if browser supports localStorage, if not, notify and exit checkLocalStorageExistingData();

    This comment lies, the code does no such thing, you probably should check the boolean returned by checkLocalStorageExistingData

  • Always consider chaining when writing jQuery code:

    // make teaxtarea height auto-increate
    $('#new_todo_item').elastic();
    
    // set focus on textbox
    $("#new_todo_item").focus();
    

    could be

    $('#new_todo_item').elastic().focus();
    
  • Instead of working with attr and disabled you could work with prop() so that this

    if($("#new_todo_item").val().length != 0)
        $("#add").removeAttr('disabled');
    else
        $("#add").attr('disabled','disabled');
    

    could be simply

    $("#add").prop('disabled' , ! $("#new_todo_item").val().length );
    
  • It does not make sense to wrap your functions for your event handling, this:

    $("#add").click(function(){ todo_item.addItem(); });
    $(".star").click(function(){ todo_item.toggleStar(); });
    

    should be simply

    $("#add").click( todo_item.addItem );
    $(".star").click( todo_item.toggleStar );
    
  • Also, lowerCamelCase is the JavaScript way so todo_item -> todoItem, personally since this is a todo application, I would simply call it item

  • 0 and 1 should be nicely named upper case constants:

        var item_content = item[CONTENT];
        var star_check = item[STAR];
    

    Other than that, consider using objects with named properties instead of arrays, you have no great efficiency or memory concerns going on in your app.

  • JsHint only complains about some missing semicolons, very nice

  • I like the way you set it up, I know what your app does because of well named functions, and then I can go in detail if I want to.

  • Your comments are a bit excessive

    // set focus on textbox again <- Captain Obvious!..
    $("#new_todo_item").focus();
    
  • The large piece of copy pasting is between lines 52-56 and 75-79, you could consider this for refactoring, but it aint too bad.

\$\endgroup\$
5
  • \$\begingroup\$ Four the 4th bullet that you have mentioned, if I do that, the function is executed onLoad. For 5th bullet, I did not use lowerCamelCase since useing _ makes it more readable IMO. For 6th bullet, if I make that change, the code doesnt work. Thank you for other bullets. :) \$\endgroup\$ Commented Jul 9, 2014 at 4:06
  • \$\begingroup\$ Interesting, 4) It worked fine for me, 5) Heresy! ;) 6) You would first have to declare var CONTENT = 0 , STAR = 1 first \$\endgroup\$
    – konijn
    Commented Jul 9, 2014 at 13:11
  • \$\begingroup\$ (3) is not working any more. Note sure why. jsfiddle.net/rdesai/csTS7/88 \$\endgroup\$ Commented Jul 12, 2014 at 2:09
  • \$\begingroup\$ To reproduce this problem, add the first item, then on second an empty item is allowed to be added, even at third too. \$\endgroup\$ Commented Jul 12, 2014 at 2:17
  • \$\begingroup\$ I don't think the 4th bullet is good. Since this todo_item is a global object with function properties it works but if todo_item was an instance of an object and the functions were on the prototype it would not. \$\endgroup\$ Commented Jul 28, 2014 at 18:33
1
\$\begingroup\$

Not only would that be okay, but I'd actually recommend it. Defining the checkLocalStorageBrowserSupport, checkLocalStorageExistingData, addItem and tweakMinorUIStuff methods in the global scope is a bad idea in js. It's too easy for another js library to overwrite your methods in global scope, and that will break your application. Putting the methods on todo_item at least pulls those methods out of global scope.

I'd recommend that you investigate one of the popular js encapsulation patterns like the Revealing Module Pattern. You'll find a bazillion examples online if you do a search.

\$\endgroup\$
1
\$\begingroup\$

What is the ideal way of doing this?

  • Create class ListStore -- Data, stores as blob

        var store=new ListStore(name);
        store.items();// returns an array
        store.items([blah]);//saves items
        store.clear();// same as store.items([]);
    
  • Create class ToDoListing -- UI, responsible for display of items

        var toDoListing= new ToDoListing(selector);
        toDoListing.addItem({blah});
        toDoListing.deleteItem(index);
        toDoListing.setData([blah]);
        toDoListing.getData();
        toDoListing.clear();
    
  • Create class ToDoList -- UI, manages a listStore and multiple toDoListings

        var toDoList= new ToDoList(name,selector);
        toDoList.createListing(selector);
    

I will stop there, but basically once you lay down your infrastructure like this, your application becomes very easy to enhance and progress down a development path. You have got chocolate in your vanilla and though it might taste good at this point in the game it will confuse you as you move forward. The fastest first step is to get the data out of your functions. If you do not want to work from the top down like I started here, work from the bottom up, start with the ListItem (UI) -- ask yourself what it is and what is the minimum IT needs to do and know about. Then go up to its parent.

\$\endgroup\$
1
\$\begingroup\$

(1) I find it very confusing that you re-use the same term in different ways. For instance you have a todo_item object which just has some properties and functions. I can't think of why these should be together and they certainly don't represent the values for an individual todo item. Then you have both an addItem function on that object and another addItem function that has a parameter named todo_item, which would hide your previous object variable. Specifically though I think it is poor form to call a different addItem function from within a function with the same name.

(2) You call checkLocalStorageBrowserSupport(), but don't store the return value and don't seem to check it in your functions that make use of local storage. It seems like you require localStorage, so I think it would be good to hide `todo_list_container' if there's no local storage so you never have buttons that don't do anything but that are just begging to be clicked.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.