current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite
1

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?

share|improve this question
    
@Nate_tyrone Then you have to use a javascript framework like jQuery. –  RaviH Mar 9 at 11:51
    
You can use the new selector methods querySelector() and querySelectorAll(). –  shmuli Mar 9 at 11:54
    
It is actually a question, while they do ask for a code review that is not the purpose of the question posted. –  eyegropram Mar 9 at 11:56
    
I personally agree with @Andy: despite the question may be valid, the code is apparently working, therefore reviewing it or writing it in a different (clearer or shorter or whatever) way should not necessarely belong to stackoverflow but, instead, to codereview, where the user can even learn faster, shorter and easier ways to read and understand its own code. –  briosheje Mar 9 at 11:57
1  
There is nothing wrong with document.getElementById(). In fact, jQuery uses it! If you decide to use jQuery, you will be using the same method. So, cut the bloat and use it. It's a lot faster than document.querySelector(). And it's more readable. –  Ismael Miguel Mar 9 at 15:29

4 Answers 4

up vote 0 down vote

You might use document.querySelector() using Native JavaScript 

Ex: document.querySelector("#Img1");

OR

jQuery(If you are open to)

for (var i = 1; 1 < sliderImg; i++ ) {
   flickrDisplay[i] = $("#Img1" + (i + 1)); //Same as flickrDisplay[i] = document.getElementById("img" + (i + 1));
   $("#identifer" + i).css('visibility ','visible'); //Same as document.getElementById("identifer" + i).style.visibility = "visible";
}

NOTE: If you are really concerned about the performance. You should go for document.getElementById(id) instead of $("#id")

getElementById VS jQuery('#id')

share|improve this answer
    
If it is to use document.querySelector("#Img1"), use document.getElementById("Img1"). Which is a lot faster and supported even by IE5.5. –  Ismael Miguel Mar 9 at 15:26
up vote 3 down vote

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.
share|improve this answer
up vote 2 down vote

Take a look the show/hide functions from jquery.

You can use them like this

$( "#id" ).show();
$( "#id" ).hide();

or just toggle the element's visibility

$( "#id" ).toggle();
share|improve this answer
up vote 0 down vote

There are a number of ways you can go about structuring your JavaScript and some of that will come down to style preferences.

Regarding the getElementByID() you could instead use querySelector('#ElementID') the mentioned function takes a css selector.

Regarding the style changes, you could use classList and use CSS classes to apply styles and remove them. However the method you are using is perfectly acceptable. You could instead use a function to toggle the visibility. You will also want to spell "visible" correctly ;)

share|improve this answer

protected by Community♦ Mar 9 at 13:15

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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