Should I declare all of the outer functions inside the object literal?

Question

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');
}

Answer 1

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}

Answer 2

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.

Answer 3

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.

Answer 4

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.

Answer 5

(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.