Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
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

closed as off-topic by Jamal Jul 25 '15 at 19:21

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.