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.
var WorkflowDialogBuilder = _.once(function () {
    'use strict';
    var workflowDialog = $('#WorkflowDialog');
    var workflowDialogContent = $('#WorkflowDialogContent');

     var events = {
        onApplyChangesSuccess: 'onApplyChangesSuccess',
        onValidationFailed: 'onValidationFailed',
        onDialogOpen: 'onDialogOpen'

    var dialogConfig = {
        autoOpen: false,
        buttons: {
            OK: function () {
                var form = $(this).find('form');

                if (form.valid()) {
                    var self = this;
                        type: "POST",
                        url: 'Form/Save',
                        dataType: 'json',
                        data: form.serialize(),
                        success: function (data) {
                            $(self).trigger(events.onApplyChangesSuccess, data);
                        error: function (error) {
                } else {
            Cancel: function () {
        open: function () {


    return {
        onApplyChangesSuccess: function (event) {
            workflowDialog.on(events.onApplyChangesSuccess, event);
        onDialogOpen: function (event) {
            workflowDialog.on(events.onDialogOpen, event);
        onValidationFailed: function (event) {
            workflowDialog.on(events.onValidationFailed, event);
        buildAddTaskDialog: function () {
            workflowDialogContent.load('NewTaskDetails', function () {
                //Load ViewModel client-side helpers and then when its ready, open the dialog.
                $.getScript('../Scripts/Orders/NewTaskDetailsModel.js', function () {
        buildAddOrderDialog: function () {
            workflowDialogContent.load('NewOrderDetails', function () {
                //Load ViewModel client-side helpers and then when its ready, open the dialog.
                $.getScript('../Scripts/Orders/NewOrderDetailsModel.js', function () {

So, I've made the above builder. It is responsible for initializing a dialog element, opening it, and responding appropriately when the dialog closes.

Now, I was pretty happy with this implementation until I realized that, given the following scenario, events were subscribing and not unsubscribing.

//User calls WorkflowDialogBuilder.buildAddTaskDialog();
//NewTaskDetailsModel.js is loaded.
//NewTaskDetailsModel.js calls the following:

WorkflowDialogBuilder.onValidationFailed(function () {
    console.log("NewTaskDetailsMode.js onValidationFailed fired.");

//AddTaskDialog is closed by the user. NewTaskDetailsModel.js has already been loaded.
//AddTaskDialog continues to be subscribed to DialogBuilder's onValidationFailed.

//User calls WorkflowDialogBuilder.buildAddOrderDialog();
//User works with the dialog and clicks 'OK' to apply validation.
//AddTaskDialog's onValidationFailed event runs because WorkflowDialogBuilder triggers an event.

Now, I can think of one very simple solution to this. I could simply respond to the dialog's onClose event and, at that pont in time, unsubscribe all events.

Question: Does that seem like a proper solution? Or, have I gone down a bad path from the getgo and should rethink my architecture? If so, advice on how to restructure?

share|improve this question

1 Answer 1

up vote 1 down vote accepted

I would refactor it into a plugin that takes the content as an option. Then you will have two instances of "dialog controllers" each with its own events.

Have a look at the "plugin with data" example in the jQuery docs.


You'd set it up like

<div id="addTaskDialog"></div>
<div id="addOrderDialog"></div>
<script type="text/javascript">
// attach events to either

Otherwise I'd switch to $.unbind followed by $.bind in the subscribe functions.

share|improve this answer
Why on earth would you switch to using $.unbind and $.bind? Those are deprecated and have been replaced with $.on. I do like the idea of handing a model to the dialog, but I think I can do it without creating multiple div elements. Thanks! –  Sean Anderson Nov 30 '12 at 17:23
Hmm, my bad if they're depreciated. Use on and off then. :) I just think you're over complicating by cramming two different dialogs into one container. If that's your goal you should at least try to separate the inner logic of the plugin into two separate "classes" with their own events, possibly forwarding to the plugins events. –  Lars-Erik Dec 1 '12 at 11:38

Your Answer


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.