Help me simplify this JS code

Question

I'm a beginner in JS and want to know a way to simplify this code. There are 7 different divs with iframes, and also 7 different links. I have shown 1 div with iframe and 1 link. I have no idea where to start. Any help would be greatly appreciated.

NOTE: The code works to my needs, but I just need to simplify it (less js code in html, and more in js file).

JavaScript in .js file:

function show_visibility(){
for(var i = 0,e = arguments.length;i < e;i++){
var myDiv = document.getElementById(arguments[i]).style;
myDiv.display = "block";}
}

function hide_visibility(){
for(var i = 0,e = arguments.length;i < e;i++){
var myDiv = document.getElementById(arguments[i]).style;
myDiv.display = "none";}
}
 function refFrame()    {
for(var i = 0,e = arguments.length;i < e;i++){
document.getElementById(arguments[i]).src = document.getElementById(arguments[i]).src;
}

}

Div/iframe to be modified:

<div id="r1-box">
<iframe id="frame-box1" class="work" src="youtubelink" width="720" height="405" frameborder="0"></iframe>
</div>

Link to execute JS:

<a id="r1" href="javascript:refFrame('frame-box2','frame-box3','frame-box4','frame-box5','frame-box6','frame-box7');show_visibility('r1-box');hide_visibility('r2-box','r3-box', 'r4-box','r5-box','r6-box','r7-box');">
</a>

Perhaps use a class instead to select the necessary elements, maybe use jQuery? — Orbling, Jun 01 '11 at 00:34
I'm hoping the code indents in your JS are messed up due to pasting it into SO, rather than messed up in your source too. — sarnold, Jun 01 '11 at 00:35
maybe you should stop and think about what this line is supposed to do: document.getElementById(arguments[i]).src = document.getElementById(arguments[i]).src — tomfumb, Jun 01 '11 at 00:39
@user519575 - it refreshes the iframe. I use it to stop the Vimeo video from playing when a link is pressed. — khwebdesigns, Jun 01 '11 at 00:42
@khwebdesigns: please [indent](http://en.wikipedia.org/wiki/Indent_style) lines for readability. You'll find it very helpful to do this with all your code. A good IDE or editor will indent your code for you. — outis, Jun 01 '11 at 01:07

score 2 · Answer 1 · edited May 23 '17 at 12:04

As a beginner you shouldn't start using jQuery until you understand Javascript more.

There are a few ways you could simplify this, the most immediate one would be to get the Javascript out of the link and into a Javascript file, or at the top of the page:

window.onload = function() {
    document.getElementById('#r1').onclick = function() {
        refFrame('frame-box2','frame-box3','frame-box4','frame-box5','frame-box6','frame-box7');
        show_visibility('r1-box');
        hide_visibility('r2-box','r3-box', 'r4-box','r5-box','r6-box','r7-box');
    };
    // more...
};

window.onload is an event which fires once the page has - you guessed it - finished loading. There are better ways of doing this, but this is about as basic as it gets. I'd advise you look at javascript domready?

After looking at your code a bit more, I realised all your seven links will do essentially the same thing. You can simply this by using a single function:

function refClick(id) {
    var i = 7,
    frames = [],
    boxes = [];
    while(i--) {
        if(i != id) {
            frames.push('frame-box' + i);
            boxes.push('r' + i + '-box');
        }
    }
    refFrame.apply(null, frames);
    hide_visibility.apply(null, boxes);
    show_visibility('r' + id + '-box');
}

What I'm doing here is looping through 7 times, and building an array of arguments for the refFrame and hide_visibility functions. The id variable tells the loop not to put in that id into the arrays. Using the .apply method, I can apply an array as the arguments and call it normally.

For each of your links, you can apply the following function

document.getElementById('#r1').onclick = function() {
    refClick(1);
};
document.getElementById('#r2').onclick = function() {
    refClick(2);
};
//.....

Hi Benjammin' Thanks for the input! When I use the code, however, I can't seem to make the hide_visibility and show_visibility work. It does not load the other divs. — khwebdesigns, Jun 01 '11 at 08:26
Here is a little background on what I'm trying to achieve:I have a gallery of videos using show/hide divs. When I click a thumbnail image, it shows a div and hides the rest. The refresh function is to refresh all the divs except for the current one. This is to stop the previous video from playing when a user clicks on another thumbnail (I have auto-play to off). — khwebdesigns, Jun 01 '11 at 08:29
There was actually a small error in the code I wrote - I had show_visibility('r' + i + '-box') when it should have been show_visibility('r' + id + '-box'). Try that out and let me know how it goes :) — Ben, Jun 01 '11 at 12:44
I've changed the code, but it still doesn't show. Maybe I'm doing it wrong? Do I put window.onload = function() { document.getElementById('#r1').onclick = function () { refClick(1); }; document.getElementById('#r2').onclick = function () { refClick(2); }; in the header? — khwebdesigns, Jun 01 '11 at 16:46

score 1 · Answer 2 · answered Jun 01 '11 at 00:35

1

You could start using jQuery.

http://jquery.com/

answered Jun 01 '11 at 00:35

Steve Wellens

20,506
2
28
69

ha. i dont get how my response was voted down and this was voted up!! I give it a plus 1, my sentiments exactly! – Glenn Ferrie Jul 01 '11 at 05:57

Help me simplify this JS code

2 Answers2