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

More maintainable API for wysihtml5 widget

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

Problem

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 = 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() {

// Allhelgonafton Nu firar man!
$('#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,
u

Solution

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

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

Code Snippets

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

Context

StackExchange Code Review Q#67496, answer score: 3

Revisions (0)

No revisions yet.