HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Interactive small town

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
towninteractivesmall

Problem

I've built this simple interactive scenario. Now everything seems to be working fine except for the start of the for loop as below. What can I do to make the script execution faster?

Here is the full code: http://codepen.io/Angussimons/pen/oZmNer

```
for (let i = 0; i < manNumber; i++) {

// Definisco quale strada
if (i < (manNumber * leftStreet)) {
street[i] = document.getElementById('path-2');

} else {
street[i] = document.getElementById('path-1');
}

// genero una velocità per ognuno
_speed[i] = Math.floor((Math.random() * 400) + 1);
_speed[i] = (_speed[i]/1000);

let shiftTop = -1.5;

// genero una posizione su ognuno
let manPosition = shiftTop + Math.floor((Math.random() * 1000) - 500)/1000;

// vario la durata dei gesti
var _duration = Math.floor((Math.random() * 1000) + 500);

mansHead.push( new mojs.Shape({
parent: '.omini',
className: 'omino man-head-'+i,
shape: 'circle',
radius: 8,
fill: manColor,
top: manPosition+'%',
left: 0,
x: {0 : 5},
y: 10,
easing: 'linear.none',
duration: _duration,
}).then({
x: {5 : 0},
}));
mansBody.push(new mojs.Shape({
parent: '.man-head-'+i,
className: 'man-body-'+i,
shape: 'line',
fill: 'none',
stroke: manColor,
radius: 10,
strokeWidth: 13,
strokeLinecap: 'round',
angle: {85 : 95},
x: {5 : -2},
y: 30,
easing: 'linear.none',
duration: _duration,
}).then({
angle: {95 : 85},
x: {[-2] : 5},
y: 30,
}));
mansArmL.push(new mojs.Shape({
parent: '.man-body-'+i,
className: 'man-arm-'+i,
shape: 'line',
fill: 'none',
s

Solution

JavaScript
The loop that generates people - an expensive bridge to cross

”...DOM access is actually pretty costly - I think of it like if I have a bridge - like two pieces of land with a toll bridge, and the JavaScript engine is on one side, and the DOM is on the other, and every time I want to access the DOM from the JavaScript engine, I have to pay that toll”

    - John Hrvatin, Microsoft, MIX09, in this talk Building High Performance Web Applications and Sites at 29:38, also cited in the O'Reilly Javascript book by Nicholas C Zakas Pg 36, as well as mentioned in this post

Bearing in mind that was stated more than 10 years ago and browsers likely have come along way since then, it is still wise to be aware of this.

This code block makes quite a few redundant DOM look-ups - 30 (based on the value I see assigned to manNumber) to be exact:

for (let i = 0; i < manNumber; i++) {


if (i < (manNumber * leftStreet)) {
    street[i] = document.getElementById('path-2');
  } else {
    street[i] = document.getElementById('path-1');
  }


It would be more efficient to look up those two elements outside the loop - preferably once the DOM is ready...

const path1 = document.getElementById('path-1');
const path2 = document.getElementById('path-2');


The inside that loop, use those references:

for (let i = 0; i < manNumber; i++) {
  street[i] = i < (manNumber * leftStreet) ? path2 : path1;
}


Selecting elements to get innerHTML

The function fillElement() uses querySelector to get elements by the id attribute:

function fillElement (ele) {


// Inner
  modalInner.innerHTML = document.querySelector('#js-modal-template-'+ele).innerHTML;
}


It isn't wrong to use querySelector to get elements by id but using getElementById() "is definitely faster" 1 (see this jsPerf test for comparison).
Redundant click handler code

$('.mic-g').on('click', function (e) {


fillElement('cineteca');
  modal.replay();
  modalOpenTimeline.replay();
});


// Belgrado / Kinoteka


$('.belgrado-g').on('click', function(e) {
  fillElement('kinoteka');
  modal.replay();
  modalOpenTimeline.replay();
});
//And on and on for more selectors...


It would be better to observe clicks on the document and look for the class names on the target element. Those class names could be put into a mapping of class names to values to be passed to fillElement(). Something like below (untested):

const classModalMapping = {
  'mic-g': 'cineteca',
  'belgrado-g': 'kinoteka',
  //...etc. 
}
$(document).click(function(e) {
  $(e).target.classList.forEach(function(className) {
    if (className in classModalMapping) {
      fillElement(classModalMapping[className]);
      modal.replay();
      modalOpenTimeline.replay(); 
    }
  });
});


CSS

The rulesets for .modal-text-title and .modal-text-inner both have padding rules where the left and right values are identical. Those can be condensed to a single value.

padding: 120px 5% 20px 5%;


Can be condensed to:

padding: 120px 5% 20px;


And

padding: 0 5% 20px 5%;


Can be condensed to:

padding: 0 5% 20px;


1https://www.sitepoint.com/community/t/getelementbyid-vs-queryselector/280663/2

Code Snippets

for (let i = 0; i < manNumber; i++) {
if (i < (manNumber * leftStreet)) {
    street[i] = document.getElementById('path-2');
  } else {
    street[i] = document.getElementById('path-1');
  }
const path1 = document.getElementById('path-1');
const path2 = document.getElementById('path-2');
for (let i = 0; i < manNumber; i++) {
  street[i] = i < (manNumber * leftStreet) ? path2 : path1;
}
function fillElement (ele) {

Context

StackExchange Code Review Q#159800, answer score: 6

Revisions (0)

No revisions yet.