26
\$\begingroup\$

I have been working with HTML5 and CSS3 for a long time now, but have avoided JavaScript because of my belief that it's most frequently used unnecessarily while having a tendency to be poorly written. So I held off until I needed to learn it to use something really cool and useful. The project that sold me was WebTorrent.

I do have programming experience (the most syntactically similar languages I know are PHP and Java), so I have some idea of standards, but I don't know best practices for JavaScript. The code certainly works and will be polished later on (it's more of a tech demo than anything), so I'd prefer critique on syntax and general methods more than function, unless I'm doing something fundamentally wrong.

I didn't go into this blind, as tempting as it was. My intro was MDN's A Re-Introduction to JavaScript, which I found very helpful. All this code is in the page's <head>, so if I should be moving it somewhere else or calling it on an element, please let me know.

<script src="https://cdn.jsdelivr.net/webtorrent/latest/webtorrent.min.js"></script> <script> var client = new WebTorrent(); // Torrent ID var torrentId = 'redacted WebTorrent magnet link - contains 2 audio files, 1 video file, 1 image'; client.add(torrentId, function ontorrent(torrent){ /* Gameplan: Load Content (done by this point) Case empty torrent Case no playable media warn user break Case multiple playable media ask which file(s) to play Case one playable media play media break */ // Compatible Media Formats var MEDIA_EXT = ['mp4', 'm4v', 'm4v', 'webm', 'm4a', 'mp3', 'wav', 'aac', 'ogg', 'oga']; function getExt(name){ // Not own work: http://stackoverflow.com/questions/190852/how-can-i-get-file-extensions-with-javascript return name.substr((~-name.lastIndexOf(".") >>> 0) + 2); } // Status logger var logElement = document.getElementById('status'); function pStatus(msg){ logElement.innerHTML += "<br/>" + msg }; var numFiles = torrent.files.length; // Check for empty torrent if(numFiles == 0){ pStatus("No files found! Cannot render media.") return; } // Find all playable media var playable = new Array(); torrent.files.forEach(function(file){ pStatus(" - Found file: " + file.name); if(MEDIA_EXT.indexOf(getExt(file.name.toLowerCase())) > 0){ playable.push(file); } }); playable.forEach(function(file){ pStatus("File " + file.name + " is usable."); }); if(playable.length === 1){ playable[0].appendTo(document.getElementById('target')); }else{ do{ var index = window.prompt("Multiple files found. Please choose the index you would like to play."); }while(playable[index] == undefined); var file = playable[index]; pStatus("Choosing index " + index + ": " + file.name + "..."); file.appendTo(document.getElementById('target')); pStatus("Now loading " + file.name); document.title = file.name; } }); </script>
\$\endgroup\$
0

3 Answers 3

Reset to default
18
\$\begingroup\$

After reviewing, I came up with these observations:

  • You really should not do inline scripts, put this in a separate .js file
  • No meaningless comments, you should remove //Torrent ID
  • Variables should be in lowerCamelCase MEDIA_EXT -> mediaExtensions, or even validFileExtensions
  • I like the attribution to SO, though I tend to put that before function so that it does not distract. Generally, a one line function that is only used once does not make sense, but because the style is so different I would leave it be
  • Write functions consistently, don't make pStatus a one-liner
  • You only need numFiles once, to exit, I would not use a separate var for this
  • I don't like the gameplan comment, it does not truly belong there, perhaps better in a design doc
  • new Array(); -> Stylistically better to use var playableMedia = [];
  • I would investigate [].filter, if you are going functional, you might as well go with the right function
  • You already know all the files that are playable, I would avoid that second loop if you tend to process a lot of files
  • You can just do 'prompt' instead of 'window.prompt', ideally you never use prompt ;)
  • Ideally you have 1 var declaration on top instead of all over the place
  • Ideally use one form of quotes, ' or ", I tend to prefer '
  • Handling 1 file is inconsistent with handling multiple files, seems wrong
  • Use JsHint.com
    • You have missing semicolons
    • It will tell you to use if(numFiles === 0) though I would use if(!numFiles)

I would refactor the code thusly:

//No script tags, I assume this in a separate js now var client = new WebTorrent(), torrentId = 'redacted WebTorrent magnet link - contains 2 audio files, 1 video file, 1 image'; client.add(torrentId, function ontorrent(torrent) { // Compatible Media Formats var validFileExtensions = ["mp4", "m4v", "m4v", "webm", "m4a", "mp3", "wav", "aac", "ogg", "oga"], playable = [], usableLog = "", index, file; // Not own work: http://stackoverflow.com/questions/190852/how-can-i-get-file-extensions-with-javascript function getExt(name) { return name.substr((~-name.lastIndexOf(".") >>> 0) + 2); } function updateStatus(msg) { document.getElementById('status').innerHTML += "<br/>" + msg; } // Check for empty torrent if (!torrent.files.length) { updateStatus("No files found! Cannot render media."); return; } // Find all playable media playable = torrent.files.filter(function(file) { updateStatus(" - Found file: " + file.name); if (validFileExtensions.indexOf(getExt(file.name.toLowerCase())) > 0) { usableLog += "<br/>File " + file.name + " is usable."; return true; } return false; }); updateStatus(usableLog || "No files were usable"); //What file should we play if (playable.length === 1) { index = 0; } else { do { index = prompt("Multiple files found. Please choose the index you would like to play."); } while (!playable[index]); } //Inform the user and play the file file = playable[index]; updateStatus("Choosing index " + index + ": " + file.name + "..."); file.appendTo(document.getElementById('target')); updateStatus("Now loading " + file.name); document.title = file.name; });
\$\endgroup\$
5
  • 2
    \$\begingroup\$ That is a thing of beauty. Thanks for all the help! I'll be sure to keep all of this in mind when revising my code. A simple question, but where should this code be included? Formerly I had it in a block in my <head>. Should it be included in the head or run in the body? \$\endgroup\$ Commented Feb 24, 2016 at 17:06
  • 2
    \$\begingroup\$ @ndm13 have something like <head><script src="torrent.js"></script></head> \$\endgroup\$ Commented Feb 24, 2016 at 18:22
  • 2
    \$\begingroup\$ What cat says, also do not hesitate to mark my answer as the selected one ;) \$\endgroup\$ Commented Feb 25, 2016 at 13:57
  • \$\begingroup\$ @konijn Consider it done! Thanks for the help. \$\endgroup\$ Commented Feb 25, 2016 at 16:12
  • \$\begingroup\$ SHOUT_CASE is perfectly fine if it is a constant value. \$\endgroup\$ Commented Mar 10, 2016 at 7:42
8
\$\begingroup\$

  1. Use good names instead of comments.

    Instead of writing:

    // Compatible Media Formats var MEDIA_EXT = ['mp4', 'm4v', 'm4v', 'webm', 'm4a', 'mp3', 'wav', 'aac', 'ogg', 'oga'];

    it is more readable to write:

    var compatibleMediaExt = ['mp4' ....

  2. Encapsulate more logic in functions.

    Instead of writing:

    if(MEDIA_EXT.indexOf(getExt(file.name.toLowerCase())) > 0){ playable.push(file); }

    just rename getExt to isValidFile and insert contents of getExt, comparing to 0, converting to lower.

  3. Describe if statements with variables

    if(playable.length === 1)

    does not tell much about what the statement means.

    If you write:

    var oneFileFound = (playable.length === 1)

    and then 

    if (oneFileFound)

    everything will be more readable.

  4. Choose better names for variables.

    playable is not a good choice for array of files. files would be much better.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Points one and two are very good, but I'm not sure about three. I personally find it to be a waste of code to make a variable to use once, unless it drastically improves readability (i.e. you don't write one 500-character line of garbage to do what would be more easily understood in four or five). \$\endgroup\$ Commented Feb 24, 2016 at 13:43
  • \$\begingroup\$ The reason I didn't name playable files is because there is already a files object in torrent, and I thought it would be more syntactically confusing to use an existing name than to use a less accurate name. But this may be coming from Java, where it would be List<File> playable. \$\endgroup\$ Commented Feb 24, 2016 at 13:43
1
\$\begingroup\$

Something that wasn't suggested yet is to make your code into a closure, so you don't pollute the global namespace:

(() => { var client = new WebTorrent(); ... })();
\$\endgroup\$
2
  • \$\begingroup\$ What advantages does this have? \$\endgroup\$ Commented Feb 25, 2016 at 22:47
  • \$\begingroup\$ As someone more experienced in JS (for future readers), a closure means that the variable names you use won't leak out into other scripts. So if you use the variable client and some other script uses the same variable, you won't get clobbered/clobber them. Although I prefer the slightly more verbose, but syntactically clearer, (function(){ ... })();. \$\endgroup\$ Commented Jun 19, 2018 at 23:55

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.