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've written some Javascript code with jQuery to display a dialog box on a web page that "floats" in the corder of the page.

(1) It has the following features: the dialog follows as the user scrolls the page.

(2) If the user holds down the [Ctrl] key, the dialog is hidden so that it doesn't obscure content.

I know that I tend to intuitively treat Javascript as if it were C code because it looks like C code and this is a habit I've been working to break.

Also, I'm working to write this code to be reusable and not litter the global namespace with various objects and functions.

I refer to the dialog as the "Little Black Book".

Bearing that in mind, please critique my code:

function getNewLittleBlackBook(blackBookId, blackBookWidth, blackBookHeight) {
    var jQueryBlackBook = $('#' + blackBookId);
    if (jQueryBlackBook.length == 0)
        throw "Could not locate the element specified by blackBookId";
    if (!blackBookWidth > 0 || !blackBookHeight > 0)
        throw "Invalid width or height specified for Little Black Book";

    var LittleBlackBook = {
        id: blackBookId,
        width: blackBookWidth,
        height: blackBookHeight,
        jQueryObj: jQueryBlackBook,

        // Functions to show and hide the little black book
        hide: function () { jQueryBlackBook.hide(); },
        show: function () { jQueryBlackBook.show(); },

        // This setPosition function determines the position based on the document scrolling.
        setPosition: function () {
            var windowHeight = $(self).height();
            var windowWidth = $(self).width();
            var scrollPosition = $(self).scrollTop();
            var newModalTop = windowHeight - this.height + scrollPosition - 40;
            var newModalLeft = windowWidth - this.width - 40;
            this.jQueryObj.css('top', newModalTop + 'px');
            this.jQueryObj.css('left', newModalLeft + 'px');
            this.jQueryObj.css('height', this.height);
            this.jQueryObj.css('width', this.width);
        }, // end setPosition

        handlers: {
            ExternalScroll: function () {                
                LittleBlackBook.setPosition();
            },
            // These next two handlers serve to show/hide the Little Black Book when the user holds down the [Ctrl] key.
            ExternalKeyDown: function (e) {
                if (e.keyCode == 17)
                    LittleBlackBook.hide();
            },
            ExternalKeyUp: function (e) {
                if (e.keyCode == 17)
                    LittleBlackBook.show();
            }
        } // end handlers
    };     // end LittleBlackBook object

    // Attach event handlers.
    $(self).scroll(LittleBlackBook.handlers.ExternalScroll);
    $(document).keydown(LittleBlackBook.handlers.ExternalKeyDown);
    $(document).keyup(LittleBlackBook.handlers.ExternalKeyUp);

    LittleBlackBook.setPosition();
    LittleBlackBook.show();

    // Return the object
    return LittleBlackBook;
}
share|improve this question

4 Answers 4

up vote 1 down vote accepted

I'd use the following pattern

; var _ = _ || {}; // Gets the _ object or creates it if it doesn't yet exist, think of it as your namespace, or your $ for jQuery

_.Stuff = (function() {
    var _privateStuff = 1;
    var _morePrivateStuff = "beer";

    function _selectBox()
    {
        // stuff
    }

    function _checkBox()
    {
        // stuff
    }

    function _radioButton()
    {
        // stuff
    }

    return {
        SelectBox : _selectBox,
        CheckBox : _checkBox,
        RadioButton : _radioButton,
        ExposedVariable : _privateStuff
    };
})();

this way, you call your functions in a kind of "Namespace.Class.Function" pattern, for instance _.Stuff.SelectBox();, you get to have private variables that may or may not be accessible from the outside of your pseudo-class, and you get the extensibility of being able to keep separate files for each of your classes by just using the pattern in them

; var _ = _ || {};

_.MoreStuff = (function() {
    return {
        Moar : "moar"
    };
})();

Here I posted a similar, perhaps more detailed, similar answer.

Update: You can also return functions like this:

; var _ = _ || {};

_.Superb = (function() {
    return {
        Moar : function(a,b){
            alert(a);
            return b;
        }
    };
})();
share|improve this answer
    
but if you have many many functions that you also want to be public, you will need to return all of them one by one. lots of extra code no? if you work in you own environment it's ok to have several large, global-scoped, functions no? –  vsync Jun 25 '11 at 17:31
    
See my edit 123 –  Nico Jun 25 '11 at 17:43
    
What if I have some "static" functions on my object. How can I make it so that my object refers to the same function in each instance when I want a function to be "static"? –  Rice Flour Cookies Jul 5 '11 at 15:57
    
@Rice what do you mean by "static"? –  Nico Jul 5 '11 at 18:06

You should throw Error objects rather than strings:

throw new Error("Invalid width or height specified for Little Black Book");

This provides a stack trace.

share|improve this answer
    
+1 Thank you for your answers. As for the overall structure of the code, do you have any suggestions? Does it look good? –  Rice Flour Cookies Jun 24 '11 at 20:04

You can combine your css calls:

this.jQueryObj.css({
    top: newModalTop, 
    left: newModalLeft,
    height: this.height,
    width: this.width
});

jQuery automatically inserts px when setting dimension properties.

share|improve this answer

You should consider changing your library to a jQuery plugin.
This way, people can write $("any selector").littleBlackBook().

You should store the object in $.data so that you can return it if the plugin is called again on the same element.


Also, you should figure out what should happen if someone calls your function on two different elements.
You can force uniqueness without cluttering the global namespace by adding a property to the function.
In a jQuery plugin, that would mean

$.fn.littleBlackBook.alreadyExists = true;
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.