4
\$\begingroup\$

I am creating a web page in which I have 4 different numbers which show companies stats. I am using javascript to dynamically show these numbers from 0 to n (where 0 is the starting point of counter and n is the end, I have n=74, n=86, n=285, n =97). I am using javascript to start the counter from 0 to n, everything is working fine, as I am just a beginner in programming so I just want to know how experienced programmers think of this code. is this a good approach for this problem? Can the algorithm efficiency be improved?

//I am invoking this function on scroll but here I have provided the button

function letsCount(){
  var num = document.getElementsByClassName('number');
  var count = 0;
  var id = setInterval(myFunction,1);
  var val = document.getElementById('value');
  function myFunction(){
    if(count == 286 ){ //stop the counter at this value
      clearInterval(id);
    }
    else if(count < 76) { //First (Smallest) value of n
      num[0].innerHTML = count;
      num[1].innerHTML = count;
      num[2].innerHTML = count;
      num[3].innerHTML = count;
      count++;
    }
    else if(count < 87){ // second value of n
      num[1].innerHTML = count;
      num[2].innerHTML = count;
      num[3].innerHTML = count;
      count++;
    }
    else if(count < 98){ // third value of n
      num[2].innerHTML = count;
      num[3].innerHTML = count;
      count++;
    }
    else if(count < 286){ // fourth value of n
      num[2].innerHTML = count;
      count++;
    }
  }
}
button {
   position: absolute;
   top: 30px;
   left: auto;
   right: 30px;
   border: none;
   background-color: black;
   color: white;
   padding: 10px;
   cursor: pointer;
  }
button:hover {
   background-color: crimson;
}
 <p class = "number">
   0
 </p>
 <h3>Styles Created</h3>
 <p class = "number">
   0
 </p>
 <h3>Awards Won</h3>
 <p class = "number">
   0
 </p>
 <h3>Stores Worldwide</h3>
 <p class = "number">
   0
 </p>
 <h3>Varities Desinged</h3>
 <button onclick = "letsCount()">START</button>

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to code review. Impressive for a first start. To get more attention to your post? you can start off by changing the title of your question so as to reflect what your code does. \$\endgroup\$ – Siobhan Nov 16 '16 at 14:21
3
\$\begingroup\$

Keep the global scope clean and data/code segregation

First I will say your code is good for a beginner but you are missing some concepts and techniques that will help you improve your code and improve the client experience.

Some important concepts

  • Use requestAnimationFrame for animations, and know the difference between animation and one off content changes.
  • Data belongs in the page. Be it the text, images, or the instructions/values for the javascript, the place for data is on the page.
  • Keep the global namespace clean. With a little effort you can completely avoid adding any variables to the window namespace / global scope.
  • Don't put code in the HTML It is tempting to put code in the page to start an animation <button onclick="startAnim()" value="start">. All good when it's a simple page but a source of headache, bugs, and unhappy clients when its get complicated
  • When parts of your code starts to look repetitive it's a good sign that there is a better way.
  • Remove event handlers when you are done. Event handlers use up resources.
  • Always keep in mind your page shares the computer with dozens of others on the client's machine. Do your best to reduce the total load on their device.

The javascript

Always start your code with "use strict" it helps you avoid some very common bugs and bad coding styles and runs your code significantly faster.

"use strict"; // is valid as the first line of a script tag or the first
              // line inside a function. Put anywhere else and it has no effect
window.addEventListener("load",function(){  // starting your code on the load 
                                           // event ensures that all the DOM 
                                           // tree is populated and ready

try to define your event handlers as named functions because you need a function referance to remove the event if you need to.

    function startButtonClick(){  // the start button event

We only want this to play once. So remove the event from the button. When events are called this references the element that triggered the event

        this.removeEventListener("click",startButtonClick); // remove event

The spread operator is a short cut way of turning a array like list into an array. Be wary as IE 11 is still popular and does not support many ES6 features. If you use things like the spread operator you should consider using babel.js so that your code can run on legacy browsers.

        var numberEl = [...document.getElementsByClassName('number')];   

The numbers moving up is an animation. It will be control by requestAnimationFrame that will call the animate function at 60fps if possible, but that is not a guarantee. To ensure you get consistent animation on all devices use the supplied time the requestAnimationFrame callback supplies to time your animation.

You were using a interval of 1ms.The rule of thumb is 60fps or 16ms between display frames, updating content any higher than that rate will simply not be seen and thus will just waste resources.

        var startTime;  // holds that animation start time
        function animateNumbers(time){ // time is the first argument of the 
                                       // requestAnimationFrame callback

This countSpeed controls how fast the numbers will increase per second. The animation will play at the same speed on all devices.

            const countSpeed = 120; // count speed in increments per second
            if(startTime === undefined){// if there is no start time then this must 
                                        // be first frame so make it the start time
                startTime = time;
            }
            var seconds = (time - startTime) / 1000; //seconds from animation start
            var count = countSpeed * seconds; // get the current count
            done = true; // flag to indicate the animation is complete
            numberEl.forEach(element => {  
                var max = Number(element.dataset.maxValue); // properties that you 
                                                // get from the  page are strings. 
                                                // If you know it's a  number let 
                                                // Javascript know

If the count is below max for any element flag the animation as not done.

                if(count < max){
                    done = false;
                }

Try to avoid setting text content with innerHTML as it can cause all sorts of problems that are avoided by telling the DOM it is only text you are adding

                element.textContent = Math.min(max,Math.floor(count));
            }
            if(!done){  // continue animation until done
                requestAnimationFrame(animateNumbers); 
            }
        }            

When ever you are animating anything on the page were you define every frame use requestAnimationFrame it ensures that the animation is in sync with the hardware and any other page rendering. It also prevents updated content from being presented to early or quicker than can actualy be seen. Smooth animation is not about speed, but about being in sync and limiting the amount of work the computer must do.

        requestAnimationFrame(animateNumbers); 
    }

Get the button element and add the event listener

    document.querySelector("#startButton").addEventListener("click", startButtonClick);
})

The HTML, data lives here

Placing the values for the counters inside the javascript code makes it hard to manage. The javascript is a constant, the page contains the data and is unique per view/client

The formal way to add data/attributes to elements is via the data- prefix though this is not enforced and any named value pair you add to an element can be accessed, data- ensures your data is safe from conflict or misinterpreted by the DOM.

data-max-value is not a valid javascript variable name so you can not easily acces the data using that name you need to use the javascript version created by the DOM. In HTML5 the DOM creates an attribute called dataset that you use to access the variable with. Replace data- with dataset. (that's a dot not a full stop) then the max, then replacing any subsequent '-' by removing the - and capitalising the first letter following, thus data-max-value becomes dataset.maxValue see the code above as an example.

 <p class = "number" data-max-value ="87">
   0
 </p>
 <h3>Styles Created</h3>
 <p class = "number" data-max-value = "286">
   0
 </p>
 <h3>Awards Won</h3>
 <p class = "number" data-max-value = "96">
   0
 </p>
 <h3>Stores Worldwide</h3>
 <p class = "number" data-max-value = "76">
   0
 </p>
 <h3>Varities Desinged</h3>

Avoid placing javascrpt inside the HTML content. It makes it much harder to manage your code. Use an unquie Id to identify elements for direct or queried access from within your javascript.

<button id = "startButton" >START</button>

Put it all together

"use strict";
window.addEventListener("load",function(){
    function startButtonClick(){
        this.removeEventListener("click",startButtonClick); // remove event
        var numberEl = [...document.getElementsByClassName('number')];      
        var startTime; 
        function animateNumbers(time){ 
            const countSpeed = 120; 
            if(startTime === undefined){ 
                startTime = time;
            }
            var seconds = (time - startTime) / 1000; // get the seconds since the animation start
            var count = countSpeed * seconds; // get the current count
            var done = true; 
            numberEl.forEach(element => {  
                var max = Number(element.dataset.maxValue);
                if(count < max) {
                    done = false;
                }
                element.textContent = Math.min(max,Math.floor(count));
            });
            if(!done){  
                requestAnimationFrame(animateNumbers); 
            }
        }            
        requestAnimationFrame(animateNumbers); 
    }
    document.querySelector("#startButton").addEventListener("click", startButtonClick);
});
button {
   position: absolute;
   top: 30px;
   left: auto;
   right: 30px;
   border: none;
   background-color: black;
   color: white;
   padding: 10px;
   cursor: pointer;
}
button:hover {
   background-color: crimson;
}
 <p class = "number" data-max-value ="87">
   0
 </p>
 <h3>Styles Created</h3>
 <p class = "number" data-max-value = "286">
   0
 </p>
 <h3>Awards Won</h3>
 <p class = "number" data-max-value = "96">
   0
 </p>
 <h3>Stores Worldwide</h3>
 <p class = "number" data-max-value = "76">
   0
 </p>
 <h3>Varities Desinged</h3>
<button id = "startButton" >START</button>

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks for reviewing my code, your explanations are of great help to me. I need someone like you to teach me. You are awesome. \$\endgroup\$ – Amir Saleem Nov 17 '16 at 6:58
1
\$\begingroup\$

Here’s what I would do and why.

  1. Use setTimeout instead of setInterval as that removes the risk of myFunction being ran twice in a row, without a pause. In your implementation myFunction might not be ready until setInterval calls it again. By letting myFunction re-invoke itself when its ready we remove this possibility.
  2. Reduce the number if statements (which reduces the number of invariants and therefore the risk of bugs) by introducing a hash map with all count limits stored in map. Now we’re only checking that num[2] is being incremented from 0 to 286 once. Before it was 4 times.
  3. Use const on non-mutable variables instead of var.

Here's a working jsfiddle. Hope that helps.

function letsCount() {
  const num = document.getElementsByClassName('number');
  const timeout = 1;
  const map = {
    "0": 76,
    "1": 87,
    "2": 286,
    "3": 98
  };

  function run(count = 0) {
    let isDone = true;
    Object.keys(map).forEach(function(index) {
      if (count < map[index]) {
        num[index].innerHTML = count;
        isDone = false;
      }
    });
    if (!isDone) {
      setTimeout(() => run(count + 1), timeout);
    }
  }

  run();
}

letsCount();
|improve this answer|||||
\$\endgroup\$
1
\$\begingroup\$

I would consider making each counter self-contained, as there is really no apparent reason to have to couple these counters together like this.

This might be something that you implement as a class in your application.

Class file:

function numberCounter(el, counterConfig) {
    this.el = null;
    this.count = null;
    this.startCount = null;
    this.endCount = null;
    this.increment = null;
    this.timeInterval = null;
    this.done = false;
    this.doneCallback = null;

    function init(el, counterConfig) {
        // not shown - el as valid Element,
        // perhaps validate values in counterConfig

        this.el = el;
        for(key in counterConfig) {
           this[key] = counterConfig[key];
        }
    }

    init();
}

numberCounter.prototype.incrementCounter = function() {
    this.count += this.increment;
    this.el.innerHtml = this.count;
    if(this.count + this.increment > this.maxCount) {
        this.done = true;
        this.doneCallback();
    } else {
        setTimeout(this.incrementCounter, this.timeInterval);
    }
}

Usage in page could be:

var counterElements = document.getElementsByClassName('number');
var counterConfig = [
    {
        start: 0,
        end: 76,
        increment: 1,
        timeInterval: 1000,
        doneCallback: function () {}
    },
    {
        start: 0,
        end: 87,
        ...
    },
    ...
}
var counters = [];
counterElements.forEach( (el, index) => {
    var counter = new numberCounter(el, counterConfig[index]);
    counters.push(counter);    
);

// then later when an event causes you to want to start the counters
counters.forEach( counter => counter.incrementCounter() );

You have have a flexible template for implementing counters like this with any sort of start/end/increment values you might need, while also even giving you the ability to attach a callback for when the count sequence completes.

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ @AmirSaleem I didn't introduce the concept of data attributes here as was done in answer provided by @Blindman67 as I didn't want to introduce too many new concepts at once, but I woulds agree with the thought that you should think about storing data on the elements themselves. That could for example, resolving the potential problem in the example code I have shown of having different HTML elements that don't match with your counterConfig array. If you read some or all of that configuration data from the element, you can make the code less fragile. \$\endgroup\$ – Mike Brant Nov 17 '16 at 22:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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