2
\$\begingroup\$

I made a game for children. This game tests the ability to count (i.e. compare numbers based on magnitude). First you must select the age of the child in the upper menu, then you need to select blocks with numbers in ascending order.

JSFIDDLE

document.addEventListener("DOMContentLoaded", ready);

let currNum;
let step;
let startContent = 'Это простейший тренажёр для детей, он проверяет способность считать. Предполагается, что: \
<br>дети от 1 до 3 лет должны уметь считать до 5 \
<br>дети от 4 до 5 лет должны уметь считать до 10 \
<br>дети от 6 до 7 лет должны уметь считать до 30 \
<br>дети от 7 до 9 лет должны уметь считать до 100 \
	<br><br>Чтобы проверить знания участнику предлагается последовательно в порядке возрастания выбрать кубики с цифрами \
';

function init() {
	currNum = -1;
	step = 0
	let startBtn = document.getElementById('start');
	startBtn.click();
};

function clearContent() {
	let contentInner = document.getElementById('contentInner');
	contentInner.innerHTML = '';
};

function deletePoint(id) {
	let pointEl = document.getElementById(id);
	pointEl.parentNode.removeChild(pointEl);
};

function createPoints(count) {
	let contentInner = document.getElementById('contentInner');

	for(let i = 0; i <= 2; i++) {
		let num_ = Math.random() * count;
		let num = Math.ceil(num_);

		let pointEl = document.createElement('div');
		pointEl.className = 'point';
		pointEl.id = 'point_' + i;
		pointEl.setAttribute('data-num', num);
		pointEl.innerHTML = num;		

		contentInner.appendChild(pointEl);
	};
};


function ready() {
	let ulEl = document.getElementById('ul');
	let contentInnerEl = document.getElementById('contentInner');

	let menu = Rx.Observable.fromEvent(ulEl, 'click');
	let contentInner = Rx.Observable.fromEvent(contentInnerEl, 'click');

	let subscription = menu
	.map(e => e.target)
	.filter(t => t.className === 'start' || t.className === 'li')
	.subscribe(function (t) {
		clearContent();
    switch (t.className) {
      case 'start':
				document.getElementById('contentInner').innerHTML = startContent;
        break;
      case 'li':				
				createPoints(t.getAttribute('data-count'));
        break;
    }
	});

	let subscription3 = contentInner
	.map(e => e.target)
	.filter(t => t.className === 'point')
	.subscribe(function (t) {
		let val = +document.getElementById(t.id).innerHTML;

		++step;
		deletePoint(t.id);

		if(currNum > val) { 
			alert('Not correct'); 
			init();
		} else {
			currNum = val;
			if(step > 2) { 
				alert('Correct'); 
				init();
			}
		}	
	});									
	
	init();
};
<script src="https://unpkg.com/rxjs@5.5.2/bundles/Rx.min.js"></script>
<ul class="ul" id="ul">
    <li class="li" id="link_0" data-count="5">1-3</li>
    <li class="li" id="link_1" data-count="10">4-5</li>
    <li class="li" id="link_2" data-count="30">6-7</li>
    <li class="li" id="link_3" data-count="100">7-9</li>
    <li class="start" id="start">start</li>
</ul>   

<div class="content" id="content">
    <div class="content-inner" id="contentInner"></div>
</div>

\$\endgroup\$
3
\$\begingroup\$

First off I noticed a problem in how the quiz chooses the numbers:

  • It can easily happen that two (or more) numbers are the same. Especially when picking number between 1..5. In that case it can be confusing to the user, which of the two equal numbers should he click.

The main problems with the code resides in tight coupling between UI logic and business logic. For example the createPoints() function:

function createPoints(count) {
    let contentInner = document.getElementById('contentInner');

    for(let i = 0; i <= 2; i++) {
        let num_ = Math.random() * count;
        let num = Math.ceil(num_);

        let pointEl = document.createElement('div');
        pointEl.className = 'point';
        pointEl.id = 'point_' + i;
        pointEl.setAttribute('data-num', num);
        pointEl.innerHTML = num;

        contentInner.appendChild(pointEl);
    };
};

This function both:

  • generates three random numbers and
  • displays the numbers on page.

Would be better two have two functions that separately perform the tasks, something that could be used like so:

displayNumbers(randomNumbersBelow(10))

Also note the naming: the original name createPoints() seems to be talking about some coordinates, which doesn't sound correct to me.

By separating the two, it should be also easier to improve randomNumbersBelow() function so that it would not return duplicates, without having to modify anything inside displayNumbers().

Additionally:

  • num_ variable can be easily eliminated by inlining the expression: Math.ceil(Math.random() * count).
  • Instead of modifying the contentInner element by itself, it could just return the new contents (e.g. as a DocumentFragment). That would eliminate the need for the existence of clearContent() function.
\$\endgroup\$
2
\$\begingroup\$

I know I have mentioned this in previous reviews - it would be wise to cache DOM references. For example, I see multiple places in your code where the element with id attribute contentInner is fetched. I would declare it outside the functions that use it (or better yet, wrap all functions in an IIFE to limit the scope), assign the value in the ready() function and then reference that variable in the other functions (e.g. createPoints(), clearContent()).

It is good that you are using let to limit the scope and the advice above might make the scope wider than desired but that would be an argument for wrapping everything in an IIFE, or else develop some object-oriented wrapper to limit the scope. One could likely get away with using const outside of a DOM-ready callback since most modern browsers are responsive enough.

document.addEventListener("DOMContentLoaded", ready);

let currNum;
let step;
let startContent = 'Это простейший тренажёр для детей, он проверяет способность считать. Предполагается, что: \
<br>дети от 1 до 3 лет должны уметь считать до 5 \
<br>дети от 4 до 5 лет должны уметь считать до 10 \
<br>дети от 6 до 7 лет должны уметь считать до 30 \
<br>дети от 7 до 9 лет должны уметь считать до 100 \
	<br><br>Чтобы проверить знания участнику предлагается последовательно в порядке возрастания выбрать кубики с цифрами \
';
let contentInnerEl;

function init() {
  currNum = -1;
  step = 0
  let startBtn = document.getElementById('start');
  startBtn.click();
};

function clearContent() {
  //let contentInner = document.getElementById('contentInner');
  contentInnerEl.innerHTML = '';
};

function deletePoint(id) {
  let pointEl = document.getElementById(id);
  pointEl.parentNode.removeChild(pointEl);
};

function createPoints(count) {
  //let contentInner = document.getElementById('contentInner');
  for (let i = 0; i <= 2; i++) {
    let num_ = Math.random() * count;
    let num = Math.ceil(num_);

    let pointEl = document.createElement('div');
    pointEl.className = 'point';
    pointEl.id = 'point_' + i;
    pointEl.setAttribute('data-num', num);
    pointEl.innerHTML = num;

    contentInnerEl.appendChild(pointEl);
  };
};


function ready() {
  let ulEl = document.getElementById('ul');
  contentInnerEl = document.getElementById('contentInner');

  let menu = Rx.Observable.fromEvent(ulEl, 'click');
  let contentInner = Rx.Observable.fromEvent(contentInnerEl, 'click');

  let subscription = menu
    .map(e => e.target)
    .filter(t => t.className === 'start' || t.className === 'li')
    .subscribe(function(t) {
      clearContent();
      switch (t.className) {
        case 'start':
          contentInnerEl.innerHTML = startContent;
          break;
        case 'li':
          createPoints(t.getAttribute('data-count'));
          break;
      }
    });

  let subscription3 = contentInner
    .map(e => e.target)
    .filter(t => t.className === 'point')
    .subscribe(function(t) {
      let val = +document.getElementById(t.id).innerHTML;

      ++step;
      deletePoint(t.id);

      if (currNum > val) {
        alert('Not correct');
        init();
      } else {
        currNum = val;
        if (step > 2) {
          alert('Correct');
          init();
        }
      }
    });

  init();
};
<script src="https://unpkg.com/rxjs@5.5.2/bundles/Rx.min.js"></script>
<ul class="ul" id="ul">
  <li class="li" id="link_0" data-count="5">1-3</li>
  <li class="li" id="link_1" data-count="10">4-5</li>
  <li class="li" id="link_2" data-count="30">6-7</li>
  <li class="li" id="link_3" data-count="100">7-9</li>
  <li class="start" id="start">start</li>
</ul>

<div class="content" id="content">
  <div class="content-inner" id="contentInner"></div>
</div>

\$\endgroup\$

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.