Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have this code to check a couple conditions before allowing a play button to become visible. Is there anything to do to consolidate this code, possibly shrink it down some?

var isNodeWebkit = (typeof process == "object"); // check if using node app
var downloadURL = 'https://www.microsoft.com/getsilverlight/get-started/install/'; // setup download URL variable
var isEncoding = false; // setup isEncoding variable

if (!Silverlight.isInstalled("1.0")) { // check if silverlight is installed
    $('.playbtn').hide();
    $('.videoposter').append('<button class="installbtn" />');
    if (isNodeWebkit) {
        $('.installbtn').on('click', function () { // open silverlight download externally
            require('nw.gui').Shell.openExternal(downloadURL);
            var win = require('nw.gui').Window.get();
            win.close(); return false;
        });
    } else {
        $('.installbtn').click(function () { // open silverlight download in new tab
            window.open(downloadURL, '_blank');
        });
        $(window).focus(function () {
            if (Silverlight.isInstalled("1.0")) {
                $('.installbtn').remove();
                $('.playbtn').show(); checkEncodingFunc();
            } else if (!Silverlight.isInstalled("1.0")) {
                $('.installbtn').remove(); $('.videoposter').append('<button class="installbtn" />');
                $('.playbtn').hide(); $('.encodingbtn').hide();
            }; ;
        });
    };
} else if (Silverlight.isInstalled("1.0")) {
    $(window).focus(function () { // after installing show play button
        if (!Silverlight.isInstalled("1.0")) {
            $('.playbtn').hide(); $('.installbtn').remove(); $('.encodingbtn').remove();
            $('.videoposter').append('<button class="installbtn" />');
            $('.installbtn').click(function () { // open silverlight download in new tab
                window.open(downloadURL, '_blank');
            });
        } else {
            $('.installbtn').remove();
            $('.playbtn').show();
        };
        if (Silverlight.isInstalled("1.0") && isEncoding == true) { checkEncodingFunc(); };
    });
};

var encodeTimer = setInterval(function () { if (isEncoding == true) { checkEncodingFunc(); }; }, 60000);
if (Silverlight.isInstalled("1.0")) { checkEncodingFunc(); };

function checkEncodingFunc() { // check if video is encoding
    $.ajax({
        url: 'video/',
        statusCode: {
            200: function () { // if video is found display play button
                if (isEncoding == true) {
                    $('.encodingbtn').remove();
                    $('.playbtn').show();
                    isEncoding = false;
                };
            },
            204: function () { // if video is encoding display encoding button
                if (Silverlight.isInstalled("1.0")) {
                    isEncoding = true;
                    $('.playbtn').hide(); $('.encodingbtn').remove();
                    $('.videoposter').append('<button class="encodingbtn" />');
                };
            }
        }
    });
};
share|improve this question

2 Answers 2

up vote 15 down vote accepted

Just a few more minor things on top of jt0dd's excellent review.


Anonymous functions in JavaScript are awesome, but your program will be more readable if you give the main functions names and declare in advance. That way the main part of your program will become something like this:

if (!haveSilverlight) {
    $('.playbtn').hide();
    $('.videoposter').append('<button class="installbtn" />');
    if (isNodeWebkit) {
        $('.installbtn').on('click', openExternalSilverlightDownloader);
    } else {
        $('.installbtn').on('click', openSilverlightDownloader);
        $(window).on('focus', setupInstallBtn);
    }
} else {
    $(window).on('focus', setupPlayBtn);
}

Easier to see the overall logic, no?


Don't write if statements this way:

if (something) {
     // ...
} else if (!something) {
     // ...
}

That's what else is for:

if (something) {
     // ...
} else {
     // ...
}

You don't need to evaluate boolean variables like this:

if (isEncoding == true) {
    // ...
}

You can do simply:

if (isEncoding) {
    // ...
}

On this line, you don't need the brackets, and I think the code would be more readable with the comment on the line before the assignment.

var isNodeWebkit = (typeof process == "object"); // check if using node app

Better this way:

// check if using node app
var isNodeWebkit = typeof process == "object";

Especially with long lines like this, definitely don't put comment after the statement, as it forces me to scroll right, which is not convenient. Even better, the a comment saying // setup download URL variable for a variable assignment is pointless, so it's better to just drop it.

var downloadURL = 'https://www.microsoft.com/getsilverlight/get-started/install/'; // setup download URL variable
var isEncoding = false; // setup isEncoding variable

These lines should have been just:

var downloadURL = 'https://www.microsoft.com/getsilverlight/get-started/install/';
var isEncoding = false;

Without comments, assignments are self-explanatory.

share|improve this answer
2  
Very good. Teamwork review +1! –  CuriousProgrammer Jul 23 at 7:11
2  
I'd +2 if I could for the last note. Code that scrolls off-screen drives my OCD wild. –  CuriousProgrammer Jul 23 at 7:22

Ok first of all.. Silverlight Rant.

Silverlight was a flop far less successful than Microsoft expected it to be. HTML5 + WebGL, on the other hand, is the future of handling anything that anyone ever thought Silverlight might do. If you want to maintain highly supported, future-proof practices, start doing some WebGL research. It's INCREDIBLE. Silverlight is another one of Microsoft's attempts to avoid standardization and control the web with their own tech. They've failed every single time so far.

Lesson: Avoid Microsoft's proprietary stuff when possible.

Now, for a Review on Practices..

As far as your code formatting and practices go, I'm happy with what I see. There are a few improvements you can make to make your code a bit simpler:

You use if(!Silverlight.isInstalled("1.0")) // do stuff all over the place. Try this instead:

var silverlightCheck = Silverlight.isInstalled("1.0");
if (silverlightcheck) // do stuff

I saw these on the same line: $('.playbtn').show(); checkEncodingFunc(); Nu uh. Nope. Use:

$('.playbtn').show(); 
checkEncodingFunc();

And one more simplification:

You can make selectors that you often repeat, such as $('.installbtn') into variables.

 var installButton = $('.installbtn'); //

That way, you type that selector a little quicker via the variable.

I've touched on some simplification. Someone might review this to contribute improvement to the structure, but this is not bad code. +1 for you!

share|improve this answer
1  
Yeah silverlight is so bad its used by almost every major streaming service ever –  Ashley Medway Jul 23 at 5:51
1  
@AshleyMedway That's ironic. Have a read. Silverlight is dying; All of the major streaming services ever are running away from it fast. –  CuriousProgrammer Jul 23 at 6:23
1  
thats not irony... –  Ashley Medway Jul 23 at 6:25
2  
IE11 is the most recent version of IE and, at least on my machine, scores better on the ACID3 test than Chrome. It's still nowhere near as good as Chrome or Firefox (mainly due to the lack of 3rd party plugin support), but it's not a deadly sin to still have to use it. It's the older versions, like IE8 thorugh 10, that should be a capital offense. And a lot of companies have legacy apps designed for older browsers that don't work all that well with more recent browsers. –  Nate Kerkhofs Jul 23 at 9:36
3  
@jt0dd for IE support, I would say that the oldest version to support would be IE9. it's the first one that started to support the actual standards. If your customer wants you to support older versions of IE, try your best to make him pay extra for it. –  Nate Kerkhofs Jul 23 at 9:51

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.