patternjavascriptModerate
WebTorrent player
Viewed 0 times
playerwebtorrentstackoverflow
Problem
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 `
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){ logEl
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 `
, so if I should be moving it somewhere else or calling it on an element, please let me know.
``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){ logEl
Solution
After reviewing, I came up with these observations:
I would refactor the code thusly:
- 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
functionso 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 usevar 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
vardeclaration 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 useif(!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 += "" + 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 += "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;
});Code Snippets
//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;
});Context
StackExchange Code Review Q#120966, answer score: 18
Revisions (0)
No revisions yet.