Skip to main content
Code Review

Return to Revisions

2 of 5
expand explanation
Sᴀᴍ Onᴇᴌᴀ ♦
Sᴀᴍ Onᴇᴌᴀ ♦
  • 28.4k
  • 16
  • 45
  • 192

##Simplifying the functions into one Yes the three functions can be combined into one. To start, the pos variable can be simplified to default to 0 when no left style has been applied using logical OR because NaN will be converted to false.

let pos = parseInt(avatar.style.marginLeft, 10) || 0;

Notice that the call to parseInt() contains two parameters:

  • the left style of the avatar element without the px characters removed - this is because parseInt() can strip those off.
  • an integer 10 - see feedback point #3 below.

Then a parameter for determining if the avatar should be moved to the end could be added, and since you are using features like let, a default value can be used:

function moveAvatarForward(moveToEnd = false) {

And that value can determine the value of movePos

let movePos = pos + sum;
if (moveToEnd) {
  movePos = progressContainerWidth - avatarSize;
}

And when I tried this (both in the snippet here as well as in jsFiddle), I always saw avatarSize as 0 - did you ever see it greater than that? Perhaps this is because the viewport's initial containing block has no width.

##Other feedback

  1. Use const whenever a variable doesn't need to be re-assigned

  2. Minimize DOM lookups

    Take for example the first two variable declarations:

let startText = document.querySelector('.circle').innerHTML = "click";

   let circle = document.querySelector('.circle');

In this case, there are two DOM lookups for the same element. Instead of doing this, the value for `startText` could be accessed from `circle`:

    const circle = document.querySelector('.circle');
    const startText = circle.innerHTML = "click";

The same is true for `avatarSize` - it can be accessed via `avatar`. And it doesn't need to be re-declared inside `endOfAvatar()` - just remove the keyword `let`. 
    
Additionally, `titleContainer` points to the same thing as `startText` so those can be reduced to one variable.
  1. When using parseInt(): "always specify a radix."1 (i.e. 2nd parameter - typically 10)
  2. use prefix increment in frame() to increment before using a variable

// Init .innerHTML for circle
const circle = document.querySelector('.circle');
circle.innerHTML = "click";
let x = 0;
const progressContainer = document.querySelector('.progress-container');
const avatar = document.querySelector('.avatar');
const avatarSize = avatar.clientWidth;
console.log('avatarSize', avatarSize);
const totalLevels = 5;

// Listen for events
circle.addEventListener('click', mainClickMaze);

// Move the avatar based on its previous location
function moveAvatarForward(moveToEnd = false) {
  let pos = parseInt(avatar.style.marginLeft, 10) || 0;
  const progressContainerWidth = progressContainer.clientWidth;
  const sum = Math.round(progressContainerWidth / totalLevels);
  let movePos = pos + sum;
  if (moveToEnd) {
    movePos = progressContainerWidth - avatarSize;
  }
  requestAnimationFrame(frame);

  function frame() {
    if (pos != movePos) {
      avatar.style.marginLeft = ++pos + "px";
      requestAnimationFrame(frame);
    }
  }
}

function mainClickMaze(event) {
  let click = parseInt(circle.innerHTML, 10) || 0;
  circle.innerHTML = ++click;
  if (click == totalLevels) {
    // Remove event listener 
    circle.removeEventListener('click', mainClickMaze);
    circle.innerHTML = 'Disabled';
  }
  if (click == 1) {
    progressContainer.style.display = 'flex';
  } else {
    moveAvatarForward(click == totalLevels);
  }
}
html,
body {
  padding: 0;
  margin: 0;
}

.container {
  display: flex;
  flex-direction: column;
  align-items: center;
  justify-content: center;
  margin-top: 33.3vh;
  margin-bottom: 33.3vh;
}

.circle {
  display: flex;
  width: 20vw;
  height: 20vw;
  background-color: red;
  border-radius: 50%;
  color: white;
  align-items: center;
  justify-content: center;
  text-align: center;
  font-size: 24px;
  cursor: pointer;
}

.move-container {
  display: flex;
  margin-top: 5vh;
  font-size: 24px;
  display: none;
}

.move-back {
  display: flex;
  align-items: center;
  justify-content: center;
  margin-right: 10vw;
  cursor: pointer;
  width: 10vw;
  padding: 10px;
  background-color: grey;
  border-radius: 25px;
  color: white;
}

.move-forward {
  display: flex;
  align-items: center;
  justify-content: center;
  margin-left: 10vw;
  cursor: pointer;
  width: 10vw;
  padding: 10px;
  background-color: grey;
  border-radius: 25px;
  color: white;
}

.progress-container {
  position: fixed;
  display: none;
  align-items: center;
  width: 90%;
  height: 2px;
  background-color: red;
  bottom: 5vh;
}

.avatar {
  width: 2vw;
  height: 2vw;
  background-color: blue;
  border-radius: 50%;
}
<div class="container">
  <div class="circle"></div>
  <div class="move-container">
    <div class="move-back user-click">Back</div>
    <div class="move-forward user-click">Forward</div>
  </div>
  <div class="progress-container">
    <div class="avatar"></div>
  </div>
</div>

Sᴀᴍ Onᴇᴌᴀ ♦
  • 28.4k
  • 16
  • 45
  • 192