4
\$\begingroup\$

I have a personal website that I use to share videos and images with friends. Below is a media generator using JavaScript and HTML. Its main purpose is to display one image at a time at the click of a button, but any type of media can be displayed. My goal was to create a fast-loading page to hold all of my media.

//VIDEO ARRAY var oddvideo = [ 'video1', 'video2', 'video3', 'video4', 'video5', ]; //AUDIO ARRAY var oddaudio = [ 'audio1', 'audio2', 'audio3', 'audio4', 'audio5', ]; //PHOTO ARRAY var oddphoto = [ 'photo1', 'photo2', 'photo3', 'photo4', 'photo5', ]; //TEXT ARRAY var oddtext = [ 'text1', 'text2', 'text3', 'text4', 'text5', ]; //RANDOM UNUSED ARRAY ITEMS var Uvideo = []; var Uaudio = []; var Uphoto = []; var Utext = []; //OLD-NEW VARIABLES var videoFor = 0; var audioFor = 0; var photoFor = 0; var textFor = 0; //NEW-OLD VARIABLES var videoRev = oddvideo.length - 1; var audioRev = oddaudio.length - 1; var photoRev = oddphoto.length - 1; var textRev = oddtext.length - 1; //GENERATOR FUNCTION function newThing() { //RANDOM MODE if(mode1.checked && (videoCheck.checked || audioCheck.checked || photoCheck.checked || textCheck.checked)) { if (videoCheck.checked) { if (!Uvideo.length) Uvideo = [...oddvideo]; var randomY = Uvideo; } if (audioCheck.checked) { if (!Uaudio.length) Uaudio = [...oddaudio]; var randomY = Uaudio; } if (photoCheck.checked) { if (!Uphoto.length) Uphoto = [...oddphoto]; var randomY = Uphoto; } if (textCheck.checked) { if (!Utext.length) Utext = [...oddtext]; var randomY = Utext; } var randomX = Math.floor(Math.random() * (randomY.length)); var y = randomY; var x = randomX; document.getElementById("thingDisplay").innerHTML = y[x]; // remove randomx from the unused array since it's been used now randomY.splice(randomX, 1); } //OLD-NEW MODE if(mode2.checked && (videoCheck.checked || audioCheck.checked || photoCheck.checked || textCheck.checked)) { if(videoCheck.checked) { document.getElementById('thingDisplay').innerHTML = oddvideo[videoFor]; videoFor++; if (videoFor >= oddvideo.length) videoFor = 0; } if(audioCheck.checked) { document.getElementById('thingDisplay').innerHTML = oddaudio[audioFor]; audioFor++; if (audioFor >= oddaudio.length) audioFor = 0; } if(photoCheck.checked) { document.getElementById('thingDisplay').innerHTML = oddphoto[photoFor]; photoFor++; if (photoFor >= oddphoto.length) photoFor = 0; } if(textCheck.checked) { document.getElementById('thingDisplay').innerHTML = oddtext[textFor]; textFor++; if (textFor >= oddtext.length) textFor = 0; } } //NEW-OLD MODE if(mode3.checked && (videoCheck.checked || audioCheck.checked || photoCheck.checked || textCheck.checked)) { if(videoCheck.checked) { document.getElementById('thingDisplay').innerHTML = oddvideo[videoRev]; videoRev--; if (videoRev < 0) videoRev = oddvideo.length - 1; } if(audioCheck.checked) { document.getElementById('thingDisplay').innerHTML = oddaudio[audioRev]; audioRev--; if (audioRev < 0) audioRev = oddaudio.length - 1; } if(photoCheck.checked) { document.getElementById('thingDisplay').innerHTML = oddphoto[photoRev]; photoRev--; if (photoRev < 0) photoRev = oddphoto.length - 1; } if(textCheck.checked) { document.getElementById('thingDisplay').innerHTML = oddtext[textRev]; textRev--; if (textRev < 0) textRev = oddtext.length - 1; } } }
<body> <div align="center" id='thingDisplay'></div> <div align="center"> <button onclick="newThing()">New Thing</button> </div> <form id="mode"> <label><input type="radio" name="modes" id="mode1"/></label>&nbsp;Random <br/><label><input type="radio" name="modes" id="mode2"/></label>&nbsp;Old&nbsp;-&nbsp;New <br/><label><input type="radio" name="modes" id="mode3"/></label>&nbsp;New&nbsp;-&nbsp;Old </form> <div align="right"> <form id="categories" align="right"> Video<label>&nbsp;<input type="radio" name="thing" id="videoCheck"/></label><br/> Audio<label>&nbsp;<input type="radio" name="thing" id="audioCheck"/></label><br/> Photo<label>&nbsp;<input type="radio" name="thing" id="photoCheck"/></label><br/> Text<label>&nbsp;<input type="radio" name="thing" id="textCheck"/></label> </form> </div> </body>

A Few Things to Note...

  • I organize the JavaScript arrays with the youngest at the top and oldest at the bottom (with dates, it would look like this:

    oddDate = ['Oct. 1', 'Oct. 2', 'Oct. 3', 'Oct. 4', 'Oct. 5'];

  • The random mode is pseudo random, and is designed to display all array items once before repeating one.

  • The old-new and new-old modes move through the arrays from top-to-bottom and bottom-to-top, respectively.

  • Each mode and category saves its place when you change to a different one. For example, let's say you have the old-new mode on and are on item 3 of the text category. You switch to the photo category, run through the array a bit, then return to the text category. It will display the next item from where you left off previously - i.e. item 4. The same can be said for switching between the three modes; they are all independent of each other. This is something I would like to maintain.

  • One concern is that the generator and page will get slower and slower as I continue to add more items to the arrays, but I'm unsure if this is legitimate. In the live version, there are 500+ items in each array and more will be added over time.

\$\endgroup\$

    1 Answer 1

    Reset to default
    1
    +50
    \$\begingroup\$

    Overall Feedback

    This code seems to work acceptably, though the semantics of "old - new" seems reversed.

    The first bullet in the notes states:

    • I organize the JavaScript arrays with the youngest at the top and oldest at the bottom (with dates, it would look like this: oddDate = ['Oct. 1', 'Oct. 2', 'Oct. 3', 'Oct. 4', 'Oct. 5']; )

    Yet the third bullet states:

    • The old-new and new-old modes move through the arrays from top-to-bottom and bottom-to-top, respectively.

    That seems contradictory, since if the oldest was at the bottom then the old-new sorting should start at the end of the array.

    The JavaScript code seems very repetitive - especially in the function newThing() but also the variable names. I'd recommend some changes described later, after the review.

    Review

    Javascript

    Variable declarations

    The code uses some features specific to ECMAScript-2015 (A.K.A. ES-6) like the spread syntax. With ES6 variables it is advisable to use const and let to limit the scope of variables and avoid bugs. Note that const doesn't mean immutability, but rather that such a variable cannot be re-assigned.

    Global variables

    The code references DOM elements using the variables implicitly created from the id attributes - e.g.

    if(mode1.checked && (videoCheck.checked || audioCheck.checked || photoCheck.checked || textCheck.checked)) {

    If you wanted to unit test the JavaScript code then this might make it difficult.

    Input labels

    It appears that all of the radio inputs are contained by <label> inputs, which is good for accessibility (e.g. screenreaders, ability of user to click label to focus/activate the input) however the text next to each input is not within the label. It seems pointless to have a label if the input is the only thing in it.

    HTML

    Inline event handlers

    The code sets up event handlers within the HTML code:

    <button onclick="newThing()">New Thing</button>

    It is better to register event handlers within the JavaScript (e.g. using newButton.addEventListener for multiple reasons:

    1. The logic can be separated from the markup - if multiple teammates worked on the project then one could work on the JavaScript while the other could work on the HTML independently.
    2. Such handlers can pollute the global namespace which can lead to strange behavior.

    Aligning child elements

    The <div> elements have align="center" and align="right". That appears to be a deprecated HTML 4 attribute and no longer supported in HTML 5. This can be converted to CSS rules. Note that text-align applies to block containers1. To center inline elements like images, video, etc. the display would need to be set to block and margin set to auto.

    Multiple form elements

    There are two separate <form> elements. The two could be combined into a single form that contains all elements.

    Simplifying code

    The following changes, along with suggestions from the review points above can be used to dramatically decrease the length of the code:

    • put options into an object that can contain the current index, and make the object a property of an object where the property name (i.e. key) is the type of thing - e.g.

       const options = { video: { options: [ //VIDEO ARRAY 'video1', 'video2', 'video3', 'video4', 'video5', ], currentIndex: -1 }, audio: { options: [ //AUDIO ARRAY 'audio1', 'audio2', 'audio3', 'audio4', 'audio5', ], currentIndex: -1 }, photo: { options: [ //PHOTO ARRAY 'photo1', 'photo2', 'photo3', 'photo4', 'photo5', ], currentIndex: -1 }, text: { options: [ //TEXT ARRAY 'text1', 'text2', 'text3', 'text4', 'text5', ], currentIndex: -1 }, }

    • instead of using id attributes for the radio buttons, just give them value attributes

    • reference the form elements via document.forms.elements

    • determine which radio buttons are selected via RadioNodeList.value

    • use an <output> element for the display of the thing.

    • use a class to encapsulate the indexes, along with methods to get an item based on the mode

    class ThingList { constructor(items) { this.items = items; this.randomUnused = [...items]; this.forwardIndex = 0; this.reverseIndex = items.length - 1; } forwardItem() { return this.items[this.forwardIndex++ % (this.items.length)]; } randomItem() { if (!this.randomUnused.length) { this.randomUnused.push(...this.items); } const index = Math.floor(Math.random() * this.randomUnused.length) return this.randomUnused.splice(index, 1); } reverseItem() { if (this.reverseIndex < 0) { this.reverseIndex = this.items.length - 1; } return this.items[this.reverseIndex--]; } } const options = { video: new ThingList([ //VIDEO ARRAY 'video1', 'video2', 'video3', 'video4', 'video5', ]), audio: new ThingList([ //AUDIO ARRAY 'audio1', 'audio2', 'audio3', 'audio4', 'audio5', ]), photo: new ThingList([ //PHOTO ARRAY 'photo1', 'photo2', 'photo3', 'photo4', 'photo5', ]), text: new ThingList([ //TEXT ARRAY 'text1', 'text2', 'text3', 'text4', 'text5', ]) } const output = document.getElementsByTagName('output')[0]; //GENERATOR FUNCTION function newThing() { if (!(document.forms.thingSelection.type.value in options)) { return false; } const list = options[document.forms.thingSelection.type.value]; const method = document.forms.thingSelection.mode.value + 'Item'; const item = list[method](); output.innerHTML = item; } document.getElementsByTagName('button')[0].addEventListener('click', newThing)
    .center { text-align: center; } .right { text-align: right; }
    <div class="center"><output></output></div> <div class="center"> <button>New Thing</button> </div> <form name="thingSelection"> <label><input type="radio" name="mode" value="random" />&nbsp;Random</label> <br /><label><input type="radio" name="mode" value="forward" />&nbsp;Old&nbsp;-&nbsp;New</label> <br /><label><input type="radio" name="mode" value="reverse" />&nbsp;New&nbsp;-&nbsp;Old</label> <div class="right"> <label>Video&nbsp;<input type="radio" name="type" value="video" /></label><br /> <label>Audio&nbsp;<input type="radio" name="type" value="audio" /></label><br /> <label>Photo&nbsp;<input type="radio" name="type" value="photo" /></label><br /> <label>Text&nbsp;<input type="radio" name="type" value="text" /></label> </div> </form>

    \$\endgroup\$
    2
    • \$\begingroup\$Thank you for taking the time to simplify and correct my code. When testing your snippet, I noticed that switching between the four categories while in any of the three modes maintains the place in each category. However, the three modes no longer appear to be independent of each other, meaning running through the old-new mode affects the order of the new-old mode. Additionally, the random mode is no longer pseudo random and displays repeats before showing all array items. Please adjust your post to account for these things.\$\endgroup\$
      – user231248
      CommentedOct 8, 2020 at 15:50
    • 1
      \$\begingroup\$okay - I have updated the snippet example to match the original requirements.\$\endgroup\$
      – Sᴀᴍ Onᴇᴌᴀ♦
      CommentedOct 12, 2020 at 5:36