1
\$\begingroup\$

I've produced some JavaScript code for a Video Player, but my experience in coding is with C# and the differences between the languages and my lack of experience has made me wonder I'm not following the correct guidelines.

I'm just wondering if anyone doing a once-over would have any good criticisms and pointers they could offer. I've commented the code to explain my thinking.

let activateDisc = false;
let playingDisc = false;
let animatingDisc = false;

let volumeOn = true;

//Get the elements on the page.
let page = document.querySelector("#page");
let cover = document.querySelector("#videocover");
let disc = document.querySelector("#disc");

let playIcon = document.querySelector("#playicon");
let pauseIcon = document.querySelector("#pauseicon");

let volumeOnIcon = document.querySelector("#volumeon");
let volumeOffIcon = document.querySelector("#volumeoff");

let videoContainer = document.querySelector("#videocontainer");
let video = document.querySelector("video");
let volumeButton = document.querySelector("#volume");
let videoSlider = document.querySelector("#videoslider");

//Set it up so clicking the disc will start the video and the video ending will eject the disc.
disc.onclick = function() {insertDisc();}
video.onended = function() {ejectDisc();}

//Attach each element to their equivalent function.
document.querySelector("#restart").onclick = function() {resetVideo();}
document.querySelector("#rewind").onclick = function() {rewindVideo();}
document.querySelector("#skip").onclick = function() {skipVideo();}
document.querySelector("#end").onclick = function() {ejectDisc();}

//For play if the disc isn't in, we want to insert it.
document.querySelector("#play").onclick = function() {
    if (!activateDisc) {
        window.scrollTo(0, 0);
        insertDisc();
    }
    else {
        //Then we want to check either pause or play the video.
        if (!playingDisc){
            playVideo();
        }
        else {
            pauseVideo();
        }
    }
}

//The volume button checks if the volume is on and calls the correct method.
volumeButton.onclick = function() {
    if (volumeOn){
        turnVolumeOff();
    }
    else {
        turnVolumeOn();
    }
}

//Every time the current playing position of the video updates we'll set the slider accordingly.
video.ontimeupdate = function() {
    //The slide is set as the current time in the video over the total length of the video multiplied by the max slider value.
    videoSlider.value = video.currentTime / video.duration * videoSlider.max;
}

//When the mouse is over the video we want to show the video slider and volume button.
videoContainer.addEventListener('mouseover', function() {
    videoSlider.style.opacity = 1;
    volumeButton.style.opacity = 1;
},false);

//When the mouse leaves the video we want to hide the video slider and volume button.
videoContainer.addEventListener('mouseout', function() {
    videoSlider.style.opacity = 0;
    volumeButton.style.opacity = 0;
},false);

//When the slider value is changed we want to set the video time to the slider value.
videoSlider.addEventListener("input", function() {
    //The current time in the video is set as the slider value over the max slider value multiplied by the total length of the video.
    video.currentTime = videoSlider.value / videoSlider.max * video.duration;
}, false);

//Inserting the disc.
function insertDisc() {
    //If the disc is already in eject it.
    if (!activateDisc){
        //If the disc is already animating we return and do nothing.
        if (animatingDisc) return;
        animatingDisc = true;
        activateDisc = true;
        disc.style.animation = "playdisc 1s linear";
        //Wait for the disc to be fully inserted and play the video.
        setTimeout(() => {
            playDiscAnimation();
            scrollToWithOffset(disc, -80);
            hideElement(cover);
            video.style.animation = "flicker 500ms infinite alternate-reverse";
            playVideo();
        }, 1000);
    }
    else {
        ejectDisc();
    }
}

//Ejecting the disc.
function ejectDisc() {
    //If the disc is in we can eject it.
    if (activateDisc){
        if (animatingDisc) return;
        animatingDisc = true;
        //Reset the video back to its original state and eject it.
        pauseVideo();
        resetVideo();
        turnVolumeOn();
        video.style.animation = "";
        showElement(cover);
        ejectDiscAnimation();
        window.scrollTo(0, 0);
        //Replay the disc bouncing animation.
        setTimeout(() => {
            disc.style.animation = "bounce 300ms infinite alternate-reverse";
            activateDisc = false;
        }, 1000);
    }
}

//Play the video.
function playVideo() {
    if (!activateDisc) return;
    playTvAnimations();
    playingDisc = true;
    video.play();
}

//Pause the video.
function pauseVideo() {
    if (!activateDisc) return;
    pauseTvAnimations();
    playingDisc = false;
    video.pause();
}

//Reset the video back to zero.
function resetVideo() {
    if (!activateDisc) return;
    video.currentTime = 0;
}

//Rewind the video 5 milliseconds.
function rewindVideo() {
    if (!activateDisc) return;
    video.currentTime -= 5;
}

//Skip 5 milliseconds in the video.
function skipVideo() {
    if (!activateDisc) return;
    video.currentTime += 5;
}

//Turn the volume on.
function turnVolumeOn() {
    video.volume = 1;
    hideElement(volumeOffIcon);
    showElement(volumeOnIcon);
    volumeOn = true;
}

//Turn the volume off.
function turnVolumeOff() {
    video.volume = 0;
    hideElement(volumeOnIcon);
    showElement(volumeOffIcon);
    volumeOn = false;
}

//Play the disc insertion and playing animations.
function playDiscAnimation() {
    disc.style.animation = "spindisc 300ms infinite linear";
    page.style.backgroundColor = "#333030";
    animatingDisc = false;
}

//Play the disc ejection animation.
function ejectDiscAnimation() {
    disc.style.animation = "playdisc 1s reverse";
    disc.style.animationPlayState = "running";
    page.style.backgroundColor = "#b8c9dd";
    animatingDisc = false;
}

//Play the tv animation.
function playTvAnimations() {
    disc.style.animationPlayState = "running";
    hideElement(playIcon);
    showElement(pauseIcon);
}

//Pause the tv animations.
function pauseTvAnimations() {
    disc.style.animationPlayState = "paused";
    hideElement(pauseIcon);
    showElement(playIcon);
}

//Show an element by setting its display to block.
function showElement(element) {
    element.style.display = "block";
}

//Hide an element by setting its display to none.
function hideElement(element) {
    element.style.display = "none";
}

//Scroll towards an element with a Y offset.
function scrollToWithOffset(element, yOffset = 0){
    var scrollY = element.getBoundingClientRect().top + window.pageYOffset + yOffset;
    window.scrollTo(0, scrollY);
}
\$\endgroup\$
4
\$\begingroup\$

First of all, congrats on readable code. It's not at all hard to follow what's happening, and seems like it would be easy for someone else to comprehend.

I have a few suggestions for you. Mostly these will just make your JavaScript more idiomatic, but there are some quality improvements possible too.

Let vs Const vs Var

In modern JavaScript, you probably don't want to use var. It makes your variable declarations subject to hoisting, which can create unpredictable behavior by making the variable name available in places you don't expect it to be, especially when coming from another language.

My suggestion is to always use const, unless you know you're going to be reassigning the variable, then use let. This implies that the variable name within a block will always refer to the same object/value in the case of const, or that you might reassign it, in the case of let. Either choice is a signal to the next person reading the code and can help with maintainability. Also, if you're linting your project, your code quality tools can warn you if you try to unintentionally reassign a const. More here.

Most of your uses of let should be const. Same with what appears to be your only use of var.

Single Line If-Then

I'm going to suggest you don't do this:

if (!activateDisc) return;

People think it's short and nifty, but it's a case where shortcuts can cause problems later. Use block scoping for the body of your if-then statements to avoid the hazards of automatic semicolon insertion, like so:

if (!activateDisc) {
    return;
}

Name Your Event Handlers

In the browser, you want to be able to remove event handlers if they're no longer needed. If you don't, things can happen that you don't expect. For that reason, and because it simply isn't necessary, you shouldn't wrap your event handler functions in anonymous functions. You should probably use addEventListener instead of assigning to onclick, so you can assign more than one event handler to the click event if required. Using a named function instead of an anonymous one will give you a reference so you can remove the handler later.

EG, this:

document.querySelector("#restart").onclick = function() {resetVideo();}

should be:

document.querySelector("#restart").addEventListener("click", resetVideo);

That way, you can later do this if you need to:

document.querySelector("#restart").removeEventListener("click", resetVideo);
\$\endgroup\$
2
  • \$\begingroup\$ To give the beginner a bit of orientation, you should be more specific about what can cause problems later and things can happen mean. \$\endgroup\$ – Roland Illig Dec 5 '20 at 21:56
  • \$\begingroup\$ This helps! Event handling is something I wasn't aware of, makes sense to use it. I'm sure it'll help when making websites for mobile and pc as I can switch up which of the events call each method, as I'm sure mouseover isn't available on mobile. As for the rest I'll try and apply your guidelines. Cheers! \$\endgroup\$ – Seb Heron Dec 6 '20 at 12:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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