Boss wants this small script "cleaned up" - Help needed Plz

I wrote a simple checkout script for our page at work. When one of the more experienced developers looked at my code, he made a comment "we need to discuss you cleaning up your code"

Ok, so I am assuming he means to re-factor it, or take a more object approach.

Can anyone help me make this look a bit more elegant?  Any help would be appreciated. Maybe I can keep him off my back tomorrow!  :)

/** Document Ready */



jQuery(function ($) {

    jQuery.ajaxSetup({
        beforeSend: function () {
            $('#mmpc-loader').show();
        },
        complete: function () {
            $('#mmpc-loader').hide();
        }
    });

    // Form Submitted
    $( "#btnPurchase" ).click( function ( e )
    {

        e.preventDefault();

        Products = (sessionStorage.getItem("product") === "") ? sessionStorage.getItem("product") : "Solopreuner";

        // Hard Code Price
        let Price = 40;

        /* API Endpoints */
        let mmpcApi = "/Product/ProcessPayment";
        let authNetApi = "/Product/ProcessPayment";

        
        let Workstations = $( "#numWorkStations" ).val();   // Qty
        let total = 40 * Workstations;

        let valid = true;  // for future use when validating

        let FirstName = $( "#firstName" ).val();
        let LastName = $( "#lastName" ).val();
        let CompanyName = $( "#companyName" ).val();
        let Title = $( "#usrTitle" ).val();
        let Email = $( "#email" ).val();
        let Address = $( "#address" ).val();
        let City = $( "#city" ).val();
        let State = $( "#state" ).val();
        let ZipCode = $("#zip").val();
        let Phone = $("#phone").val();

        let CcNum = $( "#ccnum" ).val();
        let CcExpMonth = $( "#expmonth" ).val();
        let CcExpYear = $( "#expyear" ).val();
        let CcCvv = $( "#cvv" ).val();



        //product model per device
        const ProductViewModel = function (title, price, billingCycle, quantity, recurring) {
            this.Title = title;
            this.Price = price;
            this.BillingCycle = billingCycle;
            this.Quantity = quantity;
            this.Recurring = recurring;
        };

          // Object for Authorize.NET
        var ObjAuthDotNet = {
            FirstName: FirstName,
            LastName: LastName,
            Email: Email,
            Address: Address,
            State: State,
            Zip: ZipCode,
            CardNumber: CcNum,
            ExpirationDate: CcExpMonth + "/" + CcExpYear,
            CardCode: CcCvv,
            Phone: Phone,
            Country: "USA",
            Product: new ProductViewModel(Products, Price, 1, Workstations, true)
        };

        var ObjPaymentConfirmation = {
            FirstName: FirstName,
            LastName: LastName,
            Email: Email,
            Product: sessionStorage.getItem("product"),
            ClientId: "123"  // hard code for now
        };

        // Duplicate Object For MMPC, we are going to need
        // to add data to it for our own purposes.
        var objMMPC = jQuery.extend(true, {}, ObjAuthDotNet);

        // Process the payment
        $.ajax({
            type: 'POST',
            url: mmpcApi,
            data: ObjAuthDotNet,
            dataType: 'json',
            encode: true
        })
            // promise callback
            .done(function (data) {

                console.log(data);

                // Check for Errors
                if (data.messages.resultCode === "Error") {

                    var paymentErrors = [];

                    for (var i = 0; i < data.messages.message.length; i++) {

                        // Invalid Card Number
                        if (code === "37") {
                            paymentErrors.push("You have entered an invalid card number. Please try again.");
                         // Card Declined
                        } else if (code === "145" || code === "152") {
                            paymentErrors.push("Your credit card was declined. Please try another card.");
                        // For Wells Fargo Only, Billing/Zip
                        } else if (code === "112") {
                            paymentErrors.push("Your billing state / zip code is invalid. Please try again.");
                        // Invalid Card COde
                        } else if (code === "78") {
                            paymentErrors.push("The card code you entered was incorrent. Please try again.");
                        } else {
                            /* Unspecified Error */
                            paymentErrors.push("There was an unspecified error. Please try again.");
                        }
                    }

                    // Notify User
                    alertify.alert("There was an error processing your card.");
                    return false;

  
                } else if (data.messages.resultCode === "Ok") {

                    // Successful, Send Thank You Email
                    $.ajax({
                        type: "POST",
                        url: "/Client/ThankYou",
                        data: ObjPaymentConfirmation,
                        dataType: "json"
                    });

                    // Let User Know
                    alert('Payment Successful'); // for now we'll just alert the user
                    //window.location = '/Product/ProcessPayment';  // redirect a user to another page

                } else {

                    // Unspecified Response. Report as error
                    alertify.alert('Error', 'There was an error processing your payment. Please try again.'); 
                    
                }

            });

    });

    //--------------------------------------------------

    $('#numWorkStations').on('change', function () {
        var tNumWorkstation = this.value;
        //alert(tNumWorkstation);
        UpdateProductHeader(tNumWorkstation);
        $("#total").text(40 * tNumWorkstation);

    });


    function UpdateProductHeader(wCount) {
        var prodHeaderElem = $("#productHeading");
        var headerText;

        if (wCount === 1) {
            headerText = "Solopreuner";
        }

        if (wCount >= 2) {
            if (wCount <= 50) {
                headerText = "Small Business";
            }
        }

        if (wCount > 50) {
            // Display contact modal, re-direct to contact page
            headerText = "Enterprise";
        }

        prodHeaderElem.text(headerText);
        sessionStorage.setItem('product', headerText);
    }

});

Open in new window

John S.Web DeveloperAsked:
Who is Participating?
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

David S.Commented:
Only three things stick out to me. The first is only if-conditions were used in UpdateProductHeader but 2 else-if conditions seem quite applicable. Also in that function, why the nested if-condition instead of using an "&&"?

The second issue I would address is using the comma operator when declaring variables instead of using the "var" or "let" keyword for each one.

Thirdly, were is "Products" declared? You defined it on line 22.
0
Julian HansenCommented:
1. I would move constants out of the click handler
2. I would create a factory for the ObjAuthDotNet object. I would also consider setting up constant references to the input controls (assuming you use this code more than once) and then just access the .value elements of the HTML nodes.
3. I would put the .done code into separate functions - the reason being so you can easily see the flow of your click handler
- Instantiation (setting up the call
- The call itself
- Handling the response
  - Handling message output
  - Handling AJAX call to send email

The key thing you want is that your code looks readable - and anyone reading it can easily understand the flow without having to be bogged down in detail or scrolling up and down to see where they are or what they are doing.
0
Julian HansenCommented:
Here is a rough stab at the code. Note your code has some issues - there is code that is never used, your payment Error handling code refers to a code variable that does not exist - and does not actually use the returned values from the API call - I have included the code as you have it - but just warning you - I am not advertising this as working code - it is merely an example of an alternative layout.
<script>
/** Document Ready */

jQuery(function ($) {
    /* JH: CONSTANTS */
    // Hard Code Price
    let Price = 40;

    /* API Endpoints */
    let mmpcApi = "/Product/ProcessPayment";
    let authNetApi = "/Product/ProcessPayment";

    // JH: Event Handlers Start

    // JH: $ is passed in so use it
    $.ajaxSetup({
        beforeSend: function () {
            $('#mmpc-loader').show();
        },
        complete: function () {
            $('#mmpc-loader').hide();
        }
    });

    // Form Submitted
    $( "#btnPurchase" ).click( function ( e )
    {
        e.preventDefault();

        /* JH: THESE ARE NOT USED    
        let valid = true;  // for future use when validating
        let Title = $( "#usrTitle" ).val();
        let City = $( "#city" ).val();
        let CompanyName = $( "#companyName" ).val();
        let total = 40 * Workstations;
        */

        /* JH:Possible improvement: pass a form / parent container
        to the function and then retrieve the input data relative to that using
        control names rather than relying on ID's
        */
        var ObjAuthDotNet = new createAuthDotNet();

        // Duplicate Object For MMPC, we are going to need
        // to add data to it for our own purposes.
        /* JH: ALSO NOT USED */
        var objMMPC = jQuery.extend(true, {}, ObjAuthDotNet);

        // Process the payment
        $.ajax({
            type: 'POST',
            url: mmpcApi,
            data: ObjAuthDotNet,
            dataType: 'json',
            encode: true
        })
            // promise callback
            .done(function (data) {
                console.log(data);
                // Check for Errors
                if (data.messages.resultCode === "Error") {
                    // JH: Not sure why this is here - not used but including anyway
                    // JH: SEE WARNING IN handlePayamentErrors()
                    var paymentErrors = handlePaymentErrors(data);
                    // Notify User
                    alertify.alert("There was an error processing your card.");

                    // JH: What are you hoping this will do?
                    // JH: Take some time to understand how AJAX Works
                    // JH: Returning a value from an asynchronous callback goes nowhwere
                    return false;
                } 
                else if (data.messages.resultCode === "Ok") {
                    let sessionProduct = sessionStorage.getItem("product")
                    sendThankyouEmail(objAuthDotNet.FirstName, ObjAuthDotNet.LastName, objAuthDotNet.Email, sessionProduct, 123);

                } else {
                    // Unspecified Response. Report as error
                    alertify.alert('Error', 'There was an error processing your payment. Please try again.'); 
                }
            });
    });

    //--------------------------------------------------

    $('#numWorkStations').on('change', function () {
        var tNumWorkstation = this.value;
        //alert(tNumWorkstation);
        UpdateProductHeader(tNumWorkstation);
        $("#total").text(40 * tNumWorkstation);

    });
    /* JH: EVENT HANDLERS ENDS */

    /* JH: UTILITY FUNCTIONS STARTS */
    /* JH: Handle error return from AuthNet API Call 
        Takes an array of errors as input
        Returns an array of custom error messages
    */
    function handlePaymentErrors(data)
    {
        var paymentErrors = [];

        for (var i = 0; i < data.messages.message.length; i++) {
/* 
    JH: WARNING - WHERE DO YOU GET THE CODE VALUE FROM ????? 
                  data.messages not used.

        CODE BELOW REPEATED AS PER ORIGINAL BUT IT IS NOT CORRECT
*/
            // Invalid Card Number
            if (code === "37") {
                paymentErrors.push("You have entered an invalid card number. Please try again.");
            // Card Declined
            } else if (code === "145" || code === "152") {
                paymentErrors.push("Your credit card was declined. Please try another card.");
            // For Wells Fargo Only, Billing/Zip
            } else if (code === "112") {
                paymentErrors.push("Your billing state / zip code is invalid. Please try again.");
            // Invalid Card COde
            } else if (code === "78") {
                paymentErrors.push("The card code you entered was incorrent. Please try again.");
            } else {
                /* Unspecified Error */
                paymentErrors.push("There was an unspecified error. Please try again.");
            }
        }

        return paymentErrors;
    }

    function sendThankYouEmail(FirstName, LastName, Email, Product, ClientId)
    {
        // Object for Authorize.NET
        var ObjPaymentConfirmation = {
            FirstName : FirstName,
            LastName  : LastName,
            Email     : Email,
            Product   : Product,
            ClientId  : ClientId  // hard code for now
        };

        // Successful, Send Thank You Email
        $.ajax({
            type    : "POST",
            url     : "/Client/ThankYou",
            data    : ObjPaymentConfirmation,
            dataType: "json"
        });
        // JH: NO! We don't know that this is successful. You are telling the User the 
        // API call was a Success before it has even completed
        // Put this in a .done() handler rather.

        // Let User Know
        alert('Payment Successful'); // for now we'll just alert the user

        // JH: If you are going to enable this then make sure you do it in the callback from the AJAX call
        //window.location = '/Product/ProcessPayment';  // redirect a user to another page
    }

    // Create AuthDotNet Object
    function createAuthDotNet()
    {
        Products = (sessionStorage.getItem("product") === "") ? sessionStorage.getItem("product") : "Solopreuner";

        let productViewModel = {
            Title: Products,
            Price: constPrice,
            BillingCycle: 1,
            Quantity: $( "#numWorkStations" ).val(),
            Recurring: true
        }

        return {
            FirstName     : $("#firstName" ).val(),
            LastName      : $( "#lastName" ).val(),
            Email         : $( "#email" ).val(),
            Address       : $( "#address" ).val(),
            State         : $( "#state" ).val(),
            Zip           : $("#zip").val(),
            CardNumber    : $( "#ccnum" ).val(),
            ExpirationDate: $( "#expmonth" ).val() + "/" + $( "#expyear" ).val(),
            CardCode      : $( "#cvv" ).val(),
            Phone         : $("#phone").val(),
            Country       : "USA",
            Product       : productViewModel
        };
    }

    function UpdateProductHeader(wCount) {
        var prodHeaderElem = $("#productHeading");
        var headerText;

        if (wCount === 1) {
            headerText = "Solopreuner";
        }

        if (wCount >= 2) {
            if (wCount <= 50) {
                headerText = "Small Business";
            }
        }

        if (wCount > 50) {
            // Display contact modal, re-direct to contact page
            headerText = "Enterprise";
        }

        prodHeaderElem.text(headerText);
        sessionStorage.setItem('product', headerText);
    }
});
</script>

Open in new window

0

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
ste5anSenior DeveloperCommented:
Some general comments, Julian did a great detailed analysis:

I don't see any validation of the input data. When dealing with payment processing, you must validate.

Your original code contains a lot of inline comments. In many cases inline comments indicates blocks of code which should be separate methods. Cause then the method name can carry that information.

Another approach to this is to outline your workflow on paper. This gives you also a overview of the necessary methods needed.
0
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
JavaScript

From novice to tech pro — start learning today.