# WebTorrent player

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';

/*
Gameplan:
Load Content (done by this point)
Case empty torrent
Case no playable media
warn user
break
Case multiple playable media
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'));
document.title = file.name;
}
});
</script>


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';

// 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);
}

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'));
document.title = file.name;
});

• 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? Feb 24, 2016 at 17:06
• @ndm13 have something like <head><script src="torrent.js"></script></head>
– cat
Feb 24, 2016 at 18:22
• What cat says, also do not hesitate to mark my answer as the selected one ;) Feb 25, 2016 at 13:57
• @konijn Consider it done! Thanks for the help. Feb 25, 2016 at 16:12
• SHOUT_CASE is perfectly fine if it is a constant value.
– Dan
Mar 10, 2016 at 7:42

// 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.

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)


4. Choose better names for variables.

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

• 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). Feb 24, 2016 at 13:43
• 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. Feb 24, 2016 at 13:43

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();
...
})();

• What advantages does this have? Feb 25, 2016 at 22:47
• 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(){ ... })();. Jun 19, 2018 at 23:55