Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  • Read up on using prepared statements 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?

  • 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?

  • 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?

Bounty Awarded with 50 reputation awarded by Niklas Rosencrantz
Source Link
Potherca
  • 530
  • 3
  • 10

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

AltStyle によって変換されたページ (->オリジナル) /