Carousel image slideshow script

I have learned to make a carousel image slideshow script with JavaScript. I think it's better to learn it from the basic than just from the framework (instant script), but I'm a newbie. I wrote this script using my technique, but I think it's terrible.



var runCarousel, runTimer;
firstval = 0;
secondval = 0;

function Carousel(){
firstval += 10;
document.getElementById('abc-container').style.left = "-" + firstval + "px";
document.getElementById('asas').innerHTML = "-" + firstval;
if(firstval == 400)
{
StopRun();
StartTimer()
return;
}
if(firstval == 800)
{
StopRun();
StartTimer()
return;
}
runCarousel = setTimeout(Carousel, 10);
}

function StartTimer(){
secondval += 1;
document.getElementById('asass').innerHTML = "-" + secondval;
if(secondval == 10)
{
StopTimer();
Carousel();
return;
}
if(secondval == 20)
{
StopTimer();
Carousel();
return;
}
runTimer = setTimeout(StartTimer, 1000);
}

function StopRun(){
window.clearTimeout(runCarousel);
}

function StopTimer(){
window.clearTimeout(runTimer);
}

function firstStart(){
setTimeout("Carousel()", 10000);
}

firstStart();
#wrapper {
width:400px;
height:140px;
position:absolute;
left:50%;
top:50%;
margin: -70px 0 0 -200px;
background: #383838;
overflow: hidden;
}

#abc-container {
position: absolute;
width:1200px;
height:140px;
}

#a {
width:400px;
height:140px;
background: #FF0000;
float: left;
}

#b {
width:400px;
height:140px;
background: #FFFF00;
float: left;
}

#c {
width:400px;
height:130px;
background: #00FFFF;
float: left;
}
`

Great start for far but you need to work on your overall design.

Here are some tips.

1) Separate the css, html and javascript into their own files.

If you were place all your css into a file called "style.css" and javascript in a file called "main.js", then you could import the files from your html like so.

... more code

    

... more code

2) For your css, try using a creating a class for the box to share common properties.

Old Code:

#a {
    width:400px;
    height:140px;
    background: #FF0000;
    float: left;
}

#b {
    width:400px;
    height:140px;
    background: #FFFF00;
    float: left;
}

#c {
    width:400px;
    height:130px;
    background: #00FFFF;
    float: left;    
}

New Code:

#a {
    background: #FF0000;
}
#b {
    background: #FFFF00;
}
#c {
    background: #00FFFF;
}
.box{
    width:400px;
    height:140px;
    float: left; 
}

Remember to add the classes.

3) Don't repeat yourself.

The if conditions for firstval and secondval can be combined by using or, ||.

Old Code:

if(firstval == 400){
    StopRun();
    StartTimer()
    return;
}
if(firstval == 800){
    StopRun();
    StartTimer()
    return;
}
runCarousel = setTimeout(Carousel, 10);

New Code:

if(firstval == 400 || firstval == 800){
    StopRun();
    StartTimer()
}else{
    runCarousel = setTimeout(Carousel, 10);
}

4) Declare all variables are the top of a function.

Old Code:

var runCarousel, runTimer;
firstval = 0;
secondval = 0;

New Code:

var runCarousel, runTimer, firstval = 0, secondval = 0;

5) Pass the function name instead of string to setTimeout()

Also it's better to use multiplication when setting the timeout delay.

Old Code:

setTimeout("Carousel()", 10000);

New Code:

setTimeout( Carousel, 10 * 1000);

6) Attach all the global variables to a object literal.

7) Name variables and functions based on their operations.

• Rename Carousel() to moveToNextSlide()

8) Cache elements for faster lookup.

Code:

var infoEl = document.getElementById('asas'), 
container = document.getElementById('abc-container');

9) There needs to be a limit for firstval inside Carousel()

Add this:

var abc_container = 1200;
firstval += 10;
if( abc_container < firstval ){
    firstval = 0;
}

10) Use variables in place of numeric constants.

It's hard to understand the meaning of 400.
It seems that 400 is the width of each box.

11) secondval isn't needed. Just place everything inside setTimeout() for 10 seconds.

Better yet seconds as a parameter.

Old Code:

function StartTimer() {
    secondval += 1;
    el.innerHTML = "-" + secondval;
    if (secondval == 10 || secondval == 20 ) {
        runTimer = window.setTimeout(StartTimer, 1000);
    }else{
    ...
}

New Code:

function StartTimer( seconds ) {
    setTimeout(function(){
        window.clearTimeout(runTimer);
        moveToNextSlide();
    }, seconds * 1000);
}

12) Read the source code from other image slideshow to find out better techniques.

Here's a place to start. http://www.tripwiremagazine.com/2012/08/jquery-image-slider.html

Final Code:

HTML:

Javascript:

var carouselTimer, runTimer, firstval = 0, 
    boxWidth = 400, abc_container = 1200,
    infoEl = document.getElementById('asas'), 
    container = document.getElementById('abc-container');

function moveToNextSlide() {
    firstval += 10;
    if( abc_container < firstval ){
        firstval = 0;
        return;
    }
    container.style.left = "-" + firstval + "px";
    infoEl.innerHTML = "container.style.left.px = " + firstval;
    if (firstval % boxWidth) {
        carouselTimer = setTimeout(moveToNextSlide, 10);
    }else{
        window.clearTimeout(carouselTimer);
        StartTimer( 2 );
    }
}
function StartTimer( seconds ) {
    setTimeout(function(){
        window.clearTimeout(runTimer);
        moveToNextSlide();
    }, seconds * 1000);
}
StartTimer( 2 );

CSS:

#wrapper {
    width:400px;
    height:140px;
    position:absolute;
    left:50%;
    top:50%;
    margin: -70px 0 0 -200px;
    background: #383838;
    overflow: hidden;
}
#abc-container {
    position: absolute;
    width:1200px;
    height:140px;
}
#a {
    background: #FF0000;
}
#b {
    background: #FFFF00;
}
#c {
    background: #00FFFF;
}
.box{
    width:400px;
    height:140px;
    float: left; 
}

Demo: http://jsfiddle.net/vSWmx/2

StackExchange Code Review Q#15948, answer score: 6