2
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ Welcome to Code Review. Currently, your question has no description on what the code does. A context is essential to review the code. As-is, we can only guess that the code is to do something. What it does? I don't know. Also, you seem to have missing code. You don't define the function generateRandom anywhere and your variable cards isn't defined either. Also, you don't show how you will call the function drawHumanCard(). Those are essential things for a complete review. Also, your new title isn't that explicit, since it doesn't describe the purpose of the code. \$\endgroup\$ Jul 29, 2015 at 9:03
  • \$\begingroup\$ So, your code is generating a random number between 0 and 0? According to your description on generateRandom(0), that's exactly what it is doing. Right? And what data exactly is in the cards array? You don't need to show the whole array. Just 3-4 elements is enough. Also // ... and so on tells me that there's still missing code there. If you need to ask how you can rewrite this, without bloating the comment section, feel free to go on The 2nd Monitor chatroom \$\endgroup\$ Jul 29, 2015 at 9:15
  • \$\begingroup\$ @IsmaelMiguel - Thanks for the feedback. My intention was to keep things simple and not overload the question with too much code. I didn't want to include the entire chain of functions. \$\endgroup\$
    – Lodder
    Jul 29, 2015 at 9:22
  • \$\begingroup\$ Now that's much better. You're still missing the definition of the generateRandom(). also, you should have your whole code in a single block. \$\endgroup\$ Jul 29, 2015 at 9:39
  • \$\begingroup\$ @IsmaelMiguel - Updated. \$\endgroup\$
    – Lodder
    Jul 29, 2015 at 9:56

2 Answers 2

2
\$\begingroup\$

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.

\$\endgroup\$
7
  • \$\begingroup\$ Massive thank you for the time you've given to help. Highly appreciated \$\endgroup\$
    – Lodder
    Jul 29, 2015 at 11:10
  • \$\begingroup\$ @Lodder You're welcome. I hope this is really useful for you. As a piece of advice, I would wait a little before accepting an answer. While it is really great that you accepted my answer, someone else can come up with a way better way to answer it. I always wait some time before accepting the answer, and testing to make sure it works at 100%. \$\endgroup\$ Jul 29, 2015 at 11:36
  • 1
    \$\begingroup\$ If you are trying to remove jQuery completely then you should remove $(document).ready(); as well. That can be replaced with document.addEventListener('DOMContentLoaded', function() {}, false); replaces the empty function with your code from document.ready. \$\endgroup\$ Jul 29, 2015 at 14:44
  • \$\begingroup\$ @GaryStorey Quoting myself: "This can and should be written as vanilla Javascript [...] but that is an exercise for the O.P. to try.". That's the only reason why I'm using jQuery there. But yes, it is a valid point, but not all browsers have that event, like older IE versions. jQuery wins here because it takes care of browsers' differences. I'm not saying I don't disagree, I'm saying that the focus on the question is on jQuery. And I answered with jQuery. \$\endgroup\$ Jul 29, 2015 at 14:57
  • 1
    \$\begingroup\$ The comment was more for @Lodder than for yourself. Just pointing out a way to get started w/o jQuery if that was the way he wanted to go. \$\endgroup\$ Jul 29, 2015 at 15:37
2
\$\begingroup\$

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.

\$\endgroup\$
7
  • \$\begingroup\$ One thing I disagree with you is how the function is built. jQuery is defined on the window object, but you are passing it as the first parameter. I usually recomment something similar, but with way stricter scope (basically, I ensure that the window object is the real window object). I didn't add that because the O.P. is learning and that is a little overload and because this will be in a page with known script content. Other than that, +1! \$\endgroup\$ Jul 29, 2015 at 15:59
  • \$\begingroup\$ The above code is a wrapper around all of the other code. It ensures that $ always refers to the jQuery object. It also passes the window and document objects so they are locally scoped for (slightly) faster performance. undefined is passed for those pre-ES5 browsers that actually let you re-define its value. You said basically, I ensure that the window object is the real window object. The window object can not be re-defined that I know of. Can you point me to a code sample where it can be redefined for my reference? \$\endgroup\$ Jul 29, 2015 at 21:26
  • \$\begingroup\$ It can't be defined in the global scope, but it can be defined in any other scope. On those scopes, it will have whatever the heck you wish. What I do generally is (function(window, undefined){})(Function('return this')());, which solves that problem even if you have your function in another scope. Such scope may be a lazy loading script that used the Function constructor to run the code (fetched over XHR) in the same scope, instead of eval(). \$\endgroup\$ Jul 29, 2015 at 21:58
  • \$\begingroup\$ OK... That is interesting. Do you have any links explaining this technique in detail? Thanks! \$\endgroup\$ Jul 29, 2015 at 22:02
  • \$\begingroup\$ Sadly, there's nothing documented. I partially made it up. The idea was taken from JSFuck. From one of the examples. Any constructor can be used, but if you ruin the Function constructor, you ruin everything. So, no one sane enough will touch it. JSFuck uses []["filter"]["constructor"]("return this")() but that is too clunky and huge. Function('return this')() does just perfectly. You could use (function(){return this})() and achieve the same effect. But I personally prefer the Function way. \$\endgroup\$ Jul 29, 2015 at 22:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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