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.

At the end of the immediately-invoked function expression, there is an if statement that checks the screen width and loads in some resources. I'm curious if it is best practice to include this logic inside my IIFE, or outside. Both seem to function the same as far as I can tell, though I'm curious if there is preferred/best way this should be done.

Also of interest to me, inside the IIFE I have a if/if else/else statement. I'm curious if the last else, with the return in it is necessary. Does it offer any benefits or should I remove it.

Feedback is much appreciated!

Below is the code I'm working with:

(function(w){
    w.loader = function(filetype, filename, mediaquery){

      var head = document.getElementsByTagName("head")[0],
          stylesheet = document.createElement('link'),
          script = document.createElement('script'),
          filetype = filetype.toLowerCase(); // normalize filetype data for use in conditions

      // If file type is set to CSS, create dynamic stylesheet link element
      if (filetype === 'css'){
        stylesheet.type = 'text/css';
        stylesheet.rel = 'stylesheet';
        stylesheet.href =  (filename + '.css');
        stylesheet.media = (mediaquery) ? mediaquery : 'screen'; // Use screen for media unless otherwise specified
        // If a file name has been specified, append dynamic stylesheet link element
        if (filename.length){
          head.appendChild(stylesheet);
        }
      }
      // If file type is set to JS, create dynamic script element link
      else if(filetype === 'js'){
        script.type = 'text/javascript';
        script.src = (filename + '.js');
        // If a file name has been specified, append dynamic script element
        if (filename.length){
          head.appendChild(script);
        }
      }
      else{
        return;
      }

    };

    if(Math.max(screen.width,window.outerWidth) > 600 ){
      alert("Large Screen");
      loader('CSS', 'large-screen');
      loader('JS', 'large-screen');
    }

  }(this));

  /*
  if(Math.max(screen.width,window.outerWidth) > 600 ){
    alert("Large Screen");
    loader('CSS', 'large-screen');
    loader('JS', 'large-screen');
  }
  */
share|improve this question
add comment

2 Answers 2

up vote 1 down vote accepted

jfriend00 is 100% right about the first part of your question.

To answer the second part of your question the return serves no purpose in the code your presented.

A better approach would actually be to throw an exception for the unhandled case. This way, if the user of this function tries to include something that the loader cannot handle they will know about it right away instead of having it fail silently.

share|improve this answer
add comment

In this case, it makes no difference at all. This IIFE really has no reason for even being there because all it contains is a single function. Your code does use any of the usual benefits of an IIFE (like a local and protected scope) or capturing the arguments in a closure.

So the additional code that doesn't have any local variables of its own and doesn't use the passed in argument does not benefit at all from being in the IIFE, but there is no harm in putting it in the IIFE either.

share|improve this answer
add comment

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.