Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

There's this Ajax-heavy, JQuery-based site which I'm about to work on. There hasn't been much structure in the JS code so far (lots of functions at global scope, hard-coded in the HTML onclick="something(args);". I want to improve upon that and introduce some structure. How?

Here's what I've tried. This part is supposed to handle a single element which consists of a text field (span) which is replaced by an input on focus and runs an AJAX update call when a submit button is clicked or enter is pressed.

$(function() {
    var box = $('.foobar-info'),
        input = box.find('input'),
        label = box.find('span#name'),
        button = box.find('button');

    label.click(start_edit);
    button.click(submit);
    input.keypress2(KEYS.enter, submit);

    function start_edit() {
        box.addClass('is-edit');
    }
    function submit() {
        MyAjaxModule.edit_foobar(save_success,
            {'id': GLOBAL_FOO_ID, 'value': input.val()});
    }
    function save_success(data) {
        label.text(input.val());
        box.removeClass('is-edit');
    }
});

Showing and hiding of parts is driven by the CSS:

.foobar-info .state-display {display: inline;}
.foobar-info .state-edit {display: none;}
.foobar-info.is-edit .state-display {display: none;}
.foobar-info.is-edit .state-edit {display: inline;}

So far this achieves:

  • keeps the behaviour away from the markup
  • keeps relevant behaviour together in one block

OTOH, I don't think it's testable or particularly easy to re-use with different configurations.

I've attempted to rewrite this to use objects and this. lookups instead of the big closure, but didn't end up in a code that would please me. For example, this part

label.click(start_edit);

would look like

this.label.click(this.start_edit.bind(this));

which looks really verbose plus it's super-easy to forget the unwieldy .bind(this) part.

This article served some inspiration, but felt rather hacky to me, mostly because relying on global name lookups.

So:

  • Any particular problems with my approach except what I've mentioned so far? How to solve?
  • Is there a canonical way to structure the code which I'll be happy to incorporate?
share|improve this question
The Module Pattern mentioned in that article is really great for projects like this because you can add/remove functionality in a jiffy. Also check out the Observer Pattern, aka Pub/Sub, which might be helpful with structuring your AJAX calls. – Jonny Sooter Jul 2 at 18:52

2 Answers

up vote 0 down vote

If this is a sizable component on your page which has state and a UI that reflects that state and, optionally, the data is server-backed, you might want to invest in the use of a framework. Try MVC (or MVC-like) frameworks like Backbone.js and others. In addition, use templating frameworks for the UI. My choice is Mustache, but still, there are a lot more.

This will split your code into the View, which is your UI. There's the Controller, otherwise known as the logic of the component. Then there's the Model, which is the data, usually split between local and server data, synced. Note that some frameworks are not strictly MVC but should serve the same purpose.

By now you might think "Sure, the code is now split up into separate components. But there are many components, what should I do?". Module management comes into play with dependency management libraries. I usually use RequireJS. These libraries manage the proper loading of required scripts before the scripts that require them execute.

So in summary, you split your code into parts of the MVC, glue them together with the dependency manager.

On the other hand, if this is component is somewhat small, then you might opt to make a widget/plugin. You might still need the MVC approach, but instead of using a baked framework, you roll your own. It's as simple as abstracting a few low-level operations into generic functions and data structures and you are good to go.

OOP concepts in JS would be your best friend here, as you can encapsulate several entities into objects. For example, you can create a View object and have it hold the UI elements of interest and several render methods. You can make a Model object which holds a simple data structure, and a few CRUD methods.

share|improve this answer
up vote 0 down vote

There are two schools of thought

  • Programmatic
  • Declarative

Your example here is programmatic, where JavaScript is binding events after the DOM load. The problem I have with programmatic is, using your words, although it "keeps the behavior away from the markup" it moves the markup into the behavior. The interface that is needed to reuse the same JavaScript is now tied to what is defined on the page. Using declarative, your example being onclick="something(args);", I can more easily follow a Model View Presenter (MVP) pattern where the view simply forwards all events to the Presenter to perform the business logic.

Here the events can be organized into a class object to remove them from the global namespace. Scope can be controlled by passing in the model as a parameter.

presenter.js

function Presenter(parameter) {

  // private variables
  var myVar;

  // public method
  this.submit = function() {
  };

  // private method
  function doSomething() {
  }
}

page.html

...
<script type="text/javascript" src=".../presenter.js"></script>
<script type="text/javascript">
  var presenter = new Presenter(...);
</script>
...
<button onclick="presenter.onButtonClick();">Click me</button>
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.