4
\$\begingroup\$

I have been building my own carousel over the past few days. I am not a jQuery guru, just an enthusiast, and I think my code is a little sloppy, hence the post.

Basically what happens is:

  • If someone hits the page with a # values in the URL it will show the appropriate slide and example would be www.hello.com#two, this would slide to slide two.
  • If someone clicks the numbers it will show the appropriate slide.
  • Next and prev also slide through the slides.

Is there anything I could have written better, as I know there is a lot of duplicate code?

if ($slideNumber == 1) {
    $('#prev').attr("class", "not_active")
    $('#next').attr("class", "active")
}
else if ($slideNumber == divSum) {
    $('#next').attr("class", "not_active");
    $('#prev').attr("class", "active");
}
else {
    $('#prev').attr("class", "active")
    $('#next').attr("class", "active")
};

I've made a jsFiddle and included the complete code below:

$(document).ready(function () {

    ////////////////////////////  INITAL SET UP  /////////////////////////////////////////////

    //Get size of images, how many there are, then determin the size of the image reel.
    var divWidth = $(".window").width();
    var divSum = $(".slide").size();
    var divReelWidth = divWidth * divSum;

    //Adjust the image reel to its new size
    $(".image_reel").css({ 'width': divReelWidth });

    //set the initial not active state
    $('#prev').attr("class", "not_active");

    ////////////////////////////  SLIDER  /////////////////////////////////////////////

    //Paging + Slider Function
    rotate = function () {
        var triggerID = $slideNumber - 1; //Get number of times to slide
        var image_reelPosition = triggerID * divWidth; //Determines the distance the image reel needs to slide
        //sets the active on the next and prev
        if ($slideNumber == 1) {
            $('#prev').attr("class", "not_active")
            $('#next').attr("class", "active")
        }
        else if ($slideNumber == divSum) {
            $('#next').attr("class", "not_active");
            $('#prev').attr("class", "active");
        }
        else {
            $('#prev').attr("class", "active")
            $('#next').attr("class", "active")
        };
        //Slider Animation
        $(".image_reel").animate({
            left: -image_reelPosition
        }, 500);
    };

    ////////////////////////////  SLIDER CALLS  /////////////////////////////////////////////

    //click on numbers
    $(".paging a").click(function () {
        $active = $(this); //Activate the clicked paging
        $slideNumber = $active.attr("rel");
        rotate(); //Trigger rotation immediately
        return false; //Prevent browser jump to link anchor
    });


    //click on next button
    $('#next').click(function () {
        if (!$(".image_reel").is(':animated')) { //prevent clicking if animating
            var left_indent = parseInt($('.image_reel').css('left')) - divWidth;
            var slideNumberOn = (left_indent / divWidth);
            var slideNumber = ((slideNumberOn * -1) + 1);
            $slideNumber = slideNumber;
            if ($slideNumber <= divSum) { //do not animate if on last slide
                rotate(); //Trigger rotation immediately
            };
            return false; //Prevent browser jump to link anchor
        }
    });

    //click on prev button
    $('#prev').click(function () {
        if (!$(".image_reel").is(':animated')) { //prevent clicking if animating
            var left_indent = parseInt($('.image_reel').css('left')) - divWidth;
            var slideNumberOn = (left_indent / divWidth);
            var slideNumber = ((slideNumberOn * -1) - 1);
            $slideNumber = slideNumber;
            if ($slideNumber >= 1) { //do not animate if on first slide
                rotate(); //Trigger rotation immediately
            };
        }
        return false; //Prevent browser jump to link anchor
    });

    //URL eg:www.hello.com#one                  
    var hash = window.location.hash;
    var map = {
        one: 1,
        two: 2,
        three: 3,
        four: 4
    };
    var hashValue = map[hash.substring(1)];
    //animate if hashValue is not null
    if (hashValue != null) {
        $slideNumber = hashValue;
        rotate(); //Trigger rotation immediately
        return false; //Prevent browser jump to link anchor
    };
});
.window {
    height:200px;    width: 200px;
    overflow: hidden; /*--Hides anything outside of the set width/height--*/
    position: relative;
}
.image_reel {
    position: absolute;
    top: 0; left: 0;
}
.slide {float:left; width:200px; height:200px}

.one {background: #03C}
.two {background:#0F6}
.three {background: #39C}
.four {background:#FC3}

.hide {display:none}

.not_active {background:#F00}
.active {background:#0F0}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.7.1/jquery.min.js"></script>

        <div class="window">    

            <div class="image_reel">
                <div class="slide one">slide 1</div>
                <div class="slide two">slide 2</div>
                <div class="slide three">slide 3</div>
                <div class="slide four">slide 4</div>
            </div>
        </div>

        <div class="paging">
            <a href="#" rel="1">one</a>
            <a href="#" rel="2">two</a>
            <a href="#" rel="3">three</a>
            <a href="#" rel="4">for</a>
        </div>

        <div class="nav">
            <a id="prev" class="active" href="#">prev</a>
            <a id="next" class="active" href="#">next</a>
        </div>

\$\endgroup\$
4
\$\begingroup\$

1) Separation of Concerns

Start by refactorring your code in to more granular functions. You can read more about SoF at http://en.wikipedia.org/wiki/Separation_of_concerns

Update: E.g. Instead of having your reel resizing code inline, put it in it's own function, like this:

function setImageReelWidth () {
    //Get size of images, how many there are, then determin the size of the image reel.
    var divWidth = $(".window").width();
    var divSum = $(".slide").size();
    var divReelWidth = divWidth * divSum;

    //Adjust the image reel to its new size
    $(".image_reel").css({ 'width': divReelWidth });                            
}

This achieves 2 things:

a. First, it groups a block of code that is logically cohesive, removing it from the main code which results in a much cleaner code habitat. b. It effectively gives a label to the code block via the function name that is descriptive of what it does, and therefore makes understanding of the code much simpler.

Later, you can also encapsulate the whole thing in it's own "class" (function) and you can move it into it's own js file.

2) The jQuery "on" function

Use the "on" function to attach your click events, rather than the "click" function. http://api.jquery.com/on/ This has the added advantage of also binding it to future elements matching your selector, even though they do not exist yet.

3) The ready function

// I like the more succinct:
$(handler)
// Instead of:
$(document).ready(handler)

But you might like the more obvious syntax.

Those are just a few things to start with.

-- Update 1 --

I think you should keep your original code block in your question, so that future readers can see where it started and how it systematically improved.

I would like to know more about the create a new function, outside of the jQuery ready block as i cant get this working or quite understand how i can get it to work sorry

I am not familiar with jsfiddle.net, but it looks cool and helpful, but might also be a bit confusing if you don't know what is going on. I am not sure I do :), but I think that script editor window results in a .js file that is automatically referenced by the html file.

So here is an example of a function defined outside of the ready block, but referenced from within.

function testFunction () {
    alert ('it works');
}
$(document).ready(function () {
    testFunction();

    // ... other code
});

This should pop up an alert box that says, "it works" when the page is loaded. You can try it for yourself. Then, once you got that working, you can refactor other logically cohesive blocks of code into their own functions. Later you can wrap them all up into their own javascript 'class'. But we'll get to that.

also is there a better way to write:

if ($slideNumber == 1) {
    $('#prev').attr("class", "not_active")
    $('#next').attr("class", "active")
}
else if ($slideNumber == divSum) {
    $('#next').attr("class", "not_active");
    $('#prev').attr("class", "active");
}
else {
    $('#prev').attr("class", "active")
    $('#next').attr("class", "active")
};

One option is to write it like this:

$('#prev').attr("class", "not_active")
$('#next').attr("class", "not_active")
if ($slideNumber > 1)
    $('#prev').attr("class", "active")
if ($slideNumber < divSum )
    $('#next').attr("class", "active")
\$\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.