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\$
    – ndm13
    Commented Feb 24, 2016 at 17:06
  • 2
    \$\begingroup\$ @ndm13 have something like <head><script src="torrent.js"></script></head> \$\endgroup\$
    – cat
    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\$
    – konijn
    Commented Feb 25, 2016 at 13:57
  • \$\begingroup\$ @konijn Consider it done! Thanks for the help. \$\endgroup\$
    – ndm13
    Commented Feb 25, 2016 at 16:12
  • \$\begingroup\$ SHOUT_CASE is perfectly fine if it is a constant value. \$\endgroup\$
    – Dan
    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\$
    – ndm13
    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\$
    – ndm13
    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\$
    – ndm13
    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\$
    – ndm13
    Commented Jun 19, 2018 at 23:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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