2
\$\begingroup\$

In my application, I am using a lot of jQueryUI dialog boxes.
In order to reduce the length of code, and because I am using often the very same parameterization, I was wondering about a way to efficiently and easily create multiple dialogs.

I ended up with that: I've created multiple functions (dialog_OkCancel, dialog_Input_OkCancel, dialog_YesNoCancel) that call the same "main" dialog_handler.
That way I avoid calling the big main function and use only the little and easier ones.

These functions are pretty simple so I don't know what else to add. :)

Here is a working snippet of my code to illustrate that:

// jQuery UI dialog custom management
function dialog_Handler({
  title,
  message,
  buttons,
  input
}) {
  $("<p>" + message + "</p>").dialog({ // Could use “var dialog = ”
    //autoOpen: false,					// false would prevents regular opening
    show: "drop",
    open: function(event, ui) {
      // Overlay parameterization
      $("div.ui-widget-overlay").css({
        "background": "#000",
        "opacity": "0.4"
      });
      $(".ui-widget-overlay").hide().fadeIn();
      // Adds some more configuration if input needed
      if (input) {
        // Adds input field right under the message
        $(this).append('<br /><br /><input id="dialog_Input" style="width: 350px; padding: 4px;" type="text" value="' + input + '"><br />');
        // Binds “Enter” to press first button (usually “Ok”)
        $(this).keydown(function(event) {
          if (event.keyCode == $.ui.keyCode.ENTER) {
            $(this).parent().find("button:eq(1)").trigger("click");
            return false;
          }
        });
      }
    },
    title: title,
    buttons: buttons,
    modal: true,
    resizable: false,
    height: "auto",
    width: 400,
    hide: {
      effect: "drop",
      duration: "fast"
    },
    closeOnEscape: true,
    // Overlay fadeout
    beforeClose: function(event, ui) {
      // Wait for the overlay to be faded out to try closing again
      if ($('.ui-widget-overlay').is(":visible")) {
        $('.ui-widget-overlay').fadeOut("fast", function() {
          $('.ui-widget-overlay').hide();
          $('.ui-icon-closethick').trigger('click');
        });
        return false;
      }
    },
    close: function() {
      $(this).dialog("destroy");
    }
  });
  return;
}

function dialog_OkCancel({
  title,
  message,
  funk_Ok
}) {
  var buttons = {
    "Ok": function() {
      funk_Ok();
      $(this).dialog("close");
    },
    "Cancel": function() {
      $(this).dialog("close");
    }
  }
  dialog_Handler({
    title,
    message,
    buttons
  });
  return;
}

function dialog_Input_OkCancel({
  title,
  message,
  input,
  funk_Ok
}) {
  var buttons = {
    "Ok": function() {
      funk_Ok();
      $(this).dialog("close");
    },
    "Cancel": function() {
      $(this).dialog("close");
    }
  }
  dialog_Handler({
    title,
    message,
    buttons,
    input
  });
  return;
}

function dialog_YesNoCancel({
  title,
  message,
  funk_Yes,
  funk_No
}) {
  var buttons = {
    "Yes": function() {
      funk_Yes();
      $(this).dialog("close");
    },
    "No": function() {
      funk_No();
      $(this).dialog("close");
    },
    "Cancel": function() {
      $(this).dialog("close");
    }
  }
  dialog_Handler({
    title,
    message,
    buttons
  });
  return;
}
.dialog {
  display: block;
  margin: 8px;
  text-align: center;
  width: 120px;
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/jqueryui/1.12.1/jquery-ui.min.js"></script>
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/jqueryui/1.12.1/jquery-ui.min.css" />

<html>

<body>

  <button class="dialog" onclick='dialog_OkCancel({
      title: "Title",
      message: "I do not have any idea for the message.",
      funk_Ok: function(){ console.log("Sorry. :("); }
    });'>Dialog<br>Ok/Cancel</button>
  <button class="dialog" onclick='dialog_Input_OkCancel({
      title: "Title",
      message: "I do not have any idea for the message.",
      input: "Type something…", 
      funk_Ok: function(){ console.log("Give me more words ! :)"); }
    });'>Dialog<br>Input</button>
  <button class="dialog" onclick='dialog_YesNoCancel({
      title: "Just talking…",
      message: "Are you fine?",
      funk_Yes: function(){ console.log("You said “Yes”! :)"); },
      funk_No: function(){ console.log("You said “No”! :("); }
    });'>Dialog<br>Yes/No/Cancel</button>
</body>

</html>

This snippet is one piece of code out of many.
I would like that piece of code to be improved… So I could improve all others.
How would you do it?
What would be your method to improve the management of multiple functions?

\$\endgroup\$
  • 1
    \$\begingroup\$ The question you are asking is off-topic for Code Review. If you are interested in splitting methods into smaller ones or other refactoring techniques you probably should ask it on Software Engineering because we here are only improving working code. We don't discuss philosophical questions. \$\endgroup\$ – t3chb0t Mar 4 '18 at 10:48
  • \$\begingroup\$ @t3chb0t, Your comment is funny to me, because I had posted that question on stackoverflow and I was told in the comments to post it here, on codereview.stackexchange … :) And, in fact, I'd like my code to be improved if it's possible to do so. But the ways to improve it could vary from one coder to another. Don't you think? \$\endgroup\$ – Takit Isy Mar 4 '18 at 10:54
  • 2
    \$\begingroup\$ In general it's ok to post it here however you are asking about a very specific problem: I would like to know how you guys, create multiple smaller functions from a bigger one? What is the preferred method to do it? - this makes your quesiton off-topic for Code Review. If you were interested in just improving your code and how it can be written in a more efficient etc way, then it'd be fine. You have to decide which one is it and pick the right stackexchange site for it. \$\endgroup\$ – t3chb0t Mar 4 '18 at 11:02
  • 1
    \$\begingroup\$ @TakitIsy Many people recommend to put code here for review, while it doesn't meet our policies. It's gotten wrong there all of the time. Also asking for "the best way" is always opinion based and won't generate good answers anywhere at the SE network. \$\endgroup\$ – πάντα ῥεῖ Mar 4 '18 at 11:02
  • \$\begingroup\$ @t3chb0t noted, I edited my question to… maybe, I hope, someday, get answers. \$\endgroup\$ – Takit Isy Mar 4 '18 at 11:05
3
+50
\$\begingroup\$

Question responses

What would be your method to improve the management of multiple functions?

Looking at the three functions that call dialog_Handler() it appears that the main redundancies are with the click handlers for the buttons. See the response to the question below for one technique to simplify that logic.

In a comment, you asked:

Do you know if there is any way to call $(this).dialog("close"); without writing it inside each function(){ } of the var buttons?

Yes, you can abstract the function of closing the dialog out to a separate function, like:

function closeDialog() {
    $(this).dialog("close");
}

And that function can be bound to the functions for the button handlers using the Javascript function .bind() to create a partially applied function for the funk_Ok parameter:

  var buttons = {
    "Ok": closeDialog.bind(null, funk_Ok),
    "Cancel": closeDialog
  }

And that close function could accept an optional callback function (for functions like func_Ok)- if that is a function, it can be called:

function closeDialog(callback) {
    (typeof callback === "function") && callback();
    $(this).dialog("close");
}

But the problem here is that this in this context is not the same as before - it is the window object. So one solution is to give the HTML element wrapped in the dialog object an id attribute, like below:

const dialogId = 'message_dialog';
function dialog_Handler({
  title,
  message,
  buttons,
  input
}) {
  $("<p id='"+dialogId+"'>" + message + "</p>").dialog({ // Could use “var dialog = ”

And then in the closeDialog function, just reference that id attribute when closing the dialog:

function closeDialog(callback) {
    (typeof callback === "function") && callback();
    $('#'+dialogId).dialog("close");
}

See this in action in the snippet below:

Edit:

The code can be simplified by using Object.assign()- each function call to dialog_Handler() can use that to assign the button on the object that gets destructured:

dialog_Handler(Object.assign(arguments[0], {buttons: buttons}));

Then each function can omit the object properties it doesn't use (e.g. message, title)...

//could use const
var dialogId = 'dialogContainer';
// jQuery UI dialog custom management
function dialog_Handler({
  title,
  message,
  buttons,
  input
}) {
  $("<p id='"+dialogId+"'>" + message + "</p>").dialog({ // Could use “var dialog = ”
    //autoOpen: false,                    // false would prevents regular opening
    show: "drop",
    open: function(event, ui) {
      // Overlay parameterization
      $("div.ui-widget-overlay").css({
        "background": "#000",
        "opacity": "0.4"
      });
      $(".ui-widget-overlay").hide().fadeIn();
      // Adds some more configuration if input needed
      if (input) {
        // Adds input field right under the message
        $(this).append('<br /><br /><input id="dialog_Input" style="width: 350px; padding: 4px;" type="text" value="' + input + '"><br />');
        // Binds “Enter” to press first button (usually “Ok”)
        $(this).keydown(function(event) {
          if (event.keyCode == $.ui.keyCode.ENTER) {
            $(this).parent().find("button:eq(1)").trigger("click");
            return false;
          }
        });
      }
    },
    title: title,
    buttons: buttons,
    modal: true,
    resizable: false,
    height: "auto",
    width: 400,
    hide: {
      effect: "drop",
      duration: "fast"
    },
    closeOnEscape: true,
    // Overlay fadeout
    beforeClose: function(event, ui) {
      // Wait for the overlay to be faded out to try closing again
      if ($('.ui-widget-overlay').is(":visible")) {
        $('.ui-widget-overlay').fadeOut("fast", function() {
          $('.ui-widget-overlay').hide();
          $('.ui-icon-closethick').trigger('click');
        });
        return false;
      }
    },
    close: function() {
      $(this).dialog("destroy");
    }
  });
  return;
}
function closeDialog(callback) {
    (typeof callback === "function") && callback();
    $('#'+dialogId).dialog("close");
}
function dialog_OkCancel({
  funk_Ok
}) {
  var buttons = {
    "Ok": closeDialog.bind(null, funk_Ok),
    "Cancel": closeDialog
  }
  dialog_Handler(Object.assign(arguments[0], {buttons: buttons}));
  return;
}

function dialog_Input_OkCancel({
  input,
  funk_Ok
}) {
  var buttons = {
    "Ok": closeDialog.bind(null, funk_Ok),
    "Cancel": closeDialog
  }
  dialog_Handler(Object.assign(arguments[0], {buttons: buttons}));
  return;
}

function dialog_YesNoCancel({
  funk_Yes,
  funk_No
}) {
  var buttons = {
    "Yes": closeDialog.bind(null, funk_Yes),
    "No": closeDialog.bind(null, funk_No),
    "Cancel": closeDialog
  }
  dialog_Handler(Object.assign(arguments[0], {buttons: buttons}));
  return;
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/jqueryui/1.12.1/jquery-ui.min.js"></script>
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/jqueryui/1.12.1/jquery-ui.min.css" />

<button class="dialog" onclick='dialog_OkCancel({
      title: "Title",
      message: "I do not have any idea for the message.",
      funk_Ok: function(){ console.log("Sorry. :("); }
    });'>Dialog<br>Ok/Cancel</button>
  <button class="dialog" onclick='dialog_Input_OkCancel({
      title: "Title",
      message: "I do not have any idea for the message.",
      input: "Type something…", 
      funk_Ok: function(){ console.log("Give me more words ! :)"); }
    });'>Dialog<br>Input</button>
  <button class="dialog" onclick='dialog_YesNoCancel({
      title: "Just talking…",
      message: "Are you fine?",
      funk_Yes: function(){ console.log("You said “Yes”! :)"); },
      funk_No: function(){ console.log("You said “No”! :("); }
    });'>Dialog<br>Yes/No/Cancel</button>

One could also create a hidden element in the DOM, have dialog_Handler() update the text/HTML of that element and show it (using .show() - or toggle a class name that shows it), then hide it (using .hide()) when destroying the dialog. That way, the abstracted close function can just utilize that element.

//could use const
var dialogContainer = $('#dialogContainer');
// jQuery UI dialog custom management
function dialog_Handler({
  title,
  message,
  buttons,
  input
}) {
  dialogContainer.text(message).show().dialog({ // Could use “var dialog = ”
    //autoOpen: false,                    // false would prevents regular opening
    show: "drop",
    open: function(event, ui) {
      // Overlay parameterization
      $("div.ui-widget-overlay").css({
        "background": "#000",
        "opacity": "0.4"
      });
      $(".ui-widget-overlay").hide().fadeIn();
      // Adds some more configuration if input needed
      if (input) {
        // Adds input field right under the message
        $(this).append('<br /><br /><input id="dialog_Input" style="width: 350px; padding: 4px;" type="text" value="' + input + '"><br />');
        // Binds “Enter” to press first button (usually “Ok”)
        $(this).keydown(function(event) {
          if (event.keyCode == $.ui.keyCode.ENTER) {
            $(this).parent().find("button:eq(1)").trigger("click");
            return false;
          }
        });
      }
    },
    title: title,
    buttons: buttons,
    modal: true,
    resizable: false,
    height: "auto",
    width: 400,
    hide: {
      effect: "drop",
      duration: "fast"
    },
    closeOnEscape: true,
    // Overlay fadeout
    beforeClose: function(event, ui) {
      // Wait for the overlay to be faded out to try closing again
      if ($('.ui-widget-overlay').is(":visible")) {
        $('.ui-widget-overlay').fadeOut("fast", function() {
          $('.ui-widget-overlay').hide();
          $('.ui-icon-closethick').trigger('click');
        });
        return false;
      }
    },
    close: function() {
      $(this).dialog("destroy");
      dialogContainer.hide();
    }
  });
  return;
}
function closeDialog(callback) {
    (typeof callback === "function") && callback();
    dialogContainer.dialog("close");
}
function dialog_OkCancel({
  funk_Ok
}) {
  var buttons = {
    "Ok": closeDialog.bind(null, funk_Ok),
    "Cancel": closeDialog
  }
  dialog_Handler(Object.assign(arguments[0], {buttons: buttons}));
  return;
}

function dialog_Input_OkCancel({
  input,
  funk_Ok
}) {
  var buttons = {
    "Ok": closeDialog.bind(null, funk_Ok),
    "Cancel": closeDialog
  }
  dialog_Handler(Object.assign(arguments[0], {buttons: buttons}));
  return;
}

function dialog_YesNoCancel({
  funk_Yes,
  funk_No
}) {
  var buttons = {
    "Yes": closeDialog.bind(null, funk_Yes),
    "No": closeDialog.bind(null, funk_No),
    "Cancel": closeDialog
  }
  dialog_Handler(Object.assign(arguments[0], {buttons: buttons}));
  return;
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/jqueryui/1.12.1/jquery-ui.min.js"></script>
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/jqueryui/1.12.1/jquery-ui.min.css" />
<p id="dialogContainer" style="display: none"></p>

<button class="dialog" onclick='dialog_OkCancel({
      title: "Title",
      message: "I do not have any idea for the message.",
      funk_Ok: function(){ console.log("Sorry. :("); }
    });'>Dialog<br>Ok/Cancel</button>
  <button class="dialog" onclick='dialog_Input_OkCancel({
      title: "Title",
      message: "I do not have any idea for the message.",
      input: "Type something…", 
      funk_Ok: function(){ console.log("Give me more words ! :)"); }
    });'>Dialog<br>Input</button>
  <button class="dialog" onclick='dialog_YesNoCancel({
      title: "Just talking…",
      message: "Are you fine?",
      funk_Yes: function(){ console.log("You said “Yes”! :)"); },
      funk_No: function(){ console.log("You said “No”! :("); }
    });'>Dialog<br>Yes/No/Cancel</button>


Other Review points

Feedback

I like the usage of the jQuery ui keycode constant for the enter key (i.e. if (event.keyCode == $.ui.keyCode.ENTER) {). I must admit I hadn't used that before but aim to do so in the future.

I really like the application of object destructuring for passing the named parameters - that is quite nifty!

Suggestions

Cache DOM references

I see a couple places where DOM elements are looked up in succession, for example:

if ($('.ui-widget-overlay').is(":visible")) {
    $('.ui-widget-overlay').fadeOut("fast", function() {
      $('.ui-widget-overlay').hide();
      $('.ui-icon-closethick').trigger('click');
    });
    return false;
  }

It is wise to cache those lookups in a variable - or actually, use const to declare a constant unless there is a need to re-assign that value...

const widgetOverlay = $('.ui-widget-overlay');
// Wait for the overlay to be faded out to try closing again
if (widgetOverlay.is(":visible")) {
    widgetOverlay.fadeOut("fast", function() {
      widgetOverlay.hide();
      $('.ui-icon-closethick').trigger('click');
    });
    return false;
}

Placeholder text

I would use the placeholder attribute instead of the value attribute of the text input, since it appears that text is really prompting the user...

<input type="text" placeholder="Type some text here..." />

\$\endgroup\$
  • \$\begingroup\$ Do you know if there is any way to call $(this).dialog("close");without writing it inside each function(){ } of the var buttons ? \$\endgroup\$ – Takit Isy Mar 17 '18 at 11:04
  • \$\begingroup\$ See my updated answer \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Mar 19 '18 at 6:12

Your Answer

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

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