validating form before uploading files

Question

This function below return true/false. How I could possibly improve the code quality?

function uploadcontrolhasError() {
    if (dialogisVisible === true) {
        let uploadcontrol = $("#upImport").data("kendoUpload"),
            files = uploadcontrol.getFiles();
        console.log(files);

        let FileLink = $("#txtFileLink").val();

        if (files.length === 0 || FileLink.length != 0) {
            let dialogcontrol = $('#Dialog').data("kendoDialog");
            dialogcontrol.open();
            return true;
        } else {
            processuploadControl();
            return false;
        }
    } else {
        processuploadControl();
        return false
    }
}


function processuploadControl() {
    let uploadcontrol = $("#upImport").data("kendoUpload"),
        files = uploadcontrol.getFiles();
    if (files.length != 0) {
        var counter = 0;
        $('.k-file-success[data-fileManagerIds]').each(function (kfile) {
            let filemanagerid = this.dataset.filemanagerids;
            let hdfilemanagerid = $('<input>').attr({
                type: 'hidden',
                name: 'fileManagerIds[' + counter + ']',
                id: 'fileManagerIds',
                value: filemanagerid
            });

            $("#permissionsRequestForm").append(hdfilemanagerid);
            counter++;
        });
    }
}

function processsubmitForm() {
    dialogisVisible = false;
    var dialog = $("#Dialog").data("kendoDialog");
    dialog.close();
    $("#permissionsRequestForm").submit();
}

function closeDialog() {
    dialogisVisible = false;
    var dialog = $("#Dialog").data("kendoDialog");
    dialog.close();
}

function materialsconfirmhasError() {
    let materialsconfirm = $('input[name="ckmaterialsconfirm"]:checked').length > 0;
    let uploadcontrol = $("#upImport").data("kendoUpload"),
        files = uploadcontrol.getFiles();

    let FileLink = $("#txtFileLink").val();
    console.log(FileLink);
    if (files.length != 0 || FileLink.length != 0) {
        if (materialsconfirm == false) {
            $("#lblConfirmError").show();
            return true
        } else {
            $("#lblConfirmError").hide();
            return false
        }
    }
    return false
}

function uploadcontrolsizehasError() {
    var hasError = false;
    $('.k-file-name-invalid').each(function (kfile) {
        console.log('uploadcontrolsizehasError');
        $("#lblFileSizeError").show();
        hasError = true;
    });
    return hasError;
}

Answer 1

You asked:

How I could possibly improve the code quality?

first things:

• use camelCase for function names (or else kabob_case) to separate words for readability. It is also recommended by numerous style guides (e.g. google JS, airBnB, etc.) to use camelCase for variable names.
• use const for any variable that doesn't need to be re-assigned - e.g. FileLink, dialogcontrol. This helps avoid accidental re-assignment and other bugs. If you determine that you need to re-assign a variable then switch it to let.
• when calling the jQuery .each() method utilize the first argument (currently named kfile but could be changed to index, counter, etc.) instead of manually creating and incrementing a separate variable counter.
• just as materialsconfirm is assigned in materialsconfirmhasError() based on the length of a jQuery collection, hasError in uploadcontrolsizehasError() could be assigned in a similar fashion instead of calling the .each() method to loop over each element, which calls the show() method on a single element
• The first three lines of processsubmitForm() appear to be identical to the three lines of closeDialog() so instead of duplicating those three lines processsubmitForm() could simply call closeDialog().
• in JavaScript many values are considered false-y - e.g. 0 so conditions like (files.length != 0) can be simplified to (!files.length)

Simplifying logic

Looking at uploadcontrolhasError it appears that there are two else blocks which both contain a call to processuploadControl() and return false. The first else could be eliminated if the else keywords were removed:

function uploadcontrolhasError() {
    if (dialogisVisible === true) {
        let uploadcontrol = $("#upImport").data("kendoUpload"),
            files = uploadcontrol.getFiles();
        console.log(files);

        let FileLink = $("#txtFileLink").val();

        if (files.length === 0 || FileLink.length != 0) {
            let dialogcontrol = $('#Dialog').data("kendoDialog");
            dialogcontrol.open();
            return true;
        }
    }
    processuploadControl();
    return false
} 

Global variables

I happened to notice that in the original version of the post there was concern about global variable dialogisVisible. Yes it is wise to avoid using globals though for a small site it isn't the end of the world. There are alternatives - e.g.

• using the revealing module pattern
• calling a function to check the status of whether the dialog is visible
• encapsulating the variable as a property of an object (potentially with the es6 class syntax - possibly with private class fields)

Answer 2

Separation of concerns

It's best when a function has a single responsibility.

At a glance, and by its name, uploadcontrolhasError looks like a validator function, that would perform some checks and return a boolean, with no side effects. But it has a very important side effect of populating #permissionsRequestForm with hidden input fields.

It would be easier to understand the logical flow of the program if this function didn't have that side effect, if it had the single responsibility of validating something.

The responsibility of populating #permissionsRequestForm would be good to happen right before submitting the form.

The caller of uploadcontrolhasError could decide to submit or not, based on the result of the validator functions, and then call a function to build the necessary data and submit the form.

Naming

I find the naming style somethinghasError unusual. These functions return true when something is wrong. A more common approach is to make validator functions return false when something is wrong, and follow a naming style isSomethingValid.

The code calling the "...hasError" methods was not posted, I imagine it would look something like:

if (!materialsconfirmhasError() && !uploadcontrolsizehasError() && !uploadcontrolhasError()) { ... }

I find it more natural to read something like:

if (isMaterialsConfimValid() && isUploadControlSizeValid() && isUploadControlValid()) { ... }

Another naming issue is the overuse of the term "process". This is very a generic term. It's better to use something more descriptive whenever possible. Consider for example these renames:

• processuploadControl -> setFileManagerIds
• processsubmitForm -> submitPermissionsRequestForm

Separate logic from view

The UI element references that are important for the correct functioning of the program are scattered around in the code, mixed with the validation logic. If I want to know what are all the important UI elements, I have to read the entire code.

Worse, many of the selectors are used repeatedly, for example $('#Dialog'), which is a risk:

• If there's a typo somewhere, it can be difficult to spot
• If somebody wants to rename the UI element, it must be done at many places

It would be good to encapsulate these UI element lookups in a dedicated object, that translates logical concepts (such as "dialog") to the appropriate UI elements. This will eliminate the above risks.

Don't repeat yourself

The processsubmitForm includes within its body the entire content of closeDialog. It should call closeDialog instead of duplicating code.