Create a separate JSON + syntax

Question

My code already works perfectly. Here is the JS file:

var movieData = {
   count: 6,
   movies: [{
       id: 1,
       title: "Lorem Ipsum",
       thumb: "assets/img/placehold.png"
     },
     {
       id: 2,
       title: "Lorem Ipsum",
       thumb: "assets/img/placehold.png"
     },
     {
       id: 3,
       title: "Lorem Ipsum",
       thumb: "assets/img/placehold.png"
     },
     {
      id: 4,
      title: "Lorem Ipsum",
      thumb: "assets/img/placehold.png"
    },
    {
      id: 5,
      title: "Lorem Ipsum",
      thumb: "assets/img/placehold.png"
    },
    {
      id: 6,
      title: "Lorem Ipsum",
      thumb: "assets/img/placehold.png"
    },
   ]
 };

 $(document).ready(function() {

    if (typeof movieData === 'object' && typeof movieData !== null) {
      // List the movies
      for (var i in movieData.movies) {
         var movie = movieData.movies[i];
         var movieDiv = 

         '<li class="movie-item" data-id="' + movie.id + '">' +
            '<a href="#">' +
               '<img src="' + movie.thumb + '" width="280" height="150" />' +
               '<span class="text-content"><i class="fa fa-chevron-up" aria-hidden="true"></i><br><br><i class="fa fa-4x  fa-play-circle-o"></i><br><br>' + movie.title + '</span></span>' +
            '</a>' +
         '</li>';

         $('#films').append(movieDiv);
      }
    }

 });

I actually have everything into one file. As a code review I am asking your help if you have any idea about how to improve the code.

Knowing that as an improvement I already though about making this code into 2 different files (one JS and one JSON) but I would like to know your point of view.

Answer 1

I would recommend removing count if all it does is represent the number of items in movies. Otherwise, you'll have 2 sources of truth in the data (count and the length of movies) and code that takes sides down the line (one that believes in count and one that believes in the length of movies). So as much as possible, avoid duplicating data.

Now it's not always feasible to recompute data all the time (i.e. if you had to filter thousands of rows to count "selected" items). This is one case where I'd put a fixed number instead of recomputing it from some property. Just make sure it's documented somewhere.

for (var i in movieData.movies) {

Use regular for loops when looping through arrays. The problem with for-in is that it runs through all enumerable properties, indices and other enumerable properties. You might be getting more than what's just in the array. An even better suggestion is to use array.forEach.

$('#films').append(movieDiv);

The problem with this approach is that you're doing a jquery.append on every item. That means the browser has to re-render every time you add an item. What I suggest is to create a DOM element in memory and append to that in your loop. Then, at the very end, slap the contents of that DOM element onto the page once. This way, the browser only ever renders once.

// Creates a <ul> that's in memory, not yet appended to the DOM.
const replacementFilms = $('<ul/>', { id: '#films' })

movieData.movies.forEach(movie => {
  var movieDiv = '...the markup...'

  // Appends to our <ul> that's "in memory".
  replacementFilms.append(movieDiv)
})

// Replace the #films in the DOM with the one in memory in one go.
$('#films').replaceWith(replacementFilms)

In this example, I replaced the existing #films in the DOM. But you could also create a dummy element, append the items to it, then append its contents to a container element in the DOM. The operation doesn't have to be a total replace. But the idea is that you only ever touch the DOM once.

     '<li class="movie-item" data-id="' + movie.id + '">' +
        '<a href="#">' +
           '<img src="' + movie.thumb + '" width="280" height="150" />' +
           '<span class="text-content"><i class="fa fa-chevron-up" aria-hidden="true"></i><br><br><i class="fa fa-4x  fa-play-circle-o"></i><br><br>' + movie.title + '</span></span>' +
        '</a>' +
     '</li>';

See Template Literals for multi-line strings and string interpolation.