5
\$\begingroup\$

I am a Javascript/JQuery beginner working on my first ever project. Everything in this program is working as it should and I am happy with its performance. I have pieced it together from a variety of sources and lots of trial and error.

Because this is so new to me I wanted some experienced opinions on the way I have used the various Javascript/JQuery components in my project. The code I have included here has been edited and simplified for clarity but in its entirety it is a working program so I'm not necessarily looking to have it rewritten but more looking for feedback on what I have done to see where I can improve my programming skills.

In particular am I following good programming protocols with my structure and layout of various elements and functions etc.? Should I change, combine or separate components etc. to simplify coding whilst still allowing me to increase functionality should I want to extend the program further? Are there better ways through HTML or otherwise to more efficiently achieve the outcomes I have noted in comments?

<div class="sandbox">
   <video id="videoElement" ><source src="video.mp4" > </video>
  <div id="start-page"> <button on="" onclick="playVid();">Start video</button> </div>
  <div class="respond"> <p>Click video to respond</p> </div>
  <div id="result-page"> <h2 id="result"> </h2> </div>
</div>


<script>
// when page loads hide the respond and result-page divs
 $(function(){
    $(".respond, #result-page").hide();
  });

// play video; hide start-page div; show respond div
var myVid = document.getElementById('videoElement');
function playVid() { 
    videoElement.play();
    $("#start-page").hide(); $(".respond").show();
  }

// when ‘respond’ div is clicked the ‘respond’ div is disabled
// “response recorded” message shows & response time compared to settings
  $('.respond').on('click', function() {
  $('.respond').off('click');
  $('.respond').html('<p>response recorded</p>');
    var mycurrentTime = myVid.currentTime;
    var result;
if (mycurrentTime < 10) {
      result = "under 10" ;
    } else {
      result = "over 10" ;
  }

// add ‘result’ text to result-page div
document.getElementById("result").innerHTML = result;
    }
);

// at end of video show result-page div
document.getElementById('videoElement').addEventListener('ended',myHandler,false);
function myHandler(e) {
        $("#result-page").show();
  }
</script>
\$\endgroup\$
0
2
\$\begingroup\$

This script mixes jQuery DOM lookups (e.g. $(".respond")) with native DOM lookups (e.g. document.getElementById('videoElement')). It is best to use only one style to avoid confusion (e.g. for anyone reading your code, including your future self). If you are going to load jQuery on the page then you might as well use it. Otherwise if you don't need it then it could reduce the page load time slightly if it is removed. You might find this article interesting.

Indentation is inconsistent, though maybe that was a mistake during insertion of the code into your post.

The first line in playVid() references videoElement which looks up the element by id attribute implicitly.

function playVid() { 
    videoElement.play();

This can also confuse readers of your code. Because myVid points to that same element, that variable could be used instead.

function playVid() { 
    myVid.play();
\$\endgroup\$

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.