Code that rotates images in a set of divs

Question

I am rotating images inside some divs.

An image is associated with a link.Eg: image0 is associated with link0, image1 with link1 and so on.

image0 and link0 should display in pos-1

image1/link1 and image2/link2 should rotate on page refresh in pos-2

image3/link3 and image4/link4 should rotate on page refresh in pos-3

Although currently I want to display only one image in pos-1, at a later stage I may rotate two images as well in pos-1. Similarly I may display only one image in pos-2.

<div id='pos-1'>
</div>
<div id='pos-2'>
</div>
<div id='pos-3'>
</div>

$(function(){
var image = new Array ();
  image[0] = "<img src='https://placehold.it/200/09f/fff.png?text=image1'/></a>";
  image[1] = "<img src='https://placehold.it/200/000/fff.png?text=image2'/></a>";
  image[2] = "<img src='https://placehold.it/200/22f/000.png?text=image3'/></a>";
  image[3] = "<img src='https://placehold.it/200/32f/fff.png?text=image4'/></a>";
  image[4] = "<img src='https://placehold.it/200/42f/fff.png?text=image5'/></a>";

var link = new Array ();
    link[0] = "<a href='http://www.jquery.com'>";
    link[1] = "<a href='http://www.microsoft.com'>";
    link[2] = "<a href='http://www.yahoo.com'>";
    link[3] = "<a href='http://www.msn.com'>";
    link[4] = "<a href='http://www.stackoverflow.com'>";

var min = 0;
var max = 1;
var x = Math.floor(Math.random() * (max - min) + min);
$('#pos-1').append(link[x]+image[x]);
min=1;
max=3;
x = Math.floor(Math.random() * (max - min) + min);
$('#pos-2').append(link[x]+image[x]);
min=3;
max=5;
x = Math.floor(Math.random() * (max - min) + min);
$('#pos-3').append(link[x]+image[x]);
  });

Please let me know if this code can be improved to perform better?
Demo: https://jsfiddle.net/y0hga2of/5/

Answer 1

The first thing I see in your code are the strings who represents an HTML element (in this case an img and an anchor element).

This is a bad practice. Instead of appending a whole element to your HTML just update the src or href attribute.

The second thing I noticed is your implementation of a new array. Instead of using:

var image = new Array ();
 image[0] = "<img src='https://placehold.it/200/09f/fff.png?text=image1'/></a>";

Use this approach:

var image = [
       "https://placehold.it/200/09f/fff.png?text=image1",
       "https://placehold.it/200/09f/fff.png?text=image2"
];

Also the naming of your variables is badly. The name:

 var image = new Array ();

and

var link = new Array ();

Suggests that they are holding exactly one image and one link. Actually they are holding multiple links and images. Try to use names that describes what you can expect inside the variable. So use images and links instead.

Goodluck!

(Sorry for bad english, not a native speaker)

Answer 2

Cleaning up the Arrays

I second JamesBodgan's suggestion to store only the image src attributes and link href attributes in your arrays instead of storing string representations of the HTML.

This will change the way you consume the values from the arrays. You might want to create a couple functions that create the actual HTML elements from the given attributes for you.

function createLink(url,innerElement){
    let a = document.createElement("a");
    a.setAttribute("href",url);
    a.appendChild(innerElement);
    return a;
}
function createImage(src){
    let img = document.createElement("img");
    img.setAttribute("src",src);
    return img;
}

I'll also second James's suggestion to rename those arrays to images and links to reflect their plurality.

Capture Element IDs and Min/Max Values in a Data Structure

Currently, you have the min/max values and element IDs directly in the code wherever they're needed. You can make the code easier to maintain by moving those values to a separate, dedicated data structure.

I recommend using an array of objects. Then you can loop through the array to perform the necessary operations, which makes adding more elements/images/links almost trivial.

let elements = [
    {id:'#pos-1',min:0,max:1},
    {id:'#pos-2',min:1,max:3},
    {id:'#pos-3',min:3,max:5},
];
for(let i = 0, len = elements.length; i < len; i++){
    let currElement = elements[i];
    let x = Math.floor(Math.random()*(currElement.max - currElement.min) + currElement.min);
    let link = createLink(links[x],createImage(images[x]));
    $(currElement.id).append(link);
}

This also eliminates some repetition from your original code, which is always a good sign.

Note that for improved DOM-query efficiency, you could use the native JavaScript document.getElementById() instead of the generic $() jQuery selector function.

document.getElementById(currElement.id).appendChild(link);

You'd just need to be sure you remove the # prefix from the element IDs.

Do you need jQuery?

At this point, you might want to consider whether you need jQuery at all.

Odds are good that there are other things on the page that are using jQuery, so eliminating it entirely may be unfeasible. But there can be a benefit to minimizing your use of jQuery even on pages where it's being loaded regardless.

The native, underlying JavaScript methods that are invoked by jQuery wrapper functions will always be more efficient (in terms of how quickly the browser executes them), at the expense of being slightly more verbose.

var images = ['https://placehold.it/200/09f/fff.png?text=image1',
  'https://placehold.it/200/000/fff.png?text=image2',
  'https://placehold.it/200/22f/000.png?text=image3',
  'https://placehold.it/200/32f/fff.png?text=image4',
  'https://placehold.it/200/42f/fff.png?text=image5'
];
var links = [
  "http://www.jquery.com",
  "http://www.microsoft.com",
  "http://www.yahoo.com",
  "http://www.msn.com",
  "http://www.stackoverflow.com"
];
let elements = [{
  id: 'pos-1',
  min: 0,
  max: 1
}, {
  id: 'pos-2',
  min: 1,
  max: 3
}, {
  id: 'pos-3',
  min: 3,
  max: 5
}, ];
for (let i = 0, len = elements.length; i < len; i++) {
  let currElement = elements[i];
  let x = Math.floor(Math.random() * (currElement.max - currElement.min) + currElement.min);
  let link = createLink(links[x], createImage(images[x]));
  document.getElementById(currElement.id).appendChild(link);
}

function createLink(url, innerElement) {
  let a = document.createElement("a");
  a.setAttribute("href", url);
  if (!innerElement) {
    innerElement = document.createTextNode(url);
  }
  a.appendChild(innerElement);
  return a;
}

function createImage(src) {
  let img = document.createElement("img");
  img.setAttribute("src", src);
  return img;
}

<div id='pos-1' style='margin-top: 1em;'>
</div>
<div id='pos-2' style='margin-top: 1em;'>
</div>
<div id='pos-3' style='margin-top: 1em;'>
</div>

I find that the native equivalents to jQuery wrappers tend to be more explicitly and precisely named, making it easier for me to recognize mistakes and intentions and making my code more maintainable in the long run.