5
\$\begingroup\$

I'm looking advice from more experienced people about how I can improve my code. I also want to make my code object-oriented (create classes in separate .js files) but I don't how best to do it.

How the program works:

When the user selects a button, the data-btn attribute is checked if that attribute is between (-1 to 4) the cokeAPI is called, otherwise the other server is called.

Both server return similar data, but in different formats.

That data is then added to a li and returned in an animated form to the user.

I'm currently working on extracting data from both api calls and using each item to define a "marker" in a Google Map but I want to do this in an OOP way.

  "use strict";

  (function($, window, document) {

  $(function() {

      $('#holder').on('click','button', populateListBasedOnSelectedCity);
      $('.mapBtn').on('click', getTheCityTheUserSelected);

  });//$(function()

  // The request to the Coke API
  function getCokeAPIData(city) {

    let proxy = 'https://cors-anywhere.herokuapp.com/';

    $.ajax({
      type: 'POST',
      url: proxy + "https://linkToWebsite",
      data: {
       key: 'myKey',
       schemeId : city
      },
      dataType: "json",
      statusCode: {
        404: function() {
          alert( "page not found" );
        }
      },
      success: processCokeBikeData,
      error : errorResponseFromServer,
      complete : completionResponseFromServer
    });//ajax

    $('#serverStatus').html("<p>Loading Data</p>");

  }//getCokeAPIData

  // This is an AJAX request to a OtherServerbikes API
  function getDataFromOtherServer() {

    $.ajax({
      url: "https://api.linkToOtherWebsite.json",
      success: successfulResponseFromOtherServer,
      error : errorResponseFromServer,
      complete : completionResponseFromServer
    });//ajax

    $('#serverStatus').html("<p>Loading Data</p>");

  }//getDataFromOtherServer

  function successfulResponseFromOtherServer(result){
    let entry, listOfBikesFromServer = '';

    $('#date').html("<p>Data returned for OtherServer</p>");
    console.log("Data returned at : " + result[0].timestamp + "\nData returned for OtherServer\nThere was " + result.length + " results returned");

    for (entry in result) {
      listOfBikesFromServer += "<li>" + result[entry].bikes + " bikes available at "  +  result[entry].name.toLowerCase()  +  "<br /> " + result[entry].free  +  " docks available.</li>";
    }

    $("#myList").append(listOfBikesFromServer);
    $("#loader img").fadeOut(800);
    activateAnimations();
  }//successfulResponseFromOtherServer

  // Processes the Coke bike data.
  function processCokeBikeData(result) {

    $('#serverStatus').html("<p>Data is received</p>");
    let arrayLength = result.data.length, i = 0, cokeBikesReturnedFromServer = '';

    $('#date').html("<p>Data returned for " + result.data[0].schemeShortName + "</p>");

    //result.data.forEach(function(element) {
        //console.dir(element);
    //});

    console.log("Data returned at : " + result.responseDate + "\nData returned for " + result.data[0].schemeShortName + "\nThere was " + result.data.length + " results returned");
    //$('#date').html("<p>Data returned at : " + result.responseDate + "</p><p>Data returned for " + result.data[0].schemeShortName + "</p><p>There was " + result.data.length + "  results returned");

    for(i ; i < arrayLength ; i++) {
     cokeBikesReturnedFromServer += "<li>" + result.data[i].bikesAvailable + " bikes available at "  + result.data[i].name  + "<br /> " + result.data[i].docksAvailable  + " docks available.</li>";
    }//forloop

    $("#myList").append(cokeBikesReturnedFromServer);

    activateAnimations();

    $("#loader img").fadeOut(800);
  }//processCokeBikeData

  // This is a AJAX request to a local copy of the JSON data for testing the data.
  function getLocalData() {

    $.getJSON( "allData.json", function( json ) {

      let arrayLength = json.data.length;
      let i = 0;

      alert("ResponseCode : " + json.responseCode + "\nResponseText : " + json.responseText + "\nResponseDate : " + json.responseDate);

      for(i ; i < arrayLength ; i++) {
        printBikeData(i,json);
       }//forloop


    });//getJSON

  }//getLocalData

  function activateAnimations() {
    $("#myList li").velocity("transition.slideLeftIn", { stagger: 300,  drag: true });
  }//activateAnimations

  function errorResponseFromServer( jqXHR,textStatus,errorThrown ) {
    alert("jqXHR : " +  jqXHR.statusText  +  "\njqXHR xml " + jqXHR.readyState + "\ntextStatus :" +textStatus + "\nerrorThrown : " + errorThrown);
  };

  function completionResponseFromServer(jqXHR, textStatus ) {
    $('#serverStatus').html("<p>Complete</p>");
  }

  function getTheCityTheUserSelected() {
    let latitude = $(this).attr('data-lat');
    let longitude = $(this).attr('data-lng');
    let cityCoordinates = [latitude,longitude];
    initialiseMap(cityCoordinates);
  }

  // takes coordinates from the user and creates a map centered at the coordinates.
  function initialiseMap(_coordinates) {
    let options = { zoom: 14, center : new google.maps.LatLng(_coordinates[0],_coordinates[1]) }
    let map = new google.maps.Map(document.getElementById('map'), options);
  }// initialise

  // When one of the buttons is clicked either the Coke or OtherServerBikes API is called.
  function populateListBasedOnSelectedCity() {

    $("#loader img").fadeIn(800).attr('src',"gears.svg");

    //  responseFromUser represent the city that was selected.
    var responseFromUser = $(this).data('btn');
      switch (responseFromUser) {
        case -1:
        case  2:
        case  3:
        case  4:
          getCokeAPIData(responseFromUser);
          break;
        case  5:
          getDataFromOtherServer();
      }//switch

    $("#myList").empty();
  }//populateListBasedOnSelectedCity

  }(window.jQuery, window, document));
\$\endgroup\$
2
\$\begingroup\$
url: proxy + "https://linkToWebsite",

Consider using template literals. That way, you can interpolate strings without having a disjointed string.

You also don't need to comment on the closing bracket. That's just code noise. A good IDE will be able to fold code blocks and highlight the opening bracket when you point or place the cursor on the closing bracket (and vice versa).

Your code requires nothing special from $.ajax (headers, auths, content types) you can simply use the shorthand $.post and $.get from jQuery. Also consider using the promise interface (promise.then(successFn, failFn)) instead of success and error since it's already a standard, provides better integration and flow management than the callbacks version.

alert("jqXHR : " +  jqXHR.statusText  +  "\njqXHR xml " + jqXHR.readyState + "\ntextStatus :" +textStatus + "\nerrorThrown : " + errorThrown);

Do not use alert or console.log for debugging. Use the Sources panel of your browser debugger to add breakpoints and look at values of variables.

function getCokeAPIData(city) {
  $.ajax({ ... });//ajax

  $('#serverStatus').html("<p>Loading Data</p>");
}

Suggesting you don't mix UI logic with your data logic. This makes this function coupled to UI logic. What I usually do is write them separately, one that retrieves data and another just updates the UI. A third function which describes the flow (usually business logic), will wire them together. Often times, it's the wiring function that will change.

function processCokeBikeData(result) {

    $('#serverStatus').html("<p>Data is received</p>");
    let arrayLength = result.data.length, i = 0, cokeBikesReturnedFromServer = '';

    $('#date').html("<p>Data returned for " + result.data[0].schemeShortName + "</p>");

    //result.data.forEach(function(element) {
        //console.dir(element);
    //});

    console.log("Data returned at : " + result.responseDate + "\nData returned for " + result.data[0].schemeShortName + "\nThere was " + result.data.length + " results returned");
    //$('#date').html("<p>Data returned at : " + result.responseDate + "</p><p>Data returned for " + result.data[0].schemeShortName + "</p><p>There was " + result.data.length + "  results returned");

    for(i ; i < arrayLength ; i++) {
     cokeBikesReturnedFromServer += "<li>" + result.data[i].bikesAvailable + " bikes available at "  + result.data[i].name  + "<br /> " + result.data[i].docksAvailable  + " docks available.</li>";
    }//forloop

    $("#myList").append(cokeBikesReturnedFromServer);

    activateAnimations();

    $("#loader img").fadeOut(800);
}

You might want to consider using a tiny UI framework, or at least a template library to construct your UI. Doing it this way in JS makes the UI hard to visualize, and code hard to maintain.

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