0
\$\begingroup\$

I've written the code below to display a list of tasks from a Flask API. The form and associated .ajax method post to the API, immediately appending the data to the list of tasks. The code is working, but I'm fairly new to jquery and am curious if my code could be improved.

<html>
<head>
<title>Home</title>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.12.4/jquery.min.js"></script>
<script type="text/javascript">
$(document).ready(function () {
    $.getJSON("/api/v1/tasks", function(result){
        $.each(result['tasks'], function(key, item){
            $("div").append('<li>Task:' + item.title + ' Description:' + item.description + 'Status: ' + item.done + '</li>');
        });
    });
});
</script>
</head>
<body>
<div></div>
<p>Add Task</p>
<form id="task-form">
    <input id="title" type="text">
    <input id="description" type="text">
    <button type="submit">Submit</button>
</form>
<script>
$("#task-form").submit(function(e){

    e.preventDefault();

    var data = {
            'title': document.getElementById('title').value,
            'description': document.getElementById('description').value
        };

    var json = JSON.stringify(data);

    $.ajax({
        type : 'POST',
        url : '/api/v1/tasks',
        data: json,
        contentType: 'application/json;charset=UTF-8',
        success: function(result) {
            $("div").append('<li>Task:' + data["title"] + ' Description:' + data["description"] + 'Status: false</li>');
        }
    });

});
</script>
</body>
</html>
\$\endgroup\$
1
\$\begingroup\$

Here are some improvements that can be done

  1. Move the <script>s to the end of <body>. This way, the UI is show quickly to the end user and does not have to wait to load the scripts. Also, as the code is moved to the end of the <body>, all the elements are already loaded and there is no need to use ready().
  2. Create a string of HTML and append it to the element. By this, the DOM is accessed only once for any number of items in the tasks array.
  3. There is no need to convert the Object to String using JSON.stringify(). jQuery does this for you.
  4. Use val() to get the value of an element. It is considered to trim the string which are entered by user.
  5. Use unique ID on the <div> element and use it to select it using jQuery. $('div') will select all the <div> elements on the page and the results will be unexpected when there are more than one elements on page.

$.getJSON("/api/v1/tasks", function (result) {
    var html = '';
    result.tasks.forEach(function (item) {
        // Append to the string
        html += '<li>Task:' + item.title + ' Description:' + item.description + ' Status:' + item.done + '</li>';
    });

    // Update the DOM
    $('div').append(html);
});

$("#task-form").submit(function (e) {
    e.preventDefault();

    $.ajax({
        type: 'POST',
        url: '/api/v1/tasks',
        data: {
            title: $('#title').val(),
            description: $('#description').val()
        },
        contentType: 'application/json;charset=UTF-8',
        success: function () {
            $('div').append('<li>Task:' + data.title + ' Description:' + data.description + 'Status: false</li>');
        }
    });
});

Here's same code written using Revealing module pattern.

var taskModule = (function ($) {
    'use strict';

    // Private variables and functions
    var _apiURL = 'api/v1/tasks',
        _readTasks = function () {
            $.getJSON(_apiURL, _showTasks);
        },
        /**
         * To add new task in Database
         * @param  {Object} task Object containing task title and description
         */
        _addTask = function (task) {
            var self = this;
            $.ajax({
                url: _apiURL,
                data: task,
                success: function () {
                    _showTasks([task]);
                }
            });
        },
        _showTasks = function (tasks) {
            var taskHTML = '';

            tasks.forEach(function (task) {
                taskHTML += '<li>Task:' + item.title + ' Description:' + item.description + ' Status:' + (item.done || 'false') + '</li>';
            });

            $('#tasksContainer').append(taskHTML);
        },
        _bindEvent = function () {
            $('#task-form').on('submit', function (e) {
                e.preventDefault();

                _addTask({
                    title: $('#title').val(),
                    description: $('#description').val()
                });
            });
        };

    // Load tasks from server
    _readTasks();

    // Bind form submit event
    _bindEvent();

    // For public Interface
    // return {
    //     publicAlias: method/Variable name
    // };
}(jQuery));
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This is outstanding and helped me learn more about javascript. Thank you. \$\endgroup\$ – caseym Aug 5 '16 at 20:47

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.