Fetching movie data using jQuery

Question

• Requirements

  • Display movie datas inside all tags that have the class movie_list
  • Make an ajax call for fetching movie datas
  • Fill loading text before fetch data
  • If any error occurs during loading, the content should be empty

• What I did

  • Cached all elements which have the class movie_list
  • Made functions which have only one responsibility
  • Functions take movieList elements for making easy to unit test

• What I want to know

  • More readable/reusable code
  • Any performance improvement point
  • Any good design pattern on here

function loadMovies() {
 const url = 'http://moviedatas.com/movies';
 const movieList = $('.movie_list');
 setOnLoading(movieList);
 $.ajax({
 method: 'GET',
 url: url,
 data: {
 count: 10
 },
 success: function(response) {
 movieList.each(function() {
 const wrapper = $(this);
 render(wrapper, items);
 });
 },
 error: function(error) {
 onError(movieList);
 }
 })
}
function setOnLoading(movieList) {
 movieList.each(function() {
 $(this).html('Loading...');
 });
}
function render(wrapper, items) {
 const template = `<div class="movie_item">
 <div class="title">{title}</div>
 <div class="desc">{description}</div>
</div>`;
 let movies = '';
 for (let i = 0; i < items.length; i++) {
 movies += template
 .replace('{title}', items[i].title)
 .replace('{description}', items[i].description);
 }
 wrapper.html(movies);
}
function onError(movieList) {
 movieList.each(function() {
 $(this).html('');
 });
}

• Blindman67: Your code is incomplete, you are iterating a movie list (array of elements) passing what is presumably a list of movie titles/descriptions to each, yet there is no reference to items in render(wrapper, items);? Nor does the code make semantic sense, it reads as creating a movie list of movie lists?? You will have to clarify and fix the code to get a good review.

Answer 1

As pointed out by Blindman67: in render(wrapper, items);, items is not defined.

1. const url = 'http://moviedatas.com/movies';
   

   Const variable used only one time, you can remove it and write url: http://moviedatas.com/movies

   Same for

   const wrapper = $(this);
   

2. Use ES6 arrow function shortcut syntax () =>

    success: function(response) {
    movieList.each(function() {
    const wrapper = $(this);
    render(wrapper, items);
    });
    },
   

   7 lines in one, shorter is cleaner.

   success: (response) => movieList.each(() => render($(this), response.items)),
   

3. error: function(error) {
    onError(movieList);
   }
   

   If you don't use error XHR response, just write

   error: () => onError(movieList)
   

4. Write jsDoc docstring above functions to help developers and IDE understanding what is happening.

5. If functions are becoming inline, you may write them directly in caller function.

Full review (without docstring)

function loadMovies() {
 const movieList = $('.movie_list');
 // setOnLoading
 movieList.each(() => $(this).html('Loading...'));
 $.ajax({
 method: 'GET',
 url : 'http://moviedatas.com/movies',
 data : { count: 10 },
 success: (response) => movieList.each(() => render($(this), response.items))
 error : () => movieList.each(() => $(this).html(''));
 });
}
function render(wrapper, items) {
 const template = `<div class="movie_item">
 <div class="title">{title}</div>
 <div class="desc">{description}</div>
 </div>`;
 let movies = '';
 for (let i = 0; i < items.length; i++) {
 movies += template
 .replace('{title}', items[i].title)
 .replace('{description}', items[i].description);
 }
 wrapper.html(movies);
}

Also, I don't understand the logic of getting a movie list data and then iterating over a movie list HTML to put movies in. It becomes a list of list of movies...

Answer 2

The code already uses some ecmascript-6 features like the const and let keywords and template literals.

Another ES-6 feature that could be used is the for...of loop to simplify blocks like this:

for (let i = 0; i < items.length; i++) {
 movies += template
 .replace('{title}', items[i].title)
 .replace('{description}', items[i].description);
}

To this:

for (const item of items) {
 movies += template
 .replace('{title}', item.title)
 .replace('{description}', item.description);
}

---

The error handler could be simplified using a partially applied function, to avoid an extra function call - i.e.

error: function(error) {
 onError(movieList);
}

Can be simplified to:

error: onError.bind(null, movieList)