5
\$\begingroup\$

I'm very new to backbone.js and JavaScript. I would like to hear some reasonable critiques / advice against my JavaScript code based on backbone.js framework with backbone-forms.

The aim of this simple app is to present a user with a simple registration form (twitter bootstrap modal dialog) and upon clicking on registration button post the model to a RESTful web service.

Model:

var MemberModel = Backbone.Model.extend({
    schema: {
        name: {
            type:  'Text',
            title: 'Full Name',
            validators: [
                'required'
            ],
            editorAttrs: {
                'placeholder': 'Type in your full name...',
                'class':'input-xlarge'
            }
        },
        email: {
            dataType: 'email',
            title: 'E-mail address',
            validators: [
                'required',
                'email'
            ],
            editorAttrs: {
                'placeholder': 'Type in your email address...',
                'class':'input-xlarge'
            }
        },
        country: {
            type: 'Select',
            options: [
                {
                    val: 0,
                    label: 'Choose a country...'
                },
                {
                    val: 1,
                    label: 'Country #1'
                },
                {
                    val: 2,
                    label: 'Country #2'
                },
                {
                    val: 3, 
                    label: 'Country #3'
                }
            ],
            editorAttrs: {
                'data-placeholder': 'Choose a country',
                'class': 'input-xlarge'
            }
        }
    },
    urlRoot: '/api/members',
    validate: function(attrs) {
        var errors = {};
    if ( attrs.country == 0 ) {
        errors.country = 'Choose a country from list';
    }
        if ( !_.isEmpty(errors) ) return errors;
    }
});

Create an instance of the model:

var member = new MemberModel();

The View:

var MemberSignUpView = Backbone.View.extend({
    name: "MemberSignUpView",
    model: MemberModel,
    el: $('#frontend'),
        initialize:
            function()
            {
                if ( ! ich['signup'] ) {
                    ich.addTemplate('signup', 'ajax:/media/js/tpl/frontend/' + this.name + '.html?' + (Math.random() * 1000000).toString());
                }
            },
        events:
            {
                "click #wndSignupClose": "onCloseClick",
                "click #wndSignupSubmit": "onSubmitClick"
            },
        onSubmitClick:
            function()
            {
                var errors = frmSignUp.commit();
                if ( _.isEmpty(errors) ) {
                    member.save();
                }
            },
        onCloseClick:
            function()
            {
                this.el.modal('hide');
                this.el.remove();
                this.undelegateEvents();
            },
        render:
            function()
            {           
                this.el = ich.signup();
                if ( typeof window.frmSignUp == "undefined" ) {
                    window.frmSignUp = new Backbone.Form({
                        model: member
                    }).render();
                }
                $(this.el).find('.modal-body').append(window.frmSignUp.el);
                $(this.el).modal({
                backdrop: 'static',
                keyboard: false
            });
            return this;
        }
});

Create modal window uppon clicking on a button:

$(function() {
        $('#btnSignup').bind('click', function(){
            var wndSignUp;
            if ( typeof wndSignUp == "undefined" ) {
                wndSignUp = new MemberSignUpView().render().el;
            }
            $('#frontend').append(wndSignUp);
    wndSignUp.modal('show');
    });
});
\$\endgroup\$
5
\$\begingroup\$

It's pretty good code. Well done! There are no major mistakes or pain points that would annoy other developers. All I can do is to nitpick a little bit.

  1. First of all take a look at some JavaScript style guide. jQuery has a good one and Google has a great one, too. You may find many others and although they all diverge a little bit there are a lot of common practices that they all agree on. One of those is braces' positioning. Instead of

    render:
        function()
        {
    

    you'd better use

    render: function () { // note the spaces, too :)
    
  2. Another point is the use of === instead of ==. I've noticed that you've never used a triple-equals operator in your code and I'm a little bit nervous. Maybe you know about the one vs. the other and you know what are you doing in this particular code (and btw your code works totally fine with double-equals). But what if you don't know? And what if some other developer who doesn't know JavaScript well take a look at your code and try to do something similar? Suppose he doesn't know the rules and you code is the largest and most complicated piece of JavaScript he's ever seen. Which way of thinking do you think is more bullet-proof for a JS-newbie:

    JavaScript is somewhat weird: I have to use === to compare things.

    or

    JavaScript is so easy! I'll use == to compare stuff everywhere in my code!

    I guess the former is better.

  3. I noticed you use .bind() method for event binding. Consider switching to new .on() method introduced in jQuery 1.7. One of the current goals for jQuery team is to make their library smaller and more lightweight so they work hard to cut off all the fat that's accumulated over the years. At some point in future they may remove the old API so it's better start using new API today to make your code future-proof. It''l be a lot less hassle to switch to new version of jQuery in future.

Other than that your code is fine. I really like the fact that you use Underscore JS. That saves you and other developers a lot of time and headaches.

\$\endgroup\$
  • 1
    \$\begingroup\$ === vs == is nitpicking, and not a pain point? JSLint and Douglas Crockford beg to differ. \$\endgroup\$ – ANeves wants peace for Monica May 8 '12 at 16:53
  • 1
    \$\begingroup\$ Here it may be a nitpicking. Here: typeof wndSignUp == "undefined" - typeof operator always returns string. So the'll be no conversion. And here: attrs.country == 0 the values of country are taken from val field in model JSON. Which is fine, too, since there is a limited set of possible labels which are safe for ==. In general, yes, it's better to simply stick with === but who knows, maybe the author knows the rules and wants to save one character. \$\endgroup\$ – Andrew Андрей Листочкин May 10 '12 at 9:04
3
\$\begingroup\$

About Backbone I could add some suggestions:

  1. Pass selector string to the el property — Backbone will handle it.
  2. You can change initializing block to be more "Backbonish":
$(function() {
        $('#btnSignup').on('click', function () {            
        if ( typeof wndSignUp == "undefined" ) {
            // Initialize view
            var wndSignUp = new MemberSignUpView({el: '#frontend'});
            // Append form and modal into DOM
            wndSignUp.render();
        }
        // Display modal to the user
        wndSignUp.showModal();
    });
});

In this case render view should be changed to:

var MemberSignUpView = Backbone.View.extend({
    ...
    // el: $('#frontend'), // — don't hardcode your el, pass it into constructor as above
    ...
    frmSignUp: function() {
        return new Backbone.Form({
            model: member
        }).render();
    }
    render: function () {           
        // this.el = ich.signup(); -- what is this? Do you really want to replace el?

        // Clear #frontend from all child nodes
        this.$el.empty();
        this.$el.append( ich.signup() );

        // Don't use super global variables, use it as view property instead
        // or even as local variable in render() scope
        //if ( typeof window.frmSignUp == "undefined" ) {
        //    window.frmSignUp = new Backbone.Form({
        //        model: member
        //    }).render();
        //}

        // Backbone creates good object for you: this.$el, so don't do $(this.el)
        this.$el.find('.modal-body').append(this.frmSignUp);
            this.$el.modal({
            backdrop: 'static',
            keyboard: false
        });

        // return this; // Don't do this, but encapsulate logic instead
    },
    showModal: function() {
        this.$el.modal('show');
    }
});

Suggested approach is valid for SLAs, but good code style and patterns are usable and readable even with small tasks.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy