3
\$\begingroup\$

I was asked to create a TODO manager using JavaScript and CSS. I did not get a good review on the code nor specific comments on how to improve it.

Can someone here help by give me tips on what is bad with my implementation?

<html>
<head>

    <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.7.2/jquery.min.js"></script>
    <script src="http://ajax.aspnetcdn.com/ajax/jquery.ui/1.8.13/jquery-ui.min.js"></script>

    <link rel="stylesheet" href="jquery.ui.all.css">
    <link rel="stylesheet" href="demos.css">

    <!-- for the linked list -->
    <script type="text/javascript" src="linked-list.js"></script>

    <!-- for the popup ui -->
    <script src="thickbox.js"></script>
    <link rel="stylesheet" href="thickbox.css">

    <style>
        a img {border:none;}

        #todo { float: left; width: 10%; min-height: 12em; } * html #todo { height: 12em; } /* IE6 */
        .todo.custom-state-active { background: #eee; }
        .todo li { float: left; width: 96px; padding: 0.4em; margin: 0 0.4em 0.4em 0; text-align: center; }
        .todo li h5 { margin: 0 0 0.4em; cursor: move; }
        .todo li a { float: right; }
        .todo li a.ui-icon-zoomin { float: left; }
        .todo li img { width: 100%; cursor: move; }

        #newtask { float: left; width: 30%; min-height: 60em; margin-left: 20px; margin-top:15px;} * html #newtask { height: 12em; } /* IE6 */
        .newtask.custom-state-active { background: #eee; }
        .newtask li { float: left; width: 96px; padding: 0.4em; margin: 0 0.4em 0.4em 0; text-align: center; }
        .newtask li h5 { margin: 0 0 0.4em; cursor: move; }
        .newtask li a { float: right; }
        .newtask li a.ui-icon-zoomin { float: left; }
        .newtask li img { width: 100%; cursor: move; }

        #inprogress { float: left; width: 30%; min-height: 60em; margin-left: 20px;margin-top:15px;} * html #inprogress { height: 12em; } /* IE6 */
        .inprogress.custom-state-active { background: #eee; }
        .inprogress li { float: left; width: 96px; padding: 0.4em; margin: 0 0.4em 0.4em 0; text-align: center; }
        .inprogress li h5 { margin: 0 0 0.4em; cursor: move; }
        .inprogress li a { float: right; }
        .inprogress li a.ui-icon-zoomin { float: left; }
        .inprogress li img { width: 100%; cursor: move; }

        #done { float: left; width: 30%; min-height: 60em; margin-left: 20px;margin-top:15px;} * html #inprogress { height: 12em; } /* IE6 */
        .done.custom-state-active { background: #eee; }
        .done li { float: left; width: 96px; padding: 0.4em; margin: 0 0.4em 0.4em 0; text-align: center; }
        .done li h5 { margin: 0 0 0.4em; cursor: move; }
        .done li a { float: right; }
        .done li a.ui-icon-zoomin { float: left; }
        .done li img { width: 100%; cursor: move; }

        .popupStyle { display: none; position: absolute; top: 30%; left: 40%; width: 25%; height: 16%; padding: 2px; border: 3px solid #555555; background-color: #c78505; z-index:1002; overflow: auto; font-family: tahoma, verdana, sans-serif;}
    </style>

    <script>
        // the four living states of a task
        var taskStateEnum = {
            NEW_TASK : 0,
            IN_PROGRESS : 1,
            TASK_DONE : 2,
            TASK_DELETED : 3    // if we ever want to implement the recover capability.  for now, its unused
        };

        // all tasks are held here
        var tasksList = new LinkedList();

        // monotonically increasing unique task ID
        var taskID = 0;

        $(function() {
            var $todo = $( "#todo" ),
                $newtask = $( "#newtask"),
                $inprogress = $( "#inprogress"),
                $done = $( "#done" );

            // let the todo items be draggable
            $( "li:gt(0)", $todo ).draggable({
                cancel: "a.ui-icon", // clicking an icon won't initiate dragging
                revert: "invalid", // when not dropped, the item will revert back to its initial position
                containment: $( "#demo-frame" ).length ? "#demo-frame" : "document", // stick to demo-frame if present
                helper: "clone",
                cursor: "move"
            });

            // let the inprogress be droppable, accepting the todo items
            $inprogress.droppable({
                accept: "#todo > li, #done li",
                activeClass: "ui-state-highlight",
                drop: function( event, ui ) {
                    itemDroppedInProgress( ui.draggable );
                }
            });

            // let the done be droppable, accepting the inprogress items
            $done.droppable({
                accept: "#inprogress li",
                activeClass: "ui-state-highlight",
                drop: function( event, ui ) {
                    itemDroppedInDone( ui.draggable );
                }
            });

            // let the todo be droppable as well, accepting items from the done and inprogress
            $todo.droppable({
                accept: "#inprogress li, #done li",
                activeClass: "custom-state-active",
                drop: function( event, ui ) {
                    recycleImage( ui.draggable );
                }
            });

            // let the todo be droppable as well, accepting items from the done
            $newtask.droppable({
                accept: "#inprogress li, #done li",
                activeClass: "ui-state-highlight",
                drop: function( event, ui ) {
                    itemDroppedInTodo( ui.draggable );
                    recycleImage( ui.draggable );
                }
            });

            // image deletion function
            var recycle_icon = "";

            function itemDroppedInTodo( $item ) {
                var id = $item.attr("id");
                moveTask(id, taskStateEnum.NEW_TASK);
                updateTaskCounters();
            }

            function itemDroppedInProgress( $item ) {
                $item.fadeOut(function() {
                    var $list = $( "ul", $inprogress ).length ?
                        $( "ul", $inprogress ) :
                        $( "<ul class='todo ui-helper-reset'/>" ).appendTo( $inprogress );

                    $item.find( "a.ui-icon-inprogress" ).remove();
                    $item.append( recycle_icon ).appendTo( $list ).fadeIn(function() {
                        $item
                            .animate({ width: "48px" })
                            .find( "img" )
                                .animate({ height: "36px" });
                    });
                });

                var id = $item.attr("id");
                moveTask(id, taskStateEnum.IN_PROGRESS);
                updateTaskCounters();
            }

            function itemDroppedInDone( $item ) {
                $item.fadeOut(function() {
                    var $list = $( "ul", $done ).length ?
                        $( "ul", $done ) :
                        $( "<ul class='inprogress ui-helper-reset'/>" ).appendTo( $done );

                    $item.find( "a.ui-icon-done" ).remove();
                    $item.append( recycle_icon ).appendTo( $list ).fadeIn(function() {
                        $item
                            .animate({ width: "48px" })
                            .find( "img" )
                                .animate({ height: "36px" });
                    });
                });

                var id = $item.attr("id");
                moveTask(id, taskStateEnum.TASK_DONE);
                updateTaskCounters();
            }

            // image recycle function
            var done_icon = "";
            function recycleImage( $item ) {
                $item.fadeOut(function() {
                    $item
                        .find( "a.ui-icon-refresh" )
                            .remove()
                        .end()
                        .css( "width", "96px")
                        .append( done_icon )
                        .find( "img" )
                            .css( "height", "72px" )
                        .end()
                        .appendTo( $todo )
                        .fadeIn();
                });
            }
        });

        // when the create button is clicked
        function onCreateTask() {
            tb_show("Provide some details about this task...", "#TB_inline?height=100&width=460&inlineId=divTaskDetails", "");

            return false;
        }

        // shows the details of the current task
        function onShowTask(task) {
            document.getElementById('divTaskContent').innerHTML = task;
            tb_show("Task Details...", "#TB_inline?height=300&width=400&inlineId=divTask", "");     

            return false;
        }

        // update the text on the screen that shows the various task counters
        function updateTaskCounters() {
            var total = 0, tasksInProgress = 0, tasksDone = 0, tasksInTodo = 0;

            for (i = 0; i < tasksList.size(); i++) {
                taskObj = tasksList.item(i);

                switch (taskObj.state) {
                case taskStateEnum.NEW_TASK:
                    tasksInTodo++;
                    total++;
                    break;
                case taskStateEnum.IN_PROGRESS:
                    tasksInProgress++;
                    total++;
                    break;
                case taskStateEnum.TASK_DONE:
                    tasksDone++;
                    total++;
                    break;
                default:
                    break;
                }
            }

            document.getElementById("inProgressCount").innerHTML = tasksInProgress + " Projects";
            document.getElementById("doneCount").innerHTML = tasksDone + " Projects";
            document.getElementById("todoCount").innerHTML = tasksInTodo + " Projects";
            document.getElementById("totalCount").innerHTML = "Total: " + total + " Projects";
        }

        function moveTask(task_id, where) {
            /* first find it then move it */
            if (task_id >= 0 && task_id < tasksList.size()) {
                var taskObj = tasksList.item(task_id);
                taskObj.state = where;
            }
        }

        function deleteTask(task_id) {
            /* first find it then move it */
            if (task_id >= 0 && task_id < tasksList.size()) {
                var taskObj = tasksList.item(task_id);
                taskObj.state = taskStateEnum.TASK_DELETED;
            }
        }

        // when a user creates a new task.  this adds it to the todo bin
        function onAddTask() {
            var taskData = document.getElementById("task").value;

            taskID++;
            var taskObj = {data: taskData, state: taskStateEnum.NEW_TASK, id:taskID};
            tasksList.add(taskObj);

            // always add the master task creater
            var html = "<li class='ui-widget-content ui-corner-tr'>" +
                    "<h5 class='ui-widget-header'>New Task</h5>" +
                    "<a href='' onclick='return onCreateTask();'><img src='images/newtask.png' width='96' height='72'/></a>" +
                    "Click to create" +
                    "</li>";

            // now fill in the tasks in the todo column
            for (i = 0; i < tasksList.size(); i++) {
                taskObj = tasksList.item(i);

                /* only show new tasks in the UL */
                if (taskObj.state != taskStateEnum.NEW_TASK) {
                    continue;
                }

                if (taskObj.data.length > 8) {
                    var displayText = taskObj.data.substring(0,8) + "...";
                } else {
                    var displayText = taskObj.data;
                }

                html += "<li id='" + i + "' class='ui-widget-content ui-corner-tr ui-draggable' style='width: 96px; display: list-item;'>"
                    + "<h5 class='ui-widget-header'>Click for details</h5>" 
                    + "<a onclick='return onShowTask(\""+ taskObj.data + "\");'><img src=images/task.png width=96 height=72></a>"
                    + "<p style='text-align:center;'>" + displayText + "</p>"
                    + "<a onclick='return onShowTask(\""+ taskObj.data + "\");' title='Details' class='ui-icon ui-icon-zoomin'>View details</a>"
                    + "<a title='Delete task' class='ui-icon ui-icon-trash'>Delete task</a>"
                    + "</li>";
            }

            // Set the innerHTML of the todo column to show all the Todo tasks
            var todoList = document.getElementById("todo");
            todoList.innerHTML = html;

            // This causes all of them to be draggable
            $("#todo li:gt(0)").draggable({
                cancel: "a.ui-icon", // clicking an icon won't initiate dragging
                revert: "invalid", // when not dropped, the item will revert back to its initial position
                containment: $( "#demo-frame" ).length ? "#demo-frame" : "document", // stick to demo-frame if present
                stop: function(event, ui) {
                    // updateTaskCounters();
                },
                drop: function( event, ui ) {
                    itemDroppedInTodo( ui.draggable );
                },
                helper: 'clone',
                cursor: "move"
            });

            // when the user wants to just trash a task compeletely
            $(".ui-icon-trash").on("click", function(e) {
                tb_show("Warning!", "#TB_inline?height=100&width=260&inlineId=divDelete", "");
                e.preventDefault();
                var id = $(this).parent("li").attr("id");
                deleteTask(id);
                updateTaskCounters();
                $(this).parent("li").remove();
            });

            updateTaskCounters();

            tb_remove();

            return false;
        }
    </script>

</head>
<body>

    <div id="totalCount" style="margin-left: 20px; font-size: 30px;">Total: 0 Projects</div>

    <!-- New task column.  Nw tasks are created here -->
    <div id="newtask" class="ui-widget-content ui-state-default">
        <h4 class="ui-widget-header">
            <span class="ui-icon ui-icon-done">New Task</span> New Task
            <div id="todoCount" style="float:right">0 Projects</div>
            <br>
            <br>
        </h4>

        <!-- todo column.  initially, it just has the task creater widget -->
        <ul id="todo" class="todo ui-helper-reset ui-helper-clearfix">
            <li class="ui-widget-content ui-corner-tr" id="0">
                <h5 class="ui-widget-header">New Task</h5>
                <a href="" onclick='return onCreateTask();'><img src="images/newtask.png" width="96" height="72"/></a>
                Create Task
            </li>
        </ul>
    </div>

    <!-- inprogress column.  tasks can only be dragged here from the todo column -->
    <div id="inprogress" class="ui-widget-content ui-state-default">
        <h4 class="ui-widget-header">
            <span class="ui-icon ui-icon-done">In Progress</span> In Progress
            <div id="inProgressCount" style="float:right">0 Projects</div>
            <br>
            <br>
        </h4>
    </div>

    <!-- Done column.  Tasks can only be dragged here from the inprogress column -->
    <div id="done" class="ui-widget-content ui-state-default">
        <h4 class="ui-widget-header">
            <span class="ui-icon ui-icon-done">Done</span> Done
            <div id="doneCount" style="float:right">0 Projects</div>
            <br>
            <br>
        </h4>
    </div>

    <!-- Task Details Popup -->
    <div id="divTaskDetails" class="popupStyle">
        <br>
        <br>
        <form id="taskForm" name="task" style="margin: 0; padding: 0" onsubmit="return onAddTask();">
            <input id="task" type="text" size="72" name="task">
            <input name="submit" type="submit" value="Submit" onclick="return onAddTask();">
        </form>
    </div>

    <!-- SHow Task Details Popup -->
    <div id="divTask" class="popupStyle">
        <div id="divTaskContent">
        </div>
    </div>

    <!-- Error Details Popup -->
    <div id="divDelete" class="popupStyle">
        <br>
        <br>
        <center>You have permanently deleted the task</center>
    </div>

    <!-- Background for all popups -->
    <div id="divFade" class="blankStyle">
    </div>

</body>
</html>
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Read the FAQ. This website is not for full code reviews, only for small snippets, algorithms, and such. \$\endgroup\$ Aug 15, 2012 at 20:29
  • 5
    \$\begingroup\$ @FlorianMargaine What makes you think that "full" code reviews aren't allowed? There might be a practicable limit to how large the code can be before people will decide to simply not read the question, but as far as the rules are concerned, the only requirement is that all relevant code be included in the question. \$\endgroup\$
    – sepp2k
    Aug 16, 2012 at 1:28
  • 1
    \$\begingroup\$ @sepp2k - If that's the case then, IMHO, the requirements should change. \$\endgroup\$ Aug 16, 2012 at 16:12

1 Answer 1

2
\$\begingroup\$

You code look ok but I suggust using a MVC library, since this will abstract and simplify your logic.

Check out this project on github, called TodoMVC. TodoMVC shows how to use popular MVC libraries to implement the popular 'todo list' project.

Examples can be found here.

The easiest MVC library to use is stapes.js

Here are a few tips for your code. 1) Be consistent. Use jQuery instead of raw DOM manipulation. Example:

Previous Code:

document.getElementById("inProgressCount").innerHTML = tasksInProgress + " Projects";
document.getElementById("doneCount").innerHTML = tasksDone + " Projects";
document.getElementById("todoCount").innerHTML = tasksInTodo + " Projects";
document.getElementById("totalCount").innerHTML = "Total: " + total + " Projects";

New Code:

$("#inProgressCount").html( tasksInProgress + " Projects" );
$("#doneCount").html( tasksDone + " Projects");
$("#todoCount").html( tasksInTodo + " Projects");
$("#totalCount").html( "Total: " + total + " Projects");

2) onAddTask() should be split up into multiple functions since it's longer then 8 - 12 lines.

3) Also, place all your CSS in external separate css files.

For more tips check out. https://github.com/rwldrn/idiomatic.js

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