HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Constructing a JavaScript slide

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
javascriptconstructingslide

Problem

I am a newbie to native JavaScript and am pretty much coming to grips with the operators, etc. At the moment I am currently constructing a slide.

"use strict"; 

 var intSeconds = 3;

 function loadSlider(){

flickrImgsShown = 0; 
sliderImages = new Array(slides + 10);
sliderImages[0] = document.querySelector(".img");

    for (var i = 1; 1 < sliderImg; i++ ) {
        sliderImages[i] = document.querySelector(".img" + (i + 1));
        document.querySelector(".identifer" + i);
    }

    function next() {

        flickrImgsShown++; 
        if(flickrImgsShown == slides)
            flickrImgsShown = 0;
        transition();

        clearInterval(slideTimer);
        slideTimer = setInterval(timer, interSeconds * 1000);
    }

    function prev () {

        flickrImgsShown--; 
        if(flickrImgsShown == -1)
            flickrImgsShown = slides -1;
        transition();

        clearInterval(slideTimer);
        slideTimer = setInterval(timer, interSeconds * 1000);
    }

    slideTimer = setInterval(timer, intSeconds * 1000);

    function timer() {

        flickrImgsShown--; 
        if(flickrImgsShown == -1)
            flickrImgsShown = slides -1;
        transition();
    }

}


I am not a fan of the getelementbyidand style.visibility in my code. Is there a better method I can apply to call out an ID or change a style in the DOM?

Solution

Your questions seem to have been answered (use jQuery), so here are a couple of small points regarding readability:

  • be consistent with your variable names (eg sliderImg vs flickrImgs: one is plural, one singular, and in the rest of the code, you mainly use flickrX, except for the sliderImg one; I would prefer sliderX everywhere, because it doesn't seem to matter where the images come from).



  • some variable names could be a lot better. Eg when reading flickrImgs/sliderImg I'm thinking it's an array of images (or image paths/names). But they are not, they are integers, so I would prefer imageCount/imageSize, and currentSliderPosition. flickrDisplay on the other hand seems to actually be a collection of images, so sliderImages would probably be a good name for it.



  • your indentation is off, which decreases the readability of your code.



  • your comments aren't all that helpful. NOW AND NEXT FUNCTION/SLIDE TRANSITION aren't really necessary, and For each loop incrementing each slide by one doesn't really help either (because that's exactly what the code already told me; If you do want to add a comment, it would be more helpful to explain what it means when a slide is increased.



  • use curly brackets even for one line statements to increase readability and to possibly avoid bugs.



  • visiable isn't a valid value for visibility, you probably meant visible.

Context

StackExchange Code Review Q#83640, answer score: 3

Revisions (0)

No revisions yet.