Feedback for refactoring javascript

Feedback for refactoring javascript
0

#1

Hello. I created a small js project, which fetches data from opentdb.com’s api. I used vanilla js and jQuery to create this. I am not looking for web design/css advice.
I am trying to improve my javascript skills by using the DRY (don’t repeat yourself) principle. What would be one way I could refactor the code and abstract out some of the repeating code.
Would appreciate any advice.:grinning:


#2

Well, let’s look at where you have repetition.

var sports_api = 'https://opentdb.com/api.php?amount=10&category=21&difficulty=medium&type=multiple';
          var music_api = 'https://opentdb.com/api.php?amount=10&category=12&difficulty=medium&type=multiple';
          var movies_api = 'https://opentdb.com/api.php?amount=10&category=15&difficulty=medium&type=multiple';

It looks like these are all the same string except that the number following category=. If the API were to change, you would have to fiddle with each of these strings. How can you change your code to simply insert the correct category id into a common string?


                    let correct = gameItem.correct_answer;
                    let incorrect1 = gameItem.incorrect_answers[0];
                    let incorrect2 = gameItem.incorrect_answers[1];
                    let incorrect3 = gameItem.incorrect_answers[2];
                
                    let newArray = [];
                    newArray.push(correct, incorrect1, incorrect2, incorrect3);

All 4 of these variables that you declare are immediately put in an array and never used again. Why do they need to be declared at all? Further, gameItem.incorrect_answsers is an array. Instead of hard coding the indices to include in your newArray, how can you just take 3 consecutive values from an array at once?


                    <input type='radio' name='sports' value='a'> A. ${newArray[0]}<br>
                    <input type='radio' name='sports' value='b'> B. ${newArray[1]}<br>
                    <input type='radio' name='sports' value='c'> C. ${newArray[2]}<br>
                    <input type='radio' name='sports' value='d'> D. ${newArray[3]}<br>

Could you generate this instead of hardcoding it?


 $('#sportsbtn').click(function(){
                    $.ajax({
                        type: 'GET',
                        url: sports_api
                    }).done(function(data){
                        
                        $('.Game').empty();
                        makeGame(data.results);
                    });
                });

                $('#musicbtn').click(function(){
                    $.ajax({
                        type: 'GET',
                        url: music_api
                    }).done(function(data){
                        
                        $('.Game').empty();
                        makeGame(data.results);
                        
                    })
                })

                $('#videobtn').click(function(){
                    $.ajax({
                        type: 'GET',
                        url: movies_api
                    }).done(function(data){
                        
                        $('.Game').empty();
                        makeGame(data.results);
                        
                    })
                })

These click functions are identical except for the url value (which we’ve already noticed differ only by a category id). Why not write one function that takes a parameter?


#3

Thank you for the feedback @ArielLeslie.

In response to “All 4 of these variables that you declare are immediately put in an array and never used again. Why do they need to be declared at all?

The logic I used was -

  1. Create an array that includes each incorrect answer plus the correct answer, and then randomly sort those items so that I don’t always have answer Choice A as being the Correct Answer (for example).
  2. Then I use newArray[0], newArray[1], newArray[2], newArray[3] as the text inside the radio button input tags. I would have looped through the array as you suggested if I did not want to use a random sort.

Could you elaborate on your question “Could you generate this instead of hardcoding it?” Do you mean use a JS or jQuery function to output the 4 inputs?

I am working on writing a function that includes the URL as a parameter, to see if I can clean up that part of my code. Thank you for the feedback and hope we can continue discussing, as it helps me learn and improve!

For the quesList.innerHTML variable that contains the form, would it be a good idea to use a template tool such as Mustache (or Handlebars, Pug)? or would that create a bigger mess?


#4

I understand why you’re creating newArray, but you didn’t need to declare variables just to put their values in newArray.

Those html tags are just part of a string. How could you use JavaScript to dynamically build that string to contain the correct number of nearly identical substrings?


#5

Adding to @ArielLeslie’s last reply, what she means is you are already dynamically putting the answer with arr[0[, arr[1], arr[2], and arr[3] below, but the only other difference between these 4 lines are the letters a, b, c, d (in upper and lower case). See if you can dynamically create each line instead of manually writing those four lines. There are a couple of ways you could do it.

<input type='radio' name='sports' value='a'> A. ${newArray[0]}<br>
<input type='radio' name='sports' value='b'> B. ${newArray[1]}<br>
<input type='radio' name='sports' value='c'> C. ${newArray[2]}<br>
<input type='radio' name='sports' value='d'> D. ${newArray[3]}<br>

#6

Thank you so much for this. I was into this issue and tired to tinker around to check if its possible but couldnt get it done. Now that i have seen the way you did it, thanks guys
with
regards


#7

FYI, to create a random shuffle it is generally accepted that the Fisher-Yates Shuffle algorithm is better than what you are using.