4
\$\begingroup\$

For an HTML calendar with jQuery, I put in event listeners to enable jQuery drag-and-drop that reads and writes to persistence using HTTP get and post with JSON structures.

Now I'd like to make the code more readable and easier to follow and understand. Can you let me know suggestions on how to improve? My plan is to make smaller moduler with clear responsibilities. The purpose of the script is to enable persistent data in jQuery wysihtml5 window. I use PHP from my previous question where I wrote a PHP handler to create a JSON response.

function getDateDetails($day, $month, $year, $username, $db)
    {   

    $sql = <<<SQL
SELECT calendar_images.url, 
calendar_images.id AS image_id, 
calendar_images.description,
calendar_images.category_id,
calendar_images.is_public,
calendar_dates.id,
calendar_date_image.slot_id
FROM calendar_images
INNER JOIN calendar_date_image
ON calendar_images.id = calendar_date_image.image_id
INNER JOIN calendar_users
ON calendar_users.username = '$username'
INNER JOIN calendar_dates
ON calendar_date_image.date_id = calendar_dates.id
WHERE calendar_date_image.user_id= calendar_users.id
AND calendar_dates.day = '$day'
SQL;

    $isFirst = true;
    if (!$result = $db->query($sql))
        {
        die('There was an error running the query [' . $db->error . ']');
        }

    $ret = array();
    while ($row = $result->fetch_assoc())
        {
        $names = array_filter(explode(";", $row['url']));
        $row_array = array(
            'id' => $row['image_id'],
            'name_day' => $row['url']
        );
         array_push($ret,$row_array);
        }

    $ret = json_encode($ret);
    return ($ret);
    }

My development plan is to rewrite the jQuery code that uses the JSON response so that the code below will be structured into smaller and clearer units and maybe in a separate file.

$(document).ready(function() {

//<div id="holidays" class="droppableHolidayArea infobox ui-droppable"><div data-parent-holiday-id="holiday-1" class="holidayWrapper holidays ui-draggable ui-draggable-handle" style="position: relative; z-index: 999; left: 0px; top: 0px;"><div class="title"> Allhelgonafton</div><div class="body"> Nu firar man!</div></div></div>
    $('#holidays').bind("DOMSubtreeModified", function(event) {
    alert('test');
        if ($('#modal-droppable-0 > img').attr('id') > 0) {

        }
    });

    /* Text area functionality TODO: per user, per month, many images, åäö, popup rendering, prepopulate här kan du skriva */
    $('#modal-droppable-0').bind("DOMSubtreeModified", function(event) {
        if ($('#modal-droppable-0 > img').attr('id') > 0) {
            var date_month = $('.dateDayModal > h2').text();
            $.post("php/updateImage.php", {
                    image_id: $('#modal-droppable-0 > img').attr('id'),
                    day: date_month,
                    username: 'bd107a66ba'
                },
                function(data) {
                    if (data == 0) {
                        $.post("php/addImage.php", {
                            image_id: $('#modal-droppable-0 > img').attr('id'),
                            day: date_month,
                            slot_id: '0',
                            username: 'bd107a66ba'
                        });
                    }
                });
        }
    });
    $('#modal-droppable-1').bind("DOMSubtreeModified", function() {
        if ($('#modal-droppable-1 > img').attr('id') > 0) {
            var date_month = $('.dateDayModal > h2').text();
            $.post("php/updateImage.php", {
                    image_id: $('#modal-droppable-1 > img').attr('id'),
                    day: date_month,
                    username: 'bd107a66ba'
                },
                function(data) {
                    if (data == 0) {
                        $.post("php/addImage.php", {
                            image_id: $('#modal-droppable-1 > img').attr('id'),
                            slot_id: '1',
                            day: date_month,
                            username: 'bd107a66ba'
                        });
                    }
                });

        }
    });
    $('#modal-droppable-2').bind("DOMSubtreeModified", function() {
        if ($('#modal-droppable-2 > img').attr('id') > 0) {
            var date_month = $('.dateDayModal > h2').text();
            $.post("php/updateImage.php", {
                    image_id: $('#modal-droppable-2 > img').attr('id'),
                    day: date_month,
                    slot_id: '2',
                    username: 'bd107a66ba'
                },
                function(data) {
                    if (data == 0) {
                        $.post("php/addImage.php", {
                            image_id: $('#modal-droppable-2 > img').attr('id'),
                            day: date_month,
                            slot_id: '2',
                            username: 'bd107a66ba'
                        });
                    }
                });

        }
    });
    var editor = new wysihtml5.Editor("textarea", {
        toolbar: "toolbar",
        stylesheets: "css/stylesheet.css",
        parserRules: wysihtml5ParserRules
    });
    $('#modalButtonRow').click(function(event) {
        $('#modal-droppable-0').empty();
        var textarea = document.getElementById("textarea");
        var date_month = $('.dateDayModal > h2').text();
        $.post("php/updateTextarea.php", {
                textdata: textarea.value,
                day: date_month,
                username: 'bd107a66ba'
            },
            function(data) {
                if (data == 0) {
                    var date_month = $('.dateDayModal > h2').text();
                    $.post("php/postpopupform.php", {
                        textdata: textarea.value,
                        day: date_month,
                        username: 'bd107a66ba'
                    });
                }
            })
    });
    $("#calendarAnchor").click(function(event) {
        var text = "Idag <b>2014</b> " + $(event.target).text();
        $.getJSON("php/jsonHandler.php?action=GET_DATE_DETAILS&day=" + $(event.target).text() + "&month=11&year=2014&username=bd107a66ba", function(data) {
            $.each(data, function(i, obj) {
                if (i == 'dateDescription') {
                    editor.setValue('' + obj, true);
                }
            });

        });
        $.getJSON("php/getImagesJson.php?action=GET_DATE_DETAILS&day=" + $(event.target).text() + "&month=11&year=2014&username=bd107a66ba", function(data) {
            var count = 0;
            $.each(data, function(i, obj) {             
                if (i == 'id') {
                    id = obj.id;
                }
                if (count == 0) {
                    $('#library-' + obj.id).empty();    
                    $('#modal-droppable-0').prepend('<img id="' + obj.id + '" src="' + obj.name_day + '" data-parent-icon-id="library-' + obj.id  + '" class="icon ui-draggable ui-draggable-handle" style="position: relative; z-index: 999; left: 0px; top: 0px;" data-slot="modal-droppable-0" />');
                }else
                 {
                    $('#library-' + obj.id).empty();    
                    $('#modal-droppable-1').prepend('<img id="' + obj.id + '" src="' + obj.name_day + '" data-parent-icon-id="library-' + obj.id  + '"class="icon ui-draggable ui-draggable-handle" style="position: relative; z-index: 999; left: 0px; top: 0px;" data-slot="modal-droppable-1" />');
                }
                count++;
            });
        });
    });
})
\$\endgroup\$
  • 1
    \$\begingroup\$ It would be really helpful to see your DOM in your HTML, can you make a JSFiddle for this? \$\endgroup\$ – netpoetica Oct 22 '14 at 2:44
3
+50
\$\begingroup\$

For the PHP code:

  • Read up on using prepared statements so you don't get any inadvertant SQL injection

  • The $month and $year parameters don't seem to be used anywhere.

  • The $isFirst and $names variable does't seem to be used anywhere.
  • Why use die? Why not just put the error in a JSON string and return that?

Other than that the code isn't very pretty but it gets the job done. It could be improved by splitting it up into functions (or class methods) with separate concerns so you don't need to pass the DB object to the same function you feed the day to...

For the jQuery code:

  • For anything that is repeated more than once, you should consider putting it in a variable. username: 'bd107a66ba' is a prime candidate. So are various recurring jQuery selectors.
  • Blocks of code that look almost the same except for a number (0/1/2/) could just be one function with that number as a parameter. That would reduce your line's of code quite a bit...
  • Splitting long lines of code into several lines helps readability
  • in regards to creating smaller units, instead of having nested functions, try declaring your functions first and reference them by name from your event handlers

The following example apply this to the #calendarAnchor click event handler. It may look/feel more verbose but having more smaller blocks helps re-usability and readability immensely

var count;

function prependImage(i, obj) {             

    if (i == 'id') {
        id = obj.id;
    }

    if (count != 0) {
        count = 1;
    }

    $('#library-' + obj.id).empty();    

    $('#modal-droppable-' + count).prepend(
        '<img id="' + obj.id + '"'+
        ' src="' + obj.name_day + '"' +
        ' data-parent-icon-id="library-' + obj.id  + '"' +
        ' class="icon ui-draggable ui-draggable-handle"' +
        ' style="position: relative; z-index: 999; left: 0px; top: 0px;"' + 
        ' data-slot="modal-droppable-' + count + '"' + 
        '/>'
    );

    count++;
}

function prependImagesFromJsonData(data) {
    count = 0;
    $.each(data, prependImage);
}

function setValuesFromJsonData(data) {
    $.each(data, function(i, obj) {
        if (i == 'dateDescription') {
            editor.setValue('' + obj, true);
        }
    });

}

function buildUrl(url, event) {
    return 'php/' + url + '.php' +
        '?action=GET_DATE_DETAILS' +
        '&day=' + $(event.target).text() +
        '&month=11&year=2014&username=bd107a66ba'
    ;
}

function calendarAnchorClickHandler {
    $.getJSON(buildUrl('jsonHandler', event), setValuesFromJsonData);
    $.getJSON(buildUrl('getImagesJson', event), prependImagesFromJsonData);
}

$("#calendarAnchor").click(calendarAnchorClickHandler);
\$\endgroup\$

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.