5
\$\begingroup\$

I have been doing some JS lately and I would like to get some constructive criticism.

The project I am working on

var SpotifiedList = [],
youtubeSongs, spotifySongs = 0,
spotifiedSongs = 0,
playNext;

$(document).ready(function () {
  var id = window.location.hash;
  if (id) {
    $("#playlistID").val(id.substring(1));
    startProcess();
  }
});

function resetTimer() {
  console.log("timer reset!")
  clearTimeout(playNext);
}

function startProcess() {
  $("#errorOccured").hide();
  if ($("#playlistID").val().length < 5) { // Checks if there is something entered
    $(".alert-danger").fadeIn().html("Please enter a valid Youtube <b>playlist ID</b>.");
  } else {
    window.location.hash = $("#playlistID").val();
    $("#list").html('<center>This might take some time.</center>');
    $(".alert-info").html("<center>Loading</center>");
    $(".alert-danger").hide();
    retrieveYoutubePlaylist($("#playlistID").val(), 1, [], 1);
  }
}

function retrieveYoutubePlaylist(id, startIndex, list, k) {
  $("#loading").show();
  $.ajax({
    url: 'https://gdata.youtube.com/feeds/api/playlists/' + id + '?v=2&alt=json&start-index=' + startIndex + '&max-results=50',
    dataType: 'json',
    success: function (data) {
      if (typeof data.feed.entry === 'undefined') {} else {
        for (var i = 0; i < data.feed.entry.length; i++) {
          if (k == 1) $("#list").html("");
          var title = data.feed.entry[i].title.$t;
          list.push(title);
          retrieveSpotify(title);
          k++;
        }
        retrieveYoutubePlaylist(id, startIndex + 50, list, k);
        youtubeSongs = list.length;
      }
    },
    error: function (data, a, err) {
      if (err) {
        $(".alert-danger").fadeIn().html("Error Happened: " + err);
      }
    }
  });
}

function retrieveSpotify(title) {
  var url = 'http://ws.spotify.com/search/1/track.json?q=' + parseTitle(title);
  $.getJSON(url, function (data) {
    spotifySongs += 1;
    if (data.tracks.length > 0) {
      spotifiedSongs += 1;
      $("#list").append('<li><a href="' + data.tracks[0].href + '" onClick="resetTimer();" title="' + data.tracks[0].artists[0].name + ' - ' + data.tracks[0].name + '">' + data.tracks[0].artists[0].name + ' - ' + data.tracks[0].name + '</a></li>');
      var info = {
        'url': data.tracks[0].href,
        'length': Math.round(data.tracks[0].length * 1000),
        'name': data.tracks[0].artists[0].name + ' - ' + data.tracks[0].name
      }
      SpotifiedList.push(info);
    }
    if (youtubeSongs == spotifySongs) {
      $(".alert-info").html('<div class="btn-group">\
        <button type="button" class="btn btn-default" onClick="play();">\
        <span class="glyphicon glyphicon-play"></span> Play/Skip\
        </button>\
        </div><br>Converted songs: <strong>' + spotifiedSongs + '</strong>/' + youtubeSongs);
    }
  });
}

function parseTitle(title) {

  var newTitle = title
  .replace(/[-!$%^&_+|~=`{}:";'<>?,.\/]/g, '')
  .replace(/  /g, ' ')
  .replace(/\[.*\]/g, '')
  .replace(/lyrics/g, '')
  .replace(/\(.*\)/g, '')
  .replace(/\*.*\*/g, '')
  .replace(/   /g, ' ');

  return newTitle;
}

function play() {
  clearTimeout(playNext);
  var song = SpotifiedList[Math.floor((Math.random() * SpotifiedList.length))];
  window.location = song.url;
  x = (song.length / 1000) >> 0;
  seconds = x % 60
  x = (x / 60) >> 0;
  minutes = x % 60;
  console.log('Playing: ' + song.name + ' (' + minutes + ':' + seconds + ')');
  playNext = setTimeout(play, song.length);
}
\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$

General

I could work with this code, I only have a few remarks.

Globals

You should consider as a minimum to have your 4 globals in an object.

var player = 
{
  spotifiedList : [],
  youtubeSongs : 0,
  spotifySongs : 0,
  spotifiedSongs : 0,
  playNext : 0
}

You could even add most of the functions to the player object

resetTimer

You should remove the console.log, which makes this a simple one liner. If you keep it ( function name neatly explains what it does ), make sure to call it from play() instead of doing a straight clearTimeout(playNext).

startProcess

You could consider caching the jQuery queries, maybe in the before proposed player object.

retrieveYoutubePlaylist

if (typeof data.feed.entry === 'undefined') {} else { is just wrong.

retrieveSpotify

Encoding onclick= is wrong as well. Building HTML should be contained to separate functions.

parseTitle

I have no idea what that does, a few comments are needed there

play

I am curious, what does >> 0 get you ? Force it into an integer?

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