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.

The following is a little JavaScript project to display quotes on a web page. Since I want to use it in a number of different web sites, I read up on good practices for making portable JavaScript code, e.g.:

  • use no global variables
  • use namespaces
  • make it easy to plugin
  • use default values which can be overridden

For those of you who have experience writing JavaScript libraries and portable JavaScript code, what could be improved on this code to

  1. make it more portable?
  2. avoid any unforeseen problems or conflicts?
  3. improve the naming conventions, etc.?

index.htm:

<!DOCTYPE html>
<html lang="en">
    <head>
        <meta charset="utf-8" />
        <title>smart quotes</title>
        <script type="text/javascript" src="js/jquery-1.7.2.min.js"></script>
        <script type="text/javascript" src="js/smartquotes.js"></script>
        <script type="text/javascript">
            window.onload = function() {
                SMARTQUOTES.init();
                SMARTQUOTES.quotes = new Array(
                'It\'s tempting to augment prototypes of built-in constructors such as Object(), Array(), or Function(), but it can seriously hurt maintainability.',
                'We come from XHTML backgrounds, so we will close all tags, use lowercase, and use quotation marks around attributes.',
                'Checking to see if a value exists inside an array is always a bore in JavaScript.',
                'JavaScript classes have the same effect on some people that garlic has on Dracula.',
                'Mixins are not supported natively by CoffeeScript, for the good reason that they can be trivially implemented yourself.',
                'Using a single var statement at the top of your functions is a useful pattern to adopt.',
                'Using the Function() constructor is as bad as eval()',
                'Any obstacle that I\'ve encountered during my development by placing JavaScript at the bottom of the page has been easily overcome and well worth the optimization gains.'
                );
                SMARTQUOTES.duration = 8000;
                SMARTQUOTES.start();
            };
        </script>
        <style type="text/css">
            div#quoteWrapper {
                border: 1px solid #999;
                padding: 10px;
                background: #eee;
                color: navy;
                width: 300px;
                border-radius: 5px;
                font-style: italic;
                font-family: arial;
                font-size: 12pt;
                text-align: center;
            }

        </style>
    </head>
<body>
    <div id="quoteWrapper">
        <div id="SMARTQUOTE"></div>
    </div>
</body>

smartquotes.js:

(function(global) {
    var SMARTQUOTES = {};
    if(global.SMARTQUOTES) {
        throw new Error('SMARTQUOTES has already been defined');
    } else {
        global.SMARTQUOTES = SMARTQUOTES;
    }
})(typeof window === 'undefined' ? this : window);  

SMARTQUOTES.init = function() {
    SMARTQUOTES.quoteIndex = 0;

    SMARTQUOTES.duration = 3000;

    SMARTQUOTES.quotes = new Array();
    SMARTQUOTES.quotes[0] = 'test quote #1';
    SMARTQUOTES.quotes[1] = 'this is the second quote';
    SMARTQUOTES.quotes[2] = 'and now the third and last quote'; 

    SMARTQUOTES.element = $('div#SMARTQUOTE').hide();

    SMARTQUOTES.incrementQuote = function() {
        SMARTQUOTES.quoteIndex++;
        if(SMARTQUOTES.quoteIndex >= SMARTQUOTES.quotes.length) {
            SMARTQUOTES.quoteIndex = 0;
        }
    }

    SMARTQUOTES.displayQuote = function () {
        var quote = SMARTQUOTES.quotes[SMARTQUOTES.quoteIndex];
        SMARTQUOTES.element.fadeOut('slow', function() {
            SMARTQUOTES.element.html(quote);
        });
        SMARTQUOTES.element.fadeIn();
        SMARTQUOTES.incrementQuote();
        SMARTQUOTES.startTimer();
    }

    SMARTQUOTES.startTimer = function () {
        var t = setTimeout('SMARTQUOTES.displayQuote()', SMARTQUOTES.duration);
    }

    SMARTQUOTES.start = function() {
        SMARTQUOTES.displayQuote();
    }

}
share|improve this question

1 Answer 1

up vote 4 down vote accepted
  • Take advantage of jQuery's .ready(). It's an abstraction that includes window.onload. Here's a shorthand version:

    $(function(){
        //DOM ready
    });
    
  • Build a plugin instead. That way, you get the benefits of having to be "chained" to jQuery. Here's a boilerplate to get you started. It has the necessary explanations as well as some prepared set-up for a plugin.

  • If portability is key to this code, I'd rather not hardcode the element that it will be attached to. If it were a plugin, it would look like this:

    $(function(){
    
        //DOM ready, attach plugin to div
    
        $('div#SMARTQUOTE').smartQuotes({
            duration : 8000,
            quotes : [
            'quote1',
            'quote2',
            ...
            ]
        });
    
    });
    

    everything looks "smart" this way (no pun intended);

  • Now i will dive into your code. First I notice, you are using new Array(). That's old school. Use literals intead:

    SMARTQUOTES.quotes = [];
    
  • in the boilerplate, you will only have one exposed function, and that is the smartQuotes function (the one that is attached to jQuery). Everything else is hidden in "private" so everything else is not touched. This way, there is no conflict at all. The only conflict you will get is when the user actually has a plugin of the same name as you have.

  • If you need to expose some functions/events for "hooking", hand them over as callbacks:

        $('div#SMARTQUOTE').smartQuotes(
            duration : 8000,
            quotes : [
            'quote1',
            'quote2',
            ...
            ],
            onStart : function(){
                //executed on start
            }
        );
    
        //in the plugin boilerplate
        this.options.onStart()
    
  • additionally, here are common naming conventions. They do not work as they are described but just signify their purpose:

    • UNDERSCORE_SPACED_ALL_CAPS - means a contstant
    • camelCased - public variables and functions
    • _underscoredPrefixedCamelCase - private properties and methods

    But you do have to be wary since there is a library named underscoreJS that uses an underscore namespace _.method. It may get confusing when you use _. Likewise with $, where not only jQuery uses it (I think Zepto and MooTools uses them)

share|improve this answer

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.