2
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer ! What do you mean by 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. How can do it can you please provide me more details about it. \$\endgroup\$ – Ced Jan 29 at 15:23
  • \$\begingroup\$ @Ced sure. Will update my answer in a bit. \$\endgroup\$ – Joseph Jan 29 at 15:26
  • \$\begingroup\$ Thank you for the update, this part still looks a bit unclear to me but I'll go though it this week-end. I appreciate the effort :) \$\endgroup\$ – Ced Jan 30 at 14:25

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.