6
\$\begingroup\$

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;    
}
<div id="wrapper">
    <div id="abc-container">
        <div id="a"></div>
        <div id="b"></div>
        <div id="c"></div>
    </div>
</div>
<div id="asas"></div>
<div id="asass"></div>

\$\endgroup\$

2 Answers 2

Reset to default
6
\$\begingroup\$

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
<head>
    <link rel="stylesheet" type="text/css" href="style.css" />
</head>
... more code
<script src="main.js"></script>

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.

<div id="abc-container">
    <div id="a" class="box"></div>
    <div id="b" class="box"></div>
    <div id="c" class="box"></div>
</div>

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:

<!DOCTYPE html>
<html>
    <head>
        <link rel="stylesheet" type="text/css" href="style.css" />
    </head>
    <body>
        <div id="wrapper">
            <div id="abc-container">
                <div id="a" class="box"></div>
                <div id="b" class="box"></div>
                <div id="c" class="box"></div>
            </div>
        </div>
        <div id="asas"></div>
       <!-- !!!    Use script tag to import "main.js" -->
    </body>
</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

\$\endgroup\$
5
  • \$\begingroup\$ Hi Thanks for you help Larry, It's very very very great, I love it!! Thanks \$\endgroup\$
    – user1697962
    Commented Sep 27, 2012 at 15:50
  • \$\begingroup\$ do you have an idea to make it looping? \$\endgroup\$
    – user1697962
    Commented Oct 1, 2012 at 9:03
  • \$\begingroup\$ @user1697962 It's looping becuase of this condition. if( abc_container < firstval ){ firstval = 0; } \$\endgroup\$
    – Larry Battle
    Commented Oct 1, 2012 at 15:22
  • \$\begingroup\$ @user1697962 I updated the animation so it only cycles once. Another call to moveToNextSlide() will start the cycle again. \$\endgroup\$
    – Larry Battle
    Commented Oct 1, 2012 at 15:37
  • \$\begingroup\$ I would have add "do not use IDs for CSS" \$\endgroup\$
    – dievardump
    Commented Oct 2, 2012 at 2:43
0
\$\begingroup\$

I agree with everything in Larry's answer and there is one other big point, which may be implied in the Final Code: indentation is crucial for readability.

There isn't an official "correct" indentation amount but general conventions call for two or four spaces when starting a block. For example, the Google JavaScript Style Guide calls for 2 spaces:

Each time a new block or block-like construct is opened, the indent increases by two spaces. When the block ends, the indent returns to the previous indent level. The indent level applies to both code and comments throughout the block.1

Meanwhile, Doug Crockford's Code Conventions for the JavaScript Programming Language recommends 4 spaces.

Every statement should begin aligned with the current indentation. The outermost level is at the left margin. The indentation increases by 4 spaces when the last token on the previous line is {left brace, [left bracket, (left paren. The matching closing token will be the first token on a line, restoring the previous indentation.2

The first version of your code appears to have various levels of indentation:

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;
        }
        //...

To format this consistently, use a consistent indentation:

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;
    }

And the same also applies to the CSS.

1https://google.github.io/styleguide/jsguide.html#formatting-block-indentation 2https://crockford.com/javascript/code.html#whitespace

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.