HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptModerate

To-do list in jQuery

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
listjquerystackoverflow

Problem

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(''
+ text + ' ');
$("#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,
$e

Solution

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

Code Snippets

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

    if ( !$item.hasClass('completed') ) {
        $item.addClass('completed');
    } else {
        $item.removeClass('completed');
    }
}

Context

StackExchange Code Review Q#93664, answer score: 14

Revisions (0)

No revisions yet.