Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I am new to javascript and want to make sure I am on the right track when playing a sound. Is there anything I should not be doing better or not be doing at all. Below is a simple function. 

<script language="javascript" type="text/javascript">
  function playSound(soundfile) {
    if (navigator.userAgent.indexOf('Firefox') != -1) {
      document.getElementById("dummy").innerHTML = "<embed src=\"" + soundfile + "\" hidden=\"true\" autostart=\"true\" loop=\"false\" type=\"application/x-mplayer2\" />";
    } else {
      document.getElementById("dummy").innerHTML = "<embed src=\"" + soundfile + "\" hidden=\"true\" autostart=\"true\" loop=\"false\" />";
    }
  }
</script>
</head>
<body>
<span id="dummy"></span>

  <a href="#" onclick="playSound('doorbell.wav');">Click here to hear a sound</a>
</body>

I am not sure whether I should be using the audio tag or not. I appreciate any feedback

share|improve this question
Why do you use embed instead of audio? – unor Feb 12 at 9:03
Consider building the element using something like jQuery instead of doing a concatenation of strings which is hard to read and difficult to maintain. – Quentin Pradet Feb 12 at 9:17

2 Answers

up vote 1 down vote accepted

Don't duplicate sections of code! In particular, avoid putting 'execution' code in these simple conditionals, instead use them to build a shared executor:

function playSound(soundfile) {
  var type_attr = '';
  if (navigator.userAgent.indexOf('Firefox') != -1) {
    type_attr = "type=\"application/x-mplayer2\"";
  }
  document.getElementById("dummy").innerHTML = "<embed src=\"" + soundfile + "\" hidden=\"true\" autostart=\"true\" loop=\"false\" " + type_attr + " />";
}
share|improve this answer
up vote 1 down vote

Regarding the HTML:

script element

For JavaScript, you can simply use <script> without the type attribute:

If the language is not that described by "text/javascript", then the type attribute must be present

The language attribute is obsolete:

Authors should not specify a language attribute on a script element.

a element

I'd say a real button should be used here, not a link:

  • button element with button type: <button type="button" onclick="…">Click here to hear a sound</button>
  • input element with button type: <input type="button" onclick="…" value="Click here to hear a sound" />
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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