Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Note:

  1. I am very new to Javascript.
  2. The code I provided is long but not complicated.

The code below works and runs fine. But I would like to seperate it out into logical 'pure' functions to make it clearer whats going on. I started to re-factor this myself and made some good progress, however, I quickly started to return to problems with dependencies and embedded functions.

I'd be interested to see how others would write the following:

   var contactsController = {

fetch: function()
{
    // Parses the returned JSON into the database
    var fetchSuccess = function(json)
    {          
        var NAME = "Contact",
            COLUMNS = ['Name', 'GivenNames', 'FamilyName', 'Telephone'];

        if (json.Response.Code == 0)
        {
            database.open();
            database.dropTable(NAME);
            database.createTable(NAME, COLUMNS);    

            // .Contact is an array of contact objects
            var contacts = json.Contacts.Contact; 

            for (var contact in contacts)
            {
                var contact = contacts[contact];
                database.insert(NAME, COLUMNS, [ontact.Name, contact.GivenNames, contact.FamilyName, contact.Telephone] );
            }

            contactsController.populateList();
        }
    }

    ServerConn.fetchContacts(fetchSuccess);
},

populateList: function()
{
    var $page = $("#contactsPage");

    $page.find(".content").empty();
    $page.find(".content").html("<ul data-role='listview' data-filter='true'><\/ul>");
    $list = $page.find(".content ul");

    var render = function(character)
    {
        return function(tx, result)
        {
            var max = result.rows.length;

            if (max > 0)
            {
                var header = "<li data-role='list-divider'>" + character + "</li>",
                    listItem = $(header);

                $list.append(header);

                for (var i = 0; i < result.rows.length; i++) 
                {
                    var contact = result.rows.item(i),
                        strHtml = "<li><a href='#contactPage'>" + contact.Name + "<\/a><\/li>";

                    listItem = $(strHtml);
                    $list.append(listItem);

                    // store the data in the DOM
                    $list.find("a:last").data("contactObj", contact);
                }

                $list.find("a").click(function()
                {
                    var $this = $(this);
                    $("#contactPage").data("contactObj", $this.data("contactObj"));
                });

                $list.listview(); //Only fires once?
                $list.listview('refresh');  
            }
        }
    }

    var str = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
    for(var i = 0; i < str.length; i++)
    {
        var sql = "SELECT * FROM Contact WHERE Exclude = ? AND Name LIKE ? ORDER BY Name",
            nextChar = str.charAt(i),
            selargs =  ['0', nextChar + "%"];

        database.open();
        database.query(sql, selargs, render(nextChar));
    }
}

};

share|improve this question

1 Answer

up vote 2 down vote accepted

A lot of the logic in your code belongs to a template, not directly in code. You can find tons of templating engines for javascript or make your own.

An example of a template that could be used here is:

<script type="text/html" id="letter-index-view">
@if( model.result.rows.length ) {
    <li data-role="list-divider">@model.result.character</li>
    @foreach( contact in model.result.rows ) {
        <li><a href="#contactPage" id="@contact.ContactID">@contact.Name</a></li>
    }
}
</script>

Now populateList becomes this:

populateList: function()
    {
        var $page = $("#contactsPage"),
            tmpl = Template.getById("letter-index-view"),
            str = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

        $list = $("<ul>", {
            "data-role": "listview",
            "data-filter": "true"
        });

        $list.on( "click", "a", function() {
            $("#contactPage").data("contactId", this.id);
        });

        $page.find(".content").empty().append( $list );

        for(var i = 0; i < str.length; i++)
        {
            var nextChar = str.charAt(i),
                sql = "SELECT *, ? AS character FROM Contact WHERE Exclude = ? AND Name LIKE ? ORDER BY Name",
                selargs =  [nextChar, '0', nextChar + "%"];

            database.open();
            database.query(sql, selargs, function(tx, result){
                $list.append( tmpl( {
                    result: result
                }));

                $list.listview(); //Only fires once?
                $list.listview('refresh');  
            });
        }
    }

Note that I made some other changes as well, since template is kind of static html it cannot have object references like $list.find("a:last").data("contactObj", contact); so the links are uniquely identified with their database row id.

I also modified the query to return the character used in that query so a static function can be used for the query callback instead of 26 different ones.

Another reason for populateList being lengthy is that it's doing view and model stuff at the same time. You should move all the DOM stuff into a view.

share|improve this answer
There are some really nice bits of code here. I particularly like storing the character in the SQL rather than using the immediately returning function. Is the template from scratch or have you used a framework? – CrimsonChin Apr 11 '12 at 11:15
@CrimsonChin Not sure what you mean... I just wrote the template according to your code. A template engine would convert that to real javascript function (Template.getById("letter-index-view")) which can be called to generate HTML according to the templating language used. – Esailija Apr 11 '12 at 11:39

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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