4
\$\begingroup\$

Code Review : Could promises be 'chained'?

I am developing a simple Quote tool for the business-owner to fill in a form (item number, item name, price, etc) and send to the potential customer.

This code review request is in reference to the "view-quote" page, which simply displays a quote that was previously sent. (see pic)

[pic]

Data Structure

As you can see, the data is broken up into three tables:

  1. Quotes Table: Holds general info about the quote.
  2. Clients Table: Holds general client info.
  3. Items Table: Holds individual quote items.

Fetch-ing The Quote Data

When a user hits this page, a parameter is passed in the URL (for example):

view-quote.html?quote_id=GL555

So from here, I needed to query the QUOTES table first.

Within those results, is our CLIENT_ID. Using that, I then query the CLIENTS table.

Lastly, I query the ITEMS table to get all items associated with the QUOTE.

Did I Break A Promise?

I am fairly new to writing modern JavaScript, however, I did manage to get this code functioning. But as you will see, I believe this code could be re-factored a number of ways.

The JavaScript

// Quotes View - View Model
// +---------------------------------------------------------------------------+
// | View Quote View Model                                                     |
// |                                                                           |
// | view-quote-view-model.js                                                  |
// +---------------------------------------------------------------------------+
// |                                                   |
// +---------------------------------------------------------------------------+/


let ItemsModel = function (item_number, item_name, item_price, quantity, manufacturer, subtotal) {
    this.item_number = item_number;
    this.item_name = item_name;
    this.item_price = item_price;
    this.quantity = quantity;
    this.manufacturer = manufacturer;
    this.subtotal = subtotal;
}


// GLOBAL PAGE VARIABLES
var gQuoteNumber = $_GET("quote_number");
var gClientId;
var gGrandTotal = 0;

console.log(`Quote Number: ${gQuoteNumber}`);

function ViewQuoteViewModel() {

    var self = this; // Scope Trick


    /* QUOTE Observables */
    self.quote_id = ko.observable();
    self.quote_number = ko.observable(gQuoteNumber);
    self.quote_date = ko.observable();
    self.amount = ko.observable();
    self.business_name = ko.observable();
    self.notes = ko.observable();

    /* CLIENT Observables */
    self.client_id = ko.observable();
    self.first_name = ko.observable();
    self.last_name = ko.observable();
    self.email = ko.observable();
    self.phone = ko.observable();
    self.address = ko.observable();
    self.city = ko.observable();
    self.state = ko.observable();
    self.zip = ko.observable();

    self.grand_total = ko.observable(gGrandTotal);

    /* ITEMS Observables */
    self.items = ko.observableArray();

    /* COMPUTED */
    self.fullName = ko.computed(function() {
        return self.first_name() + " " + self.last_name();
    }, self);


    /* GET PAGE DATA */


    self.getQuote = function (quoteNumber) {

        return fetch(apiQuotesOne + gQuoteNumber, {
            method: "GET",
            headers: {
                "Content-Type": "application/json"
            }
        })
        .then(handleErrors)
        .then(function(response) {
            return response.json();
        }).then(function(data) {
            // Populate Observables
            self.quote_id(data[0].id);
            self.quote_number(gQuoteNumber);
            self.quote_date(data[0].quote_date);
            self.amount(data[0].amount);
            self.business_name(data[0].business_name);
            self.notes(data[0].notes);
            self.client_id(data[0].client_id)

            // Set Global Variable
            gClientId = data[0].client_id;

            // GO TO NEXT FUNCTION, GET CLIENT INFORMATION
            self.getClient(gClientId);
        });

    };



    self.getClient = function(clientId){
        return fetch(apiCustomersOne + gClientId, {
            method: "GET",
            headers: {
                "Content-Type": "application/json"
            }
        })
        .then(handleErrors)
        .then(function(response) {
            return response.json();
        }).then(function(data) {
            // Populate Observables
            console.log(data);
            self.first_name(data[0].first_name);
            self.last_name(data[0].last_name);
            self.email(data[0].email);
            self.phone(data[0].phone);
            self.address(data[0].address);
            self.city(data[0].city);
            self.state(data[0].state)
            self.zip(data[0].zip)

            // GO TO NEXT FUNCTION, GET ITEMS
            self.getItems();
        });
    }



    self.getItems = function(quoteId){
        return fetch(apiItems + gQuoteNumber, {
            method: "GET",
            headers: {
                "Content-Type": "application/json"
            }
        })
        .then(handleErrors)
        .then(function(response) {
            return response.json();
        }).then(function(data) {
            // Populate Items Array
            var subtotal;
            $.each(data,
                function (key, val) {
                    //console.log(val);
                    subtotal = val.quantity * val.item_price;
                    gGrandTotal = gGrandTotal + subtotal;
                    self.grand_total(gGrandTotal);
                    self.items.push(new ItemsModel(val.item_number,val.item_name, val.item_price, val.quantity, val.manufacturer, subtotal ));
                });


        });

    }





    // Kick it off
    self.getQuote(gQuoteNumber);


}


ko.applyBindings(new ViewQuoteViewModel());

...here is my helper function to grab query string parameters:

function $_GET(param) {
    var vars = {};
    window.location.href.replace( location.hash, '' ).replace( 
        /[?&]+([^=&]+)=?([^&]*)?/gi, // regexp
        function( m, key, value ) { // callback
            vars[key] = value !== undefined ? value : '';
        }
    );

    if ( param ) {
        return vars[param] ? vars[param] : null;    
    }
    return vars;
}

I got this working by calling one function from inside another. It just feels like "cheating". It feels like bad code.

Could anyone point me in the right direction to clean this up a bit? I am thinking the three calls to the API can be really chained.

Thank you for looking.

PS. I am using KNOCKOUTJS as my MVVM pattern, in case that changes anything.

John

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Regarding the self "trick", don't use it, just switch to arrow functions which preserve this:

  this.fullName = ko.computed(() => this.first_name() + ' ' + this.last_name(), this);

Regarding the main concern, simply call those functions via .then and pass the data via return in the previous .then handler:

Passing of data:

  this.getQuote = quoteNumber => fetchJson(/*foo*/).then(data => {
    // ...............
    return data[0].client_id;
  });

  this.getClient = clientId => fetchJson(/*bar*/).then(data => {
    // ................
    return /*bar*/;
  });

  this.getItems = quoteId => {
    // ................
    return /*whatever*/;
  };

Calling/chaining:

  this.getQuote(gQuoteNumber)
  .then(this.getClient) // this is an arrow function that preserves `this`
  .then(this.getItems);

fetchJson could be something like:

function fetchJson(url, options) {
  return fetch(url, options).then(r => r.json(), handleErrors);
}
\$\endgroup\$

Your Answer

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

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