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

Foreach loop displaying instruction relative to a release number

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

Problem

I've got a foreach loop, going through each section of an XML document (an example of said XML document can be found in this SO question) and only display them in divs with corresponding classes.


  
  
    
  
  
    
      
        
        
          
        
      
    
  
  
    
      
        
        
          
        
      
    
  
  
    
      
        
        
          
        
      
    
  
  
    
      
        
        
          
        
      
    
  
  
    
      
        
        
          
        
      
    
  


What I do after that is using a drop-down menu, and with the help of javascript switch the style.display property of the divs accordingly.

Form:


  
     10r3
     11r2
     12r1
     12r2
     13r1
   
 


Javascript:

function showHide(selection) {
    var f = selection.form;
    // Retrieve the value of the option selected.
    var opt = selection.options[selection.selectedIndex].value;
    /* Retrieve all the div elements of the document and then parse them to
     * see if they have a classname corresponding with the selected release.
     * If it is the case, remove all display style, otherwise set the
     * display style to none.
     */
    var divArray = document.getElementsByTagName("div");
    for(var i=0; i<divArray.length; i++){
        var relArray = ["10r3","11r2","12r1","12r2","13r1"];
        for(var j=0; j<relArray.length; j++){
            var relName = relArray[j];
            if(divArray[i].className == "rel"+relName){
                if(opt===relName){
                    divArray[i].style.display = "";
                }else{
                    divArray[i].style.display = "none";
                }
            }
        }
    }
    return false;
}


I've already refactored my javascript code, but there is still plenty of repetitive code out there, and it can only grow the more releases there will be.

EDIT: One more thing, those instruction are shown in a CSS pop-up using a t

Solution

-
in your showHide() function:

var relArray = ["rel10r3","rel11r2","rel12r1","rel12r2","rel13r1"];
for(var i=0; i<divArray.length; i++)
{
   var currentDiv = divArray[i];
   if(currentDiv.className && relArray.indexOf(currentDiv.className) != -1)
   {
        currentDiv.style.display = "";     
   }
   else
   {
        currentDiv.style.display = "none";
   }
}


-
Don't be skimpy on the formatting as it makes a big difference on the readability. forexample:

function toggle(div_id) {
    var el = document.getElementById(div_id);
    if (el.style.display == 'none' )
    {
        el.style.display = 'block';
    }
    else 
    {
        el.style.display = 'none';
    }
}


-
Javascript is very nice with falsey values:

if (typeof window.innerWidth != 'undefined') {
    viewportheight = window.innerHeight;
} else {
    viewportheight = document.documentElement.clientHeight;
}


can be written:

if (window.innerWidth) {
    viewportheight = window.innerHeight;
} else {
    viewportheight = document.documentElement.clientHeight;
}


or

viewportheight = window.innerHeight || document.documentElement.clientHeight;


-
patterns like:

if(){
    //
}else{
    if(){
        //
    }else{
        //
    }
}


can be written much easier and cleaner:

if(){
    //
}else if(){
    //
}else{
    //
}


-
you have:

if ((viewportwidth > document.body.parentNode.scrollWidth) && (viewportwidth > document.body.parentNode.clientWidth)) {
     //etc etc


can be:

window_width = Math.max(viewportwidth, document.body.parentNode.clientWidth, document.body.parentNode.scrollWidth);


-
you are using variables to hold a value for one line:

var popUpDiv = document.getElementById(popUpDivVar);
window_width=50;
popUpDiv.style.left = window_width + 'px';


instead:

var popUpDiv = document.getElementById(popUpDivVar);
if(popUpDiv) // incase it couldn't find the div.
{
    popUpDiv.style.left = "50px";
}


TL;DR? http://jsfiddle.net/37Uge/1/

Code Snippets

var relArray = ["rel10r3","rel11r2","rel12r1","rel12r2","rel13r1"];
for(var i=0; i<divArray.length; i++)
{
   var currentDiv = divArray[i];
   if(currentDiv.className && relArray.indexOf(currentDiv.className) != -1)
   {
        currentDiv.style.display = "";     
   }
   else
   {
        currentDiv.style.display = "none";
   }
}
function toggle(div_id) {
    var el = document.getElementById(div_id);
    if (el.style.display == 'none' )
    {
        el.style.display = 'block';
    }
    else 
    {
        el.style.display = 'none';
    }
}
if (typeof window.innerWidth != 'undefined') {
    viewportheight = window.innerHeight;
} else {
    viewportheight = document.documentElement.clientHeight;
}
if (window.innerWidth) {
    viewportheight = window.innerHeight;
} else {
    viewportheight = document.documentElement.clientHeight;
}
viewportheight = window.innerHeight || document.documentElement.clientHeight;

Context

StackExchange Code Review Q#4861, answer score: 2

Revisions (0)

No revisions yet.