Emptying and populating an element in jQuery

Question

I have written the following function to empty and then populate multiple elements using jQuery:

function drawHumanCard()
{
    var random = generateRandom();

    var humanEl = $('#human');

    humanEl.find('#human-name').empty().text(cards[random]['name']);
    humanEl.find('#human-job').empty().text(cards[random]['job']);
    humanEl.find('#human-image').attr('src', 'img\/trumps\/' + cards[random]['image']);
}

var cards = [
    new TopTrumpCard('Name 1', 'designer', 'image1.jpg'),
    new TopTrumpCard('Name 2', 'designer', 'image2.jpg'),
    new TopTrumpCard('Name 3', 'designer', 'image3.jpg')
];

function TopTrumpCard(name, job, image) 
{
    this.name       = name;
    this.job        = job;
    this.image      = image;
}

function generateRandom()
{
    var objCount = Object.keys(cards).length - 1;
    var number   = Math.floor(Math.random() * (objCount - 0 + 1)) + 0;

    return number;
}

What the drawHumanCard function does:

var random is a randomly generated number between a specific range. The range is 0 and number of cards array keys.

cards is a variable that contains an array of data. The random variable is used to target a specific key in the cards array.

From then on, I've defined 3 elements. 2 of which require the text to be changed and 1 that requires the image src attribute value to be changed.

---

What would be the best way to write this in a cleaner way?

I was thinking about defining the jQuery element objects in an array and then running an each loop.

As you can see, I am using jQuery for this, but am happy to go with vanilla JS too.

Answer 1

Your code isn't bad, but there are some aspects that tick me a bit.

1. Lets start with the generateRandom()

   You currently have the following:

   var objCount = Object.keys(cards).length - 1;
   var number   = Math.floor(Math.random() * (objCount - 0 + 1)) + 0;
   
   return number;
   

   And I ask:

   • Why + 0 or - 0?
   • Why Object.keys(cards).length - 1; for an array?
   • Why take 1 to sum it again?
   • Why that bloat?

   Here's how I would write it:

   function generateRandom()
   {
       return Math.floor(Math.random() * cards.length);
   }
   

   Since the length is 1-based and the indexes are 0-based and Math.random() returns numbers between 0 and 1 (excluding the 1), you don't need more than this.

2. And now, the drawHumanCard()

   You have the following code:

   var random = generateRandom();
   
   var humanEl = $('#human');
   
   humanEl.find('#human-name').empty().text(cards[random]['name']);
   humanEl.find('#human-job').empty().text(cards[random]['job']);
   humanEl.find('#human-image').attr('src', 'img\/trumps\/' + cards[random]['image']);
   

   You can store cards[random] inside a variable, like this:

   var card = cards[random];
   

   And to improve performance, you don't need the .empty(). It's a waste of time and CPU power.

   Also, why humanEl.find('#human-name') instead of $('#human-name')? And why not an array with these objects?

   I would rewrite the whole thing as this:

   var human;
   
   //it must run after the elements exist in the DOM
   $(document).ready(function(){
       human = {
           name: $('#human-name'),
           job: $('#human-job'),
           image: $('#human-image')
       };
   });
   
   function drawHumanCard()
   {
       var random = generateRandom();
   
       var card = cards[random];
   
       human.name.text(card['name']);
       human.job.text(card['job']);
       human.image.attr('src', 'img/trumps/' + card['image']);
   }
   

So, the final code would be like this:

var human;

$(document).ready(function(){
    human = {
        name: $('#human-name'),
        job: $('#human-job'),
        image: $('#human-image')
    };
});

var cards = [
    new TopTrumpCard('Name 1', 'designer', 'image1.jpg'),
    new TopTrumpCard('Name 2', 'designer', 'image2.jpg'),
    new TopTrumpCard('Name 3', 'designer', 'image3.jpg')
];

function TopTrumpCard(name, job, image) 
{
    this.name = name;
    this.job = job;
    this.image = image;
}

function drawHumanCard()
{
    var random = generateRandom();

    var card = cards[random];

    human.name.text(card['name']);
    human.job.text(card['job']);
    human.image.attr('src', 'img/trumps/' + card['image']);
}

function generateRandom()
{
    return Math.floor(Math.random() * cards.length);
}

This can and should be written as vanilla Javascript. The use of jQuery isn't justifiable here but that is an exercise for the O.P. to try.

Answer 2

Just adding my $0.02 even though there is an accepted answer.

You should wrap your code in an IIFE to help prevent code collision. By using an IIFE, you create a private scope for you to work in. That way you don't pollute the global scope. If you are using jQuery, you can also use this to force the $ to be the jQuery object. The first line in this should be use strict to put you in JS strict mode.

(function( $, window, document, undefined ){
  'use strict';
  // your code here
})( jQuery, window, document );

I also recommend using the One True Brace Style (1TBS) or K&R style braces. Using Allman style braces can lead to issues in JS if you are not careful. If you understand the ramifications, and that's your preferred style, then that's fine. You can read more about it here (why to use K&R style in JS) and here (differences btwn styles).

Other than that, I would refer to @ismaelmiguel 's answer.