0
\$\begingroup\$

Actually I got a form with multiple input (around 39) on which I wish to perform some check and display customs messages based on sets of conditions.

I found a way to address my problem but I'm wondering (and I'm barely sure there is) if there's any better way to approach this.

My trouble come from the fact there's a lot of custom field I need to address to (many input to check with custom error displayer per input and sometimes more than custom message to send / display) plus quite lot of check to perform.

function hideShowError(elem, sub) {
    sub = sub || false;
    parentElem = getBoxParent($(elem), sub);

    if (parentElem != false)
        parentElem.addClass('errorWrapper');

    return false;
}

function hideShowErrorWithMessage(elem, message, special, sub, noBorder) {
    sub = sub || false;
    special = special || false;
    noBorder = noBorder || false;
    parentElem = getBoxParent($(elem), sub);

    if (parentElem != false) {
        parentElem.addClass('errorWrapper');
        if (noBorder)
            parentElem.addClass('errorWrapper-no-border');
        parentElem.find('.errorSpe').text(message);
        special ? parentElem.find('.disable-error').addClass("active") : parentElem.find('.disable-error-pb').removeClass("active");
    }

    return false;
}

function getBoxParent(childElem, sub) {
    childElem = childElem || "";
    sub = sub || false;

    if (childElem != "") {
        if (!sub)
            return childElem.closest('.questBox3');
        return childElem.closest('.subQuestBox3');
    }
    return false;
}

function testInput() {
    var input1 = $('input1-t').is(':checked') || $('input1-f').is(':checked');
    var input2 = $('input2-f').is(':checked');
    var input3 = !$('input3-t').is(':checked') && !$('input2-f').is(':checked');
    var input4 = $('input4-f').is(':checked');
    var input5 = !$('input5-t').is(':checked') && !$('input4-f').is(':checked');
    var input6 = $('input6-f').is(':checked');
    var input7 = !$('input7-t').is(':checked') && !$('input6-f').is(':checked');
    var input8 = $('input8-f').is(':checked');
    var input9 = !$('input9-t').is(':checked') && !$('input8-f').is(':checked');
    var input10 = $('input10-t').is(':checked');
    var input11 = !$('input11-f').is(':checked') && !$('input10-t').is(':checked');
    var input12 = $('input12-t').is(':checked') || $('input12-f').is(':checked');
    var input13 = $('input13-t').is(':checked') || $('input13-f').is(':checked');
    var input14 = $('input14-t').is(':checked') || $('input14-f').is(':checked');
    var input15 = $('input15-f').is(':checked');
    var input16 = $('input16select option:selected').length > 0 && $('input16select option:selected').val() != 0;
    var input17 = $('input17select option:selected').length > 0 && $('input17select option:selected').val() != 0;
    var input18 = $('input18').val().length > 0;
    var input19 = $('input19').val().length > 0;
    var input20 = $('input20').val().length > 0;
    var input21 = $('input21').val().length > 0;
    var input22 = $('input22').val().length > 0;
    var input23 = $('input23').val().length > 0;
    var input24 = $('input24').val().length > 0;
    var input25 = $('input25-t').is(':checked') || $('input25-f').is(':checked');
    var input26 = $('input26-t').is(':checked') || $('input26-f').is(':checked');
    var input27 = $('input27-t').is(':checked') || $('input27-f').is(':checked');
    var input28 = $('input28').val().length > 0 || $('input28-2').val().length > 0 || $('input28-3').val().length > 0;
    var input29 = $('input29-t').is(':checked') || $('input30-f').is(':checked');
    var input30 = $('input30-f').is(':checked');
    var input31 = $('input31-t').is(':checked') || $('input32-f').is(':checked');
    var input32 = $('input32-f').is(':checked');
    var input33 = $('input33').val().length > 0;
    var input34 = $('input33').val() != "" && (!$.isNumeric($('input33').val()) || ($('input33').val() > 99 || $('input33').val() < 0));

    // checkbox input
    var input35 = $('input35:checked').length;
    var input36 = $('input36:checked').length;

    // siret control
    var numSiret = ($('input28').val().toString()).replace(/\s+/g, '');

    var validSiret = ("" == numSiret || (verif_siren_siret(numSiret, 9) && verif_siren_siret(numSiret, 14)))

    // Get CP (97, 971, 972 ...)
    var departement = '<%= @adresse.try(:departement) %>'.match("^97");

    // Error check Question 1 (input)
    if (!input1)
        ret = hideShowError('input1-t');
    // Error check Question 9 (input)
    if (input2 && !input3)
        ret = hideShowErrorWithMessage('input2-f', "<%= t('sometrad1') %>", false, false, true);
    // Error check Question 10 (input)
    if (input4 && !input5)
        ret = hideShowErrorWithMessage('input4-f', "<%= t('sometrad1') %>", false, false, true);
    // Error check Question 11 (input)
    if (input6 && !input7)
        ret = hideShowErrorWithMessage('input6-f', "<%= t('sometrad1') %>", false, false, true);
    // Error check Question 12 (input)
    if (input8 && !input9)
        ret = hideShowErrorWithMessage('input8-f', "<%= t('sometrad1') %>", false, false, true);
    // Error check Question 13 (input)
    if (input10 && !input11)
        ret = hideShowErrorWithMessage('input10-t', "<%= t('sometrad1') %>", false, false, true);

    if (!input2 && input3)
        ret = hideShowErrorWithMessage('input2-f', "<%= t('sometrad2') %>", false);
    if (!input4 && input5)
        ret = hideShowErrorWithMessage('input4-f', "<%= t('sometrad2') %>", false);
    if (!input6 && input7)
        ret = hideShowErrorWithMessage('input6-f', "<%= t('sometrad2') %>", false);
    if (!input8 && input9)
        ret = hideShowErrorWithMessage('input8-f', "<%= t('sometrad2') %>", false);
    if (!input10 && input11)
        ret = hideShowErrorWithMessage('input10-t', "<%= t('sometrad2') %>", false);
    if (!input12)
        ret = hideShowError('input12-t');
    if ($('input12-t').is(':checked') && !input13)
        ret = hideShowErrorWithMessage('input13-t', "<%= t('sometrad2') %>", false, true);
    if (!input14)
        ret = hideShowErrorWithMessage('input14-t', "<%= t('sometrad2') %>", false, true);
    if (input15)
        ret = hideShowErrorWithMessage('input13-t', "<%= t('sometrad3') %>", false, true);
    if (!input16)
        ret = hideShowError('input16select');
    if (!input17)
        ret = hideShowError('input17select');
    if (!input18)
        ret = hideShowError('input18');
    if (!input19)
        ret = hideShowError('input19');
    if (!input25)
        ret = hideShowError('input25-t');
    if ($('input25-t').is(':checked') && !input20)
        ret = hideShowError('input20');
    if ($('input25-t').is(':checked') && !input21)
        ret = hideShowError('input21');
    if ($('input25-t').is(':checked') && !input22)
        ret = hideShowError('input22');
    if ($('input25-t').is(':checked') && !input23)
        ret = hideShowError('input23');
    if ($('input25-t').is(':checked') && !input24 && null != departement)
        ret = hideShowError('input24');
    if (!input26)
        ret = hideShowError('input26-t');
    if (input26 && !input27)
        ret = hideShowErrorWithMessage('input27-t', "<%= t('sometrad2') %>", false);
    if ($('input27-t').is(':checked') && !input28) {
        ret = hideShowErrorWithMessage('input28', "<%= t('sometrad4') %>", false);
        ret = hideShowErrorWithMessage('input28-2', "<%= t('sometrad4') %>", false);
        ret = hideShowErrorWithMessage('input28-3', "<%= t('sometrad4') %>", false);
    }
    if ($('input27-t').is(':checked') && $('input28').val().length > 0 && !validSiret)
        ret = hideShowErrorWithMessage('input28', "<%= t('sometrad5') %>", false);

    // Error check Question 13 (checkbox)
    if (input35 > 0)
        ret = hideShowErrorWithMessage('#pb-decence-1', "<%= t('sometrad2') %>", false, false, true);
    if (input35 == 0 && input36 == 0)
        ret = hideShowErrorWithMessage('#pb-decence-1', "<%= t('sometrad2') %>", false);

    if (!input29)
        ret = hideShowErrorWithMessage('input29-t', "<%= t('sometrad2') %>", false);
    if (input30)
        ret = hideShowErrorWithMessage('input29-t', "<%= t('sometrad6') %>", false);

    if (!input31)
        ret = hideShowErrorWithMessage('input31-t', "<%= t('sometrad2') %>", false);
    if (input32)
        ret = hideShowErrorWithMessage('input31-t', "<%= t('sometrad7') %>", false, false, true);

    if (!input33)
        ret = hideShowErrorWithMessage('input33', "<%= t('sometrad8') %>", false);
    if (input34)
        ret = hideShowErrorWithMessage('input33', "<%= t('sometrad2') %>", false);
}

I also add an HTML (slim) block of code, though all input are not the sames the general structure is mostly the same

div.questGridWrapper
    div.questBox
        span= t('quest_9')
    div.questBox
        span.special-info
            span= t('quest_error_warn.warn_0') + " "
            <i class="fa fa-question-circle"></i>
        div.no-display-info
            br
            span= t('quest_9_prec.sub_1')
            br
            br
            ul
                li.li-style
                    span= t('input.sub_2')
                span.color OR
                li.li-style
                    span= t('input.sub_3')
div.questGridWrapper
    div.questBox3
        div.form-group.radio_buttons
            div.form-check-hacked

                span.radio
                    label for="input-t"
                        - if @pb_projet_courant.pb_logement.espace_suffisant == true
                            input.form-check-input.radio_buttons type="radio" name="input[value]" checked="checked" id="input-t" value="true"
                            = t('generic.quest_yes')
                        - else
                            input.form-check-input.radio_buttons type="radio" name="input[value]" id="input-t" value="true"
                            = t('generic.quest_yes')

                span.radio
                    label for="input-f"
                        - if @pb_projet_courant.pb_logement.espace_suffisant == false
                            input.form-check-input.radio_buttons type="radio" name="input[value]" checked="checked" id="input-f" value="false"
                            = t('generic.quest_no')
                        - else
                            input.form-check-input.radio_buttons type="radio" name="input[value]" id="input-f" value="false"
                            = t('generic.quest_no')

        .error-message
            span.errorSpe.errorSpe-noBorder= t('someDefaultTrad')

(I had to anonymize the code before posting, so sadly getting a working exemple may take some times but the code itself actually work, I'm looking for way to improve my if statements if there's any regarding the numbers of conditions that are checked.

\$\endgroup\$
  • \$\begingroup\$ Welcome to code review, where we review working code. We understand the need to anonymize code, but if it's not working it is off-topic and we can't review it. \$\endgroup\$ – pacmaninbw May 14 '19 at 15:19
  • \$\begingroup\$ Thx, the code itself is working, it just need quite lot of context (mostly HTML) to be fully working, my main concern is about the whole testInput() function, and how to best optimise the whole "if" part \$\endgroup\$ – Matthias May 14 '19 at 15:28
  • 1
    \$\begingroup\$ Even if you anonymize the code, it still needs to be realistic enough so that we can understand what is going on, so that we can give you proper advice. This code is, in my opinion, too obfuscated to review. For example, why is the if (input15) check not negated like the others, and why does it trigger an error message for input13-t? Voting to close for lack of context. \$\endgroup\$ – 200_success May 14 '19 at 21:25
  • \$\begingroup\$ Thx for your return I get your point, in your exemple input15 is not negated like other 'cause it's a simple check , input15 condition is only one check (if the radio button is ticked on false) and that's the only condition to trigger the error, for the error message it's triggered in input13-t 'cause it's the parent element in my HTML, but yep I get your point. My asking is more about how to optimise the whole "var =" and "if" statement part, I didn't though the whole HTML was needed for this, still I can provide more code but it gonna take some time as there's quite lot of HTML \$\endgroup\$ – Matthias May 15 '19 at 8:16
1
\$\begingroup\$

There's A LOT or repeated code. Start by DRYing your code, for example:

All this lines are too similar:

var input3 = !$('input3-t').is(':checked') && !$('input2-f').is(':checked');
var input5 = !$('input5-t').is(':checked') && !$('input4-f').is(':checked');
var input7 = !$('input7-t').is(':checked') && !$('input6-f').is(':checked');
var input9 = !$('input9-t').is(':checked') && !$('input8-f').is(':checked');

you could have a function:

function is_not_checked(inputNum) {
  return !$('input'+inputNum+'-t').is(':checked') && !$('input'+(inputNum-1)+'-f').is(':checked');
}

Now you can do

var input3 = is_not_checked(3);
var input5 = is_not_checked(5);
var input7 = is_not_checked(7);
var input9 = is_not_checked(9);

And you don't even need to set the variable, it's consice enough to use as the if condition:

if (is_checked(2) && is_not_checked(3)) //you could have that `is_checked` function too
  ret = hideShowErrorWithMessage('input2-f', "<%= t('sometrad1') %>", false, false, true);

You can also move that ret = hideShow..... line to another function so you don't have to repeat that same call to the hideShow... function.

Use the same idea on the other inputX assignments and the if conditions. When you finish removing all the repeated code you'll end up with a cleaner code that will be easier to analyze to simplify the logic (maybe after refactoring you find even more repeated patterns).

EDIT:

You are also doing ret = .... every time and you don't use ret anywhere after that, remove all those assignements.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer, yep the ret is an artifact I forgot my bad, I was using it to get to know in other functions if there was an error but indeed it's not used anymore. I get the point of using a function and quite like the idea, one question though I was using "variable" storage to get only one call as some input are used multiple times isn't it more optimum to store the "check" instead of calling it each time in a ressource perspective ? \$\endgroup\$ – Matthias May 15 '19 at 8:17
  • \$\begingroup\$ You could save the returned value if you need to use it multiple times, sure. Just follow the DRY principle. \$\endgroup\$ – arieljuod May 15 '19 at 12:19

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