7
\$\begingroup\$

I am looking for someone to help me improve this code. It works great but can sometimes slow my browser down a lot when raindrops are at a high amount.

<section class="rain"></section>

<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/2.1.3/jquery.min.js"></script>
<style>
  html {
    height: 100%;
  }

  body {
    background: #0D343A;
    background: -webkit-gradient(linear, 0% 0%, 0% 100%, from(rgba(13, 52, 58, 1)), to(#000000));
    background: -moz-linear-gradient(top, rgba(13, 52, 58, 1) 0%, rgba(0, 0, 0, 1) 100%);
    overflow: hidden;
  }

  .drop {
    background: -webkit-gradient(linear, 0% 0%, 0% 100%, from(rgba(13, 52, 58, 1)), to(rgba(255, 255, 255, 0.6)));
    background: -moz-linear-gradient(top, rgba(13, 52, 58, 1) 0%, rgba(255, 255, 255, .6) 100%);
    width: 1px;
    height: 89px;
    position: absolute;
    bottom: 200px;
    -webkit-animation: fall .63s linear infinite;
    -moz-animation: fall .63s linear infinite;
  }
  /* animate the drops*/

  @-webkit-keyframes fall {
    to {
      margin-top: 900px;
    }
  }

  @-moz-keyframes fall {
    to {
      margin-top: 900px;
    }
  }
</style>
<script>
  // number of drops created.
  var nbDrop = 1000;

  // function to generate a random number range.
  function randRange(minNum, maxNum) {
    return (Math.floor(Math.random() * (maxNum - minNum + 1)) + minNum);
  }

  // function to generate drops
  function createRain() {

    for (i = 1; i < nbDrop; i++) {
      var dropLeft = randRange(0, 1600);
      var dropTop = randRange(-1000, 1400);

      $('.rain').append('<div class="drop" id="drop' + i + '"></div>');
      $('#drop' + i).css('left', dropLeft);
      $('#drop' + i).css('top', dropTop);
    }

  }
  // Make it rain
  createRain();
</script>
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Here's Demo of the OP's code. \$\endgroup\$
    – Tushar
    Commented Mar 12, 2017 at 13:19

1 Answer 1

6
\$\begingroup\$

Naming

If you need comment to explain a variable then the variable is not named correctly.

Variable nbDrop and functions randRange and createRain are accompanied with comments explaining their purpose. This is indication of non-descriptive names. Instead of nbDrop, numberOfDrops is self-explanatory name and does not require comment explaining what the variable contains. Similarly, randRange can be renamed to getRandomNumberInRange. Although, the name is looking long, it's descriptive.

Unnecessary Comments

Now, after changing the variables/functions name as shown in above section comment associated with them can be removed.

Below comments are unnecessary:

// Make it rain
createRain();

createRain is pretty expressive. Anyone reading code can infer by reading the function name that it must be creating rain. So, the comment is not required.

Or, in other words, you may rename the function to makeRain and then remove comment. Which one to choose depends on your taste.

Globals

nbDrop, randRange, createRain are globals. While it doesn't affect when working on small projects, it'll interfere with other developers code when working with large projects in big team due to variable names collision. To solve this the code can be wrapped in IIFE. See What is the (function() { } )() construct in JavaScript?

Implicit Global

Variable i used in for is implicit global. It is not declared(no var, let, const) but used. See Declaring variables without var keyword

Better use of jQuery

    $('#drop' + i).css('left', dropLeft);
    $('#drop' + i).css('top', dropTop);

Using method chaining, above code can be written as

    $('#drop' + i).css('left', dropLeft)
        .css('top', dropTop);

Or better, css() accepts object of styles(key-value pairs). Now, the code will become

$('#drop' + i).css({
    'left': dropLeft,
    'top': dropTop
});

It works great but can sometimes slow my browser down a lot when raindrops are at a high amount.

Main reason behind this could be more number of DOM dives.

Each time $(selector) is used, jQuery has to dive into DOM, get the element and then perform operations on it. In the function createRain inside of for, three times DOM elements are referred($('.rain') and $('#drop' + i) twice). This means, 3000 dives for 1000 rain drops.

The question is Can we reduced these dives? And how much?

Yes. We can reduce the dives by caching the elements. $('.rain') can be moved before the for and cached

var $rain = $('.rain');
for (i = 1; i < nbDrop; i++) {

Here, we've cached the element which can be referenced inside the loop. This will bring the dives to 2001(one for .rain, 2000 for '#drop' + i).

After chaining/using css(properties), number of dives will become 1001.

Can we reduce this further?

Yes. Instead of applying styles after element is added to DOM, we can add styles to element and then throw it in DOM.

    $('.rain').append('<div class="drop" id="drop' + i + '" style="left: ' + dropLeft + 'px; top: ' + dropTop +'px;"></div>');

Did this actually reduced the DOM dives? append is also a dive. But, this code help us realize that ID attribute on each .drop is just used to apply styles on it. If we use this syntax, we can remove ID of element.

Let's use old formula here:

Create a string.    
loop:
    append to string
end:
use string here.

Using this, DOM dives will reduced to one. peace!.

var rainDrops = '';
for (var i = 0; i < numberOfDrops; i++) {
    var dropLeft = randRange(0, 1600);
    var dropTop = randRange(-1000, 1400);

    rainDrops += '<div class="drop" style="left: ' + dropLeft + 'px; top: ' + dropTop + 'px;"></div>';
}

$('.rain').append(rainDrops);

Who needs jQuery?

Looking at above code, I realize jQuery is used only once to append the drops in container. append? The function is called only once, so we're not even actually appending(adding to existing content) them, we're setting(by setting I mean to add them and not append) them.

So, we can remove dependency on jQuery completely and use innerHTML with document.querySelector to select element

document.querySelector('.rain').innerHTML = rainDrops;

It's funny to put this right after above section

Magic Numbers

1600, 1400 what are they?

Instead of using them directly, create a variable(of course with a descriptive name) and then use that variable.

We can infer them as screen width and screen height respectively. But how did you know my screen resolution? Or even, is that my screen resolution?

JavaScript provides a better way to get the width and height of browser. So, 1600 will become window.innerWidth and 1400 will become window.innerHeight.

Configurable number of drops

By passing the numberOfDrops as argument to createRain() will give more control on how many drops to be shown. If nothing is passed, default 1000 can be used.

Enough of talking, show me the code now!

(function() {
  'use strict';

  function getRandomNumberInRange(minimum, maximum) {
    return Math.floor(Math.random() * (maximum - minimum + 1)) + minimum;
  }

  function createRain(numberOfDrops) {
    numberOfDrops = numberOfDrops || 1000;
    var windowWidth = window.innerWidth;
    var windowHeight = window.innerHeight;

    var rainDrops = '';
    for (var i = 0; i < numberOfDrops; i++) {
      var dropLeft = getRandomNumberInRange(0, windowWidth);
      var dropTop = getRandomNumberInRange(-1000, windowHeight);

      rainDrops += '<div class="drop" style="left: ' + dropLeft + 'px; top: ' + dropTop + 'px;"></div>';
    }

    document.querySelector('.rain').innerHTML = rainDrops;
  }

  createRain(250);
}());
html {
  height: 100%;
}

body {
  background: #0D343A;
  background: -webkit-gradient(linear, 0% 0%, 0% 100%, from(rgba(13, 52, 58, 1)), to(#000000));
  background: -moz-linear-gradient(top, rgba(13, 52, 58, 1) 0%, rgba(0, 0, 0, 1) 100%);
  overflow: hidden;
}

.drop {
  background: -webkit-gradient(linear, 0% 0%, 0% 100%, from(rgba(13, 52, 58, 1)), to(rgba(255, 255, 255, 0.6)));
  background: -moz-linear-gradient(top, rgba(13, 52, 58, 1) 0%, rgba(255, 255, 255, .6) 100%);
  width: 1px;
  height: 89px;
  position: absolute;
  bottom: 200px;
  -webkit-animation: fall .63s linear infinite;
  -moz-animation: fall .63s linear infinite;
}


/* animate the drops*/

@-webkit-keyframes fall {
  to {
    margin-top: 900px;
  }
}

@-moz-keyframes fall {
  to {
    margin-top: 900px;
  }
}
<section class="rain"></section>

\$\endgroup\$
0

Your Answer

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

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