3
\$\begingroup\$

I have two forms in my HTML page. Both share a set of input elements (not a part of any of the forms, but rather residing separately). Besides that, the forms have their own input elements as well. Each of them also have their own notification div.

The process of POST request for both the forms is nearly identical, except those specifically owned input elements and notification divs.

Here is what I did to remove the code duplication:

function execute(config) {
    var initiator_token = getQueryVariable(window.location.search.substring(1), 'token');
    if (!initiator_token) {
        config.notifyEle
            .removeClass('alert-info')
            .addClass('alert-warning')
            .attr('data-i18n', config.invalidTokenMessage);
        return;
    }

    var params = {
        'initiator_token': initiator_token,
        'recipient_email': config.emailEle.val()
    };

    var lang = $('#mailLangSelector').val();
    if (lang) params['lang'] = lang;

    var message = $('#message').val();
    if (message) params['message'] = message;

    resetInputs();

    var loc = window.location;
    var url = loc.protocol + '//' + loc.host + $SCRIPT_ROOT + '/social' + config.url;

    $.ajax({
        url: url,
        type: 'POST',
        data: JSON.stringify(params),
        contentType: 'application/json',
        success: function(data, statusText, xhr) {
            config.notifyEle
                .removeClass('alert-warning')
                .addClass('alert-info')
                .attr('data-i18n', config.successMessage)
                .i18n();
        },
        error: function(xhr, statusText, errorThrown) {
            config.notifyEle
                .removeClass('alert-info')
                .addClass('alert-warning')
                .attr('data-i18n', config.failureMessage)
                .i18n();
        }
    });
}

So, the function accepts a config object, in which I pass form-specific values AND elements (I mean, jquery elements).

A config is like:

var inviteFormConfig = {
    emailEle: $('#inviteEmail'),
    notifyEle: $('#inviteInfo'),
    invalidTokenMessage: 'sharePage.descriptions.validTokenRequired',
    url: '/invitation',
    successMessage: 'sharePage.descriptions.invitationSent',
    failureMessage: 'sharePage.descriptions.invitationFailed'
};

And the usage for a given form:

$('#inviteForm').submit(function(evt) {
    evt.preventDefault();
    execute(inviteFormConfig);
});

Now, though this works well for my use, I want to know if there lies a better pattern for such a scenario. Also, should I be passing around jQuery elements like this in config?

\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

This looks quite good, one nitpick I have is that you spend a significant amount of lines showing messages, that should be a more generic information setting function like

function notifyUser( messageClass, message ){
  config.notifyEle
        .removeClass( 'alert-info alert-warning' )
        .addClass( messageClass )
        .attr('data-i18n', message );   
}

And I must warn you that not putting curly braces is playing a bit with fire: if (lang) params['lang'] = lang; at least use a new line?

Edit: I would go for an MVC approach, create a Model object that knows what fields are on the form and knows how to validate them. Then a View object that knows how to style and update the form ( put red borders around blank input types, show messages etc. ) and a a Controller object which puts all the listeners in place and also takes care of working with the model to validate the data on the form before submitting web requests.

\$\endgroup\$
4
  • \$\begingroup\$ Nice touch there with the generic notify message. I have integrated that. Also, I agree with the concern of the lack of curly braces. I've added them. \$\endgroup\$
    – jysh
    Oct 31, 2014 at 5:09
  • \$\begingroup\$ One question though. If it had been your code, would you have done this any other way? \$\endgroup\$
    – jysh
    Oct 31, 2014 at 5:10
  • \$\begingroup\$ Updated with my approach \$\endgroup\$
    – konijn
    Oct 31, 2014 at 12:17
  • \$\begingroup\$ Alright, thanks! MVC approach seems interesting. I will give it a try, and may be put it here for review! \$\endgroup\$
    – jysh
    Nov 3, 2014 at 4:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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