22
\$\begingroup\$

I'm making a simple to-do list with jQuery. This is my first step in JavaScript and jQuery. How can I improve my code? User enter tasks in input field, submit by pressing 'Enter'. On double-click, he can edit his record. He can mark check-box, and record will make completed. By clicking bottom check-box all records will be marked as completed. Button 'Clear competed' delete all records marked as completed.

function addListItem()
    {

    var text = $('#new-text').val();
        $("#todo-list").append('<li ><input type="checkbox" class="toggle"/ ><span class="text">'
    + text + ' </span><button class="destroy"></button></li>');
    $("#new-text").val('');

    }

function clearCompleted()
    {

    $("#todo-list .toggle:checked").parent().remove();

    }

function deleteItem()
    {

    $(this).parent().remove();

    }

function completed() {
    if
    (
         $(this).parent().css('textDecoration') == 'line-through' )
    {
         $(this).parent().css('textDecoration', 'none');
         $(this).parent().css('opacity', '1');
    }

    else

    {
         $(this).parent().css('textDecoration', 'line-through');
         $(this).parent().css('opacity', '0.50');
    }

    }


$(document).ready(function(){

    $('#new-text').keyup(function(e)
    {

        if (e.keyCode === 13)

        {
            addListItem();
        }

    });

    $(document).on('click', '.destroy', deleteItem);

    $("#toggle-all").click(function ()

    {

        $('input:checkbox').not(this).prop('checked', this.checked);
        if ( $('li').css('textDecoration') == 'line-through' )
        {
            $('li').css('textDecoration', 'none');
            $('li').parent().css('opacity', '1');
        }

        else

        {
            $('li').css('textDecoration', 'line-through');
            $('li').parent().css('opacity', '0.5');
        }

    });

    $(document).on('click', '.toggle', completed);

    $("#clearcompleted").click(clearCompleted);

    $('#todo-list').on('dblclick', 'span', function ()

        {

            var thisData = this.innerHTML,
            $el = $('<input type="text" class="in-edit-text"/>');
            $(this).replaceWith($el);
            $el.val(thisData).focus();
            $(this).find(".text").hide();
            $(this).find(".destroy").hide();

        }
    );

    $('#todo-list').on('keyup', '.in-edit-text', (function(e)

    {
        if (e.keyCode === 13)

        {
            $(this).replaceWith($('<span class="text">' + $(this).val() + '</span>'));

        }

        if (e.keyCode == 27) {
            $('.in-edit-text').remove();
        }

    }));

});
html,
body {
    margin: 0;
    padding: 0;
}

body {
    font: 14px 'Helvetica Neue', Helvetica, Arial, sans-serif;
    line-height: 1.4em;
    background: #f5f5f5;
    color: #4d4d4d;
    margin: 0 auto;
}

button {
    outline: none;
}

input[type="checkbox"] {
    width: auto;
}

#new-text {
    background: white;
    font-size: 24px;
    margin: 130px 0 40px 0;
    position: relative;
}

#toggle-all {
    position: relative;
    top: 0px;
    left: 0px;
    width: 40px;
    height: 20px;
}

#todo-list {
    margin: 0;
    padding: 0;
    list-style: none;
}

#todo-list li {
    position: relative;
    font-size: 24px;
    border-bottom: 1px solid lightgrey;
}

#todo-list li .toggle {
    text-align: center;
    width: 40px;
    height: auto;
    position: absolute;
    top: 0;
    bottom: 0;
    margin: auto 0;
    appearance: none;
}

#todo-list li span {
    white-space: pre;
    padding: 15px 60px 15px 15px;
    color: black;
    margin-left: 45px;
    display: block;
    line-height: 1.2;
}

#todo-list li .destroy {
    display: none;
    position: absolute;
    top: 0;
    right: 10px;
    bottom: 0;
    width: 40px;
    height: 40px;
    margin: auto 0;
    font-size: 30px;
    color: #cc9a9a;
    margin-bottom: 11px;
}

#todo-list li .destroy:after {
    content: '×';
}

#todo-list li:hover .destroy {
    display: block;
}

.in-edit-text{

    padding: 15px 60px 15px 15px;
    margin-left: 45px;
    font-size: 24px;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<h1><input type="text" id="new-text" placeholder="Input text here..."/></h1>
<ul id="todo-list">

</ul>
<br>
<input type="checkbox" id="toggle-all"/>
<br>
<br>
<button type="checkbox" id="clearcompleted">Clear completed</button>

\$\endgroup\$
3
  • 4
    \$\begingroup\$ Please don't encourage users to remove their fiddles. I know some of you guys like StackSnippets, but they are very inconvenient for testing recommendations I'd like to make. \$\endgroup\$
    – cimmanon
    Commented Jun 15, 2015 at 20:33
  • \$\begingroup\$ You could use a client-side MVC (Model-View-Controller) framework. There is a site for comparison of various frameworks capabilities: TodoMVC I practice AngularJS (by Google) for months, and even if it's not always easy, it is very powerful. With Angular, you could do your "todo" list with data binding within a controller. Here is a Fiddle for a simple "ToDo list" with AngularJS. \$\endgroup\$
    – Elo
    Commented Jun 16, 2015 at 15:42
  • \$\begingroup\$ Since no one mentioned this yet, adding a couple of comments to help yourself skim this code 2 years from now, is always nice. \$\endgroup\$
    – HC_
    Commented Jun 17, 2015 at 21:40

6 Answers 6

14
\$\begingroup\$
  • Use CSS classes rather than inline styles. Instead of modifying the style of each TODO item, use CSS classes. This offloads the styling into a style sheet where it belongs, and it still allows you to test an item to see if it is already completed:

    function completed() {
        if ( !$(this).parent().hasClass('completed') ) {
            $(this).parent().addClass('completed');
        } else {
            $(this).parent().removeClass('completed');
        }
    }
    

    This makes your code appear more fluent and language-like.

  • Remove multiple $(this) calls. There is no need to call $(this) multiple times. The $ function returns a new jQuery object on every call. Save the return value of this call in a variable. Furthermore, the complete function constantly refers to $(this).parent(), so stash that as a variable:

    function completed() {
        var $item = $(this).parent();
    
        if ( !$item.hasClass('completed') ) {
            $item.addClass('completed');
        } else {
            $item.removeClass('completed');
        }
    }
    
\$\endgroup\$
3
  • 3
    \$\begingroup\$ Why not use toggleClass? Incidentally, you have a bug in the first code block: you add a class to the element's parent, but you remove it from element itself. \$\endgroup\$
    – Flambino
    Commented Jun 15, 2015 at 20:08
  • \$\begingroup\$ @Flambino: Fixed the code error. Funny thing. I had fixed it in my editor, but forgot to copy and paste into my answer \$\endgroup\$ Commented Jun 15, 2015 at 20:10
  • \$\begingroup\$ Cool - and yeah, I've made that mistake way too often too :P \$\endgroup\$
    – Flambino
    Commented Jun 15, 2015 at 20:11
10
\$\begingroup\$

Redundant properties

You have a few redundant properties. The margin on your 2nd body declaration is doing nothing because you haven't set a width.

html,
body {
    margin: 0; /* original */
    padding: 0;
}

body {
    font: 14px 'Helvetica Neue', Helvetica, Arial, sans-serif;
    line-height: 1.4em;
    background: #f5f5f5;
    color: #4d4d4d;
    margin: 0 auto; /* redundant */
}

Also, this isn't necessary since inline elements don't honor the width property:

input[type="checkbox"] {
    width: auto;
}

Redundant selectors

You have a lot of redundant elements referenced in your selectors. If you element is a list, there's no need to reference the li element if you're targeting one of its descendants because the ul must contain li elements. In other words, li can be removed from all of these selectors:

#todo-list li .toggle {

}

#todo-list li span {
}

#todo-list li .destroy {

}

#todo-list li .destroy:after {

}

The only selector that doesn't contain a redundant li is #todo-list li:hover .destroy

Markup / Usability

Your checkboxes need labels to go with them. The click area is sooo tiny, users with mobility problems will have a difficult time clicking on them without the aid of a label. The fact that you're using a span here instead is a crying shame.

There's no indication as to what the last checkbox is for until you click on it. User's shouldn't have to guess as to what any part of your website does. Also, it should either be disabled or hidden until the list has been populated.

\$\endgroup\$
7
\$\begingroup\$

Lyle's Mug gave a nice answer regarding how to improve the JavaScript overall. I'd like to instead look at the HTML:

  • Don't put an input inside a heading. HTML elements have meaning, and an <h1> element means "this is a heading for the document or a part of the document". But it seems you're just using it to make the input larger. Besides, an input is not a heading. Making it larger should be handled by CSS instead, since it's a purely visual thing.

  • No need to use multiple <br> tags. Again, it seems you're using them to do layout, but, again, layout and styling should be done with CSS.

  • I see that Greg Burghardt just beat me to it, but I too was going to suggest using a class for styling completed items. Don't add inline styling.

\$\endgroup\$
7
\$\begingroup\$

3 things

  1. Comments
  2. Consistency
  3. Fewer new lines

  1. We all know that this is the Enter Key, but you should give a comment why you are using a Magic Number here

    $('#new-text').keyup(function(e)
    {
    
        if (e.keyCode === 13)
    
        {
            addListItem();
        }
    
    });
    
  2. You use different bracing styles throughout your code, and it gets a little confusing.

    function completed() {
    if
    (
         $(this).parent().css('textDecoration') == 'line-through' )
    {
         $(this).parent().css('textDecoration', 'none');
         $(this).parent().css('opacity', '1');
    }
    
    else
    
    {
         $(this).parent().css('textDecoration', 'line-through');
         $(this).parent().css('opacity', '0.50');
    }
    
    }
    

    This is the only block of code that uses the "normal" bracing style for JavaScript, but the if statements don't use that same style.

    if (condition) {
        //code
    }
    

    This is how JavaScript is normally braced

  3. don't put a new line between the if (condition) { and the block of code. Just use less new lines.


looking at this piece of code I noticed that you put the condition on a new line, please don't do that.

function completed() {
if
(
     $(this).parent().css('textDecoration') == 'line-through' )
{
     $(this).parent().css('textDecoration', 'none');
     $(this).parent().css('opacity', '1');
}

else

{
     $(this).parent().css('textDecoration', 'line-through');
     $(this).parent().css('opacity', '0.50');
}

}

Doesn't this look nicer?

function addListItem() {
    var text = $('#new-text').val();
    $("#todo-list").append('<li ><input type="checkbox" class="toggle"/ ><span class="text">' + text + ' </span><button class="destroy"></button></li>');
    $("#new-text").val('');
}

function clearCompleted() {
    $("#todo-list .toggle:checked").parent().remove();
}

function deleteItem() {
    $(this).parent().remove();
}

function completed() {
    if ( $(this).parent().css('textDecoration') == 'line-through' )
    {
        $(this).parent().css('textDecoration', 'none');
        $(this).parent().css('opacity', '1');
    }
    else
    {
        $(this).parent().css('textDecoration', 'line-through');
        $(this).parent().css('opacity', '0.50');
    }
}


$(document).ready(function(){
    $('#new-text').keyup(function(e)
    {
        if (e.keyCode === 13)
        {
            addListItem();
        }
    });
    $(document).on('click', '.destroy', deleteItem);
    $("#toggle-all").click(function () {
        $('input:checkbox').not(this).prop('checked', this.checked);
        if ( $('li').css('textDecoration') == 'line-through' ) {
            $('li').css('textDecoration', 'none');
            $('li').parent().css('opacity', '1');
        } else {
            $('li').css('textDecoration', 'line-through');
            $('li').parent().css('opacity', '0.5');
        }
    });
    $(document).on('click', '.toggle', completed);
    $("#clearcompleted").click(clearCompleted);
    $('#todo-list').on('dblclick', 'span', function () {
        var thisData = this.innerHTML,
            $el = $('<input type="text" class="in-edit-text"/>');
        $(this).replaceWith($el);
        $el.val(thisData).focus();
        $(this).find(".text").hide();
        $(this).find(".destroy").hide();
    });

    $('#todo-list').on('keyup', '.in-edit-text', (function(e) {
        if (e.keyCode === 13) {
            $(this).replaceWith($('<span class="text">' + $(this).val() + '</span>'));
        }
        if (e.keyCode == 27) {
            $('.in-edit-text').remove();
        }
    })
});
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Grammatical thing: "less newlines" -> "fewer newlines" \$\endgroup\$
    – Flambino
    Commented Jun 15, 2015 at 20:14
6
\$\begingroup\$

This will be my first review, so bare with me. I'm going to step though every function:


First, I'd start with saving the reference to the todoList. This way, you only call it once and can use it thoughout the script:

var $todoList = $('#todo-list'); // I use the $ to indicate a jQuery Object
var $newText = $('#newText'); // Dito for newText

addListItem()

var AddListItem_prependString = '<li><input type="checkbox" class="toggle" /><span class="text">';
var AddListItem_appendString  = '</span><button class="destroy"></button></li>'
function addListItem(){
    $todoList.append(AddListItem_prependString + $('#new-text').val() + AddListItem_appendString);
    $newText.val('');
}
  • I prefer the open bracket on the function line, but is just an opinion, but IMO this creates an easier to read file
  • I took the strings in your append() and turned them into vars. The vars in the function now explain their value, without creating a long line
    • This removes a little hardcoding, you don't need the exact value to understand what it does
    • By placing them before the function, they exist in the global scope.
      This makes the function more lightweight, takes less memory on call
  • Used my predefined selectors to $todoList and $newText, saving two DOM-lookups

clearCompleted()

function clearCompleted(){
    $todoList.find(".toggle:checked").closest('li').remove();
}
  • The bracket one line up, IMO better
  • Used my predefined var $todoList, saving a DOM-lookup
    • I also used find. It's (almost?) always faster to first select an ID, then find the child element. ID's are always superfast and allow jquery to use document.getElementById()
  • I changed .parent() to .closest('li'). .parent() might be faster, but if you change your HTML, all your code will break. Now it won't.

deleteItem()

function deleteItem(){
    if( confirm("Are you sure?") ){
        $(this).closest('li').remove();
    }
}
  • The bracket one line up, IMO better
  • I changed .parent() to .closest('li'). .parent() might be faster, but if you change your HTML, all your code will break. Now it won't.
  • I've added a confirm. This is optional, but users might click it accidentally

toggleItemCompleted()

function toggleItemCompleted() {
   $(this).closest('li').toggleClass('completedItem') )
}
  • Changed function name to 'toggleItemCompleted', which is a lot more descriptive and is what your code is doing.
    • Styles should not be in Javascript, but in css
    • Checking a class is way faster than checking a style. Also, checking a style is foolish, but if you decide you dont want a linethrough anymore? You'd have to change all javascript because of style?!
  • Used .toggleClass because that's exactly what you want it to do now
  • I changed .parent() to .closest('li'). .parent() might be faster, but if you change your HTML, all your code will break. Now it won't.

Various document.ready code

$(document).ready(function(){
    $(document).on('click', '.destroy', deleteItem);
    $(document).on('click', '.toggle', toggleItemCompleted);
    $("#clearcompleted").click(clearCompleted);

    $('#new-text').keyup(function(e){
        // Check if enter is pressed:
        if (e.keyCode === 13){ addListItem(); }
    });


    $("#toggle-all").click(function (){
        // Set all LI's as compelted/uncompleted based in the toggle-all checkbox
        $todoList.find('li').addClass('completedItem', this.checked);
    });



    $('#todo-list').on('dblclick', 'span', function (){
        $el = $('<input type="text" class="in-edit-text"/>');
        $(this).replaceWith($el);
        $el.val( this.innerHTML ).focus();
        $(this).find(".text, .destroy").hide();
    });

    $('#todo-list').on('keyup', '.in-edit-text', function(e){
        // Check if enter is pressed:
        if (e.keyCode === 13){
            $(this).replaceWith($('<span class="text">' + $(this).val() + '</span>'));
        }

        // Check if XXX is pressed:
       if (e.keyCode == 27) {
            $('.in-edit-text').remove();
        }
    });

});
  • Removed all newlines, this is an opinion, but I think this makes an easier to udnerstand file
  • Placed all eventhandlers with a function together, easier to expand.
    • They kinda got lost mid-code
  • The #toggle-all uses a class now, saving csslines.
    • The toggle has a 2nd param, true will add the class, false remove. We use this
  • Added comment about which key we're ooking for
  • the todo-list doubleclick didn;t need this.innerHTML in a var, it allready is
    • It allready exists in this, and you only use it once, no need to store it
\$\endgroup\$
1
\$\begingroup\$

One quick item that no one else seems to have mentioned is the use of an IIFE. I always recommend this. It basically creates a private scope for your code and prevents your code from polluting the global scope (which is a very bad thing). The more code you have in the global scope the more chance of your code conflicting with someone else's code.

Here is how to do it:

(function( $ ) {
  //your code here
})( jQuery );

At the bottom of the IIFE you can add a single object to the global scope and have it contain all of your code:

window.toDo = {
  add : addListItem,
  clear : clearCompleted,
  //etc
};

You then refer to the functions like so:

$('#new-text').on ('keyup', function(e) {
  if (e.keyCode === 13) { //enter key
    toDo.add();
  }
});

Hope that helps!

\$\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.