2
\$\begingroup\$

I wrote the following code for move a box to right and return it to left, and repeat this action forever.

This code works, but is there a better solution?

function move(){
 let box = document.getElementById("box");
 if(box.style.left.replace("px","") < 180 && check == true){
   moveRight(box);
  }else if (box.style.left.replace("px","") == 180 || check == false)
    {

      if(box.style.left.replace("px","") > 0){
          check = false;
          moveleft(box);
      }else if (box.style.left.replace("px","") == 0){
            check = true;
        }
    }
}
function moveRight(box){
   if(box.style.left != ""){
     let temp = box.style.left.replace("px","");
     temp = parseInt(temp);
     box.style.left = temp+1+"px";
   }else{box.style.left = "1px";}
}
function moveleft(box){
   if(box.style.left != ""){
     let temp = box.style.left.replace("px","");
     temp = parseInt(temp);
     box.style.left = temp-1+"px";
   }else{box.style.left = "1px";}
}
var check = true;
setInterval(move,10);
#plan{
        width: 200px;
        height: 200px; 
		background: #00C0FF;
        position: relative;
	}
	#box{
	    width: 20px;
		height: 20px;
		background: red;
        position: absolute;
		
}
<!DOCTYPE html>
<html>
<head>
</head>
<body>
	<div id="plan">
		<div id="box"></div>
	</div>
</body>
</html>

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

Please take care to use consistent indentation for your JavaScript, HTML, and CSS. In particular, avoid mixing spaces and tabs.

Avoid using meaningless variable names such as check and temp. In this case, you didn't even need temp, because:

If parseInt encounters a character that is not a numeral in the specified radix, it ignores it and all succeeding characters and returns the integer value parsed up to that point. parseInt truncates numbers to integer values. Leading and trailing spaces are allowed.

Your sleep() function appears to be unused. In any case, you should avoid having a while loop that simply wastes CPU power; setInterval() and setTimeout() are much more appropriate ways to control timing. For DOM-related updates and animations, though, you should use requestAnimationFrame().

The entire problem would be better solved using CSS animations, which are supported by a large proportion of modern browsers. In fact, the set of browsers that support the let keyword (which you used) is a subset of the browsers that support CSS Animations.

@keyframes slide {
    from { left: 0; }
    to   { left: 180px; /* 200px - 20px */ }
}

#plan {
    width: 200px;
    height: 200px; 
    background: #00C0FF;
    position: relative;
}

#box {
    width: 20px;
    height: 20px;
    background: red;
    position: absolute;
    animation: 3.6s linear infinite alternate slide;
}
<div id="plan"><div id="box"></div></div>

\$\endgroup\$
  • 1
    \$\begingroup\$ -1 for bad advice. setTimeout, or setIntervalshould NEVER be used to render DOM animations. They run out of sync with the display, present new content immediately on exit, rather than delayed till Vsync, and continue even when page content is not visible, chewing up power and resources for no reason. Always use requestAnimationFrame for animating the DOM (and for that matter any type of DOM updates) \$\endgroup\$ – Blindman67 Feb 8 '18 at 3:30
  • 1
    \$\begingroup\$ @Blindman67 Thanks for the reminder. Incorporated into Rev 2. \$\endgroup\$ – 200_success Feb 8 '18 at 3:33

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.