ES6: Break it up

With promisesarrow functions, and functional array chaining available to us natively as of ES6, we now have a myriad of ways to consolidate overly verbose code. The problem is simply that this is too funtoo novel, and we all get carried away, cutting away code that isn’t necessary for compilation but improves readability, maintainability and in some cases testability.

This is an actual snippet of code I wrote for a personal project. The project is an Automated LendingClub Bot utilizing my NodeJS Lending Club API Implementation, and this snippet represents its buy function. This method takes an array of filters as an argument, applies them to a list of new loans, and ultimately purchases a $25.00 note for each.

Bot.prototype.buy = function buy(filters) {
  return this.api.loans.getNewListings().then(loans => {
    return filters.reduce((orders, filter) => {
      loans.filter(filter).forEach(loan => {
        if (!orders.has(loan.id)) {
          orders.set(loan.id, loan.createOrder(25.00));
        }
      });

      return orders;
    }, new Map());
  }).then(orders => this.api.account.submitOrders(Array.from(orders.values())));
};

All of these promisesarrow functions and functional array chains allowed me to write a very compact sliver of code that takes care of many functions:

  1. Fetching a list of available new loans from our API,
  2. Creating a list for each filter containing all the matching loans,
  3. Merging the lists together,
  4. Removing duplicates while maintaining insertion order,
  5. Creating an order for each unique loan, and
  6. Submitting those orders through our API.

However, after a good night’s rest and returning to the code the next morning, I had trouble making modifications. First, it took me a few minutes to remember how the logic was working. Then, I realized I’d have to restructure a good portion of the code to implement a new step to the process.

This is when I realized I had gone too far and it was time to separate out the components by function.

Breaking it up

Let’s walk through the steps we might take to make this method more legible, maintainable and testable.

Step 1. Separate logic by function

A good starting point is to separate the logic by function, and be verbose  about it. Forget about trying to be efficient with our lines of code or performance; we can address those concerns later. Our immediate goal is simply to decouple:

Bot.prototype.buy = function buy(filters) {
  // 1. Fetch a list of available new loans
  this.api.loans.getNewListings().then((loans) => {
    
    // 2. Create a list for each filter containing all the matching loans
    const filteredLists = filters.map(filter => loans.filter(filter));
    
    // 3. Merge the lists together
    const allMatches = [].concat.apply([], filteredLists);
    
    // 4. Remove any duplicates
    const uniquesById = new Map();
    
    allMatches.forEach(loan => {
      if (!uniquesById.has(loan.id)) {
        uniquesById.set(loan.id, loan);
      }
    });
    
    const uniques = Array.from(uniquesById.values());
    
    // 5. Create an order for each unique loan
    const orders = uniques.map(loan => loan.createOrder(25.00));
    
    // 6. Submit the orders
    return this.api.account.submitOrders(orders);
  });
};

Now we’ve made the code fairly verbose, and declared a lot of variables, but each step of the method is running sequentially. We can clearly see where one step ends and the next begins. Legibility has already improved. But we can do better.

Step 2. Create reasonable groupings

We now should be thinking about breaking our logic out into their own methods based on their function. However, just as we don’t want all of our logic in one method, we also don’t want to assign each statement to its own method. We should slim our methods down to the smallest reasonable functionality and no smaller. For example, these steps may be useful on their own:

  • [1. getNewListings] and [6. submitOrders] – These are already separated, as methods of my API implementation.
  • [5. createOrders] returns an array of orders for the array of loans passed in.

But these steps are only marginally useful on their own:

  • [2. filterLists] returns an array of arrays. These arrays are more useful to us if they are flattened, and don’t contain duplicates.
  • [3. flatten] returns an array, however it does not remove duplicates.
  • [4. removeDuplicates] returns an array without duplicates.

These steps can be grouped together to form larger, but more reasonable functions:

  1. union is an established operation in programming that effectively flattens and removes duplicates.
  2. filterLists can return the union of the array of arrays. This way, the output of the method is requires no manipulation to be useful to us.

Now that we’ve identified some useful groups, let’s break our steps our into their own methods:

// Step 1: Fetching new loans is already defined in
// bot.api.loans.getNewListings()

// Steps [2,3,4]: Filter Loans
Bot.filterLoans = function filterLoans(loans, filters) {
  const filteredLoanArrays = filters.map(filter => loans.filter(filter));
  return unionBy(filteredLoanArrays, 'id');
};

// Steps [3,4]: Union By
function unionBy(arrays, keyProperty) {
  const flattened = [].concat.apply([], array);
  const uniqueItems = new Map();
  
  flattened.forEach(item => {
    const key = item[keyProperty] || item;
    
    if (!uniqueItems.has(key)) {
      uniqueItems.set(key, item);
    }
  });
  
  return Array.from(uniqueItems.values());
}

// Step 5: Create Orders
Bot.createOrders = function createOrders(loans, amount) {
  return loans.map(loan => loan.createOrder(amount));
};

// Step 6: Submitting orders is already defined in
// bot.api.account.submitOrders(orders)

Now, our distinct logical groupings are re-usable, easy to read and create great targets for unit testing.

Note that filterLoans and createOrders are defined on the Bot class rather than its prototype. This is because these methods are related to the Bot use-case but are not tied to a specific instance of Bot. These methods are exposed because they may be useful to the user of our API outside the context of the buy method.

Additionally, unionBy has been decoupled from our loan logic. Now, it takes a customizable key property to check the array elements for uniqueness. This method can now be exported from this project’s code base into a more general tool-set to be used across projects. Or, we can re-use existing code from an existing library (such as lodash).

You may notice we’re missing the actual buy function. This brings me to the last step.

Step 3. Rebuild

Now that all of our pieces are separated into components, and not cluttering our buy method, we can rebuild the original method using our broken out pieces:

Bot.prototype.buy = function buy(filters) {
  return this.api.loans.getNewListings()
    .then(loans => Bot.filterLoans(loans, filters))
    .then(loans => Bot.createOrders(loans, 25.00))
    .then(orders => this.api.account.submitOrders(orders));
};

This method should require no inline comments; it lists very clear, discernible steps:

  1. Get the new loan listings via the loans API,
  2. Then filter those loans with the specified filters,
  3. Then create $25.00 orders for those loans,
  4. Then submit those orders via the account API

End Product

Here’s a snapshot of (roughly) how this code looks currently as it sits in my local git repository:

import { unionBy } from 'lodash';
import LCApi from 'lc-api';

function Bot(apiKey, investorId) {
  this.api = new LCApi(apiKey, investorId);
}

Bot.filterLoans = function filterLoans(loans, filters) {
  const filteredLoanArrays = filters.map(filter => loans.filter(filter));
  return unionBy.apply(null, filteredLoanArrays.concat(['id']));
};

Bot.createOrders = function createOrders(loans, amount) {
  return loans.map(loan => loan.createOrder(amount));
};

Bot.prototype.buy = function buy(filters) {
  return this.api.loans.getNewListings()
    .then(loans => Bot.filterLoans(loans, filters))
    .then(loans => Bot.createOrders(loans, 25.00))
    .then(orders => this.api.account.submitOrders(orders));
};

Note that we’re still using promises, arrow functions and functional array chains, but instead of using them to reduce our code footprint, we’re striking a balance between readability and brevity. The result is a much more elegant set of methods that is a pleasure to look at. We also exposed some useful functionality to the users of our API in the process, and that’s priceless.

I love ES6. I am extremely excited about the future of JavaScript development. Promises and arrow functions are very powerful and we’re all having a blast seeing what they’re capable of. Let’s just keep in mind that at some point, readability may suffer from overuse / misuse. I truly believe that maintaining this balance between readability and brevity is an art.

13 thoughts on “ES6: Break it up

  1. I like this. Let’s not turn this cool new stuff into another case of callback soup.

    Do you think promises are better than having well organised [named] callbacks?

    1. I find that in any instance where a callback interface might be considered, either a promise or event interface can be implemented instead for greater effect. In my eyes, the two biggest draws for the promise are:

      1) The promise creates human-readable chains of logic and
      2) The promise is the basis for ES7’s async/await, so any promise logic written today should “just work” in ES7 tomorrow.

      I’d also point out that you can actually name the functions you pass to promise constructors, as well as the promise itself, and organize them in all the same ways you could organize callback functions.

      If you have any examples of how you’ve seen callbacks used to better effect than Promise / event interfaces, I’d like to see them!

      1. I think promises are most useful when they represent the result of a single async operation, like fetching something from a server. Lots of awesome benefits – chaining, passing a promise around within an application to access that fetched data at some point in the future, etc.

        I don’t know if callbacks are ever significantly better, but in many instances they’re just as good and require fewer keystrokes. If you’re not chaining async operations or passing the resulting promise around your app, why include the extra code? I think the best possible async interface accepts a callback as the simplest possible usage, but also returns a promise should the developer want to take advantage of it.

        1. Thank you for this post, Kevin. This is something I’ve been thinking about for a while and haven’t had the time to sit down and articulate until now.

          The community is somewhat split on this issue, and appealing to both audiences means not limiting the market, a crucial concern for any product. So in the case where the product is an API to be consumed by another developer, I absolutely agree with the validity of the argument for supporting both callbacks and Promises in that API.

          However, in my opinion, the value of callback support ends there (at legacy support). I think we already agree that Promises are a clear choice when chaining, gating or other nontrivial logic is involved. So to address your specific question:

          I don’t know if callbacks are ever significantly better, but in many instances they’re just as good and require fewer keystrokes. If you’re not chaining async operations or passing the resulting promise around your app, why include the extra code?

          I’ll talk about these two examples in contrast:

          // Callbacks
          createOrders(orders, 25.00, 'myPortfolio', submitOrders, onFailure);
          
          // Promise
          createOrders(orders, 25.00, 'myPortfolio')
            .then(submitOrders)
            .catch(onFailure);
          

          Foremost, the Promise separates the method input from the program flow, creating a more explicit syntax. I would argue that as a result, the code is more intuitive to both read and write. It also eliminates some parameter guesswork & mental overhead:

          // Do I need to null it out?
          createOrders(orders, 25.00, null, submitOrders, onFailure);
          
          // Does this API support leaving it out?
          createOrders(orders, 25.00, submitOrders, onFailure);
          
          // Does this API take a failure callback or pass an err object?
          createOrders(orders, 25.00, function submitOrders(err, res) {
            if (err) { return onFailure(err); }
            // ...
          });
          

          To answer your question: I see a much greater gain in intuitiveness and legibility than in saving a few keystrokes. In addition, I don’t think the sum of any potential gains offered by callbacks outweighs the value of consistent asynchronous interfaces in our projects, and in our community.

          1. Having a function signature that accepts multiple callbacks is pretty gross. If all you need is success/failure, better to have a single node-style callback with takes an error as the first positional argument.

            I think promises are fine and useful, but it’s possible to design an API using only callbacks which works just fine. I think I am only pushing back on the idea that promises are always inherently better.

          2. One more bit and I will leave your blog in peace :)

            I definitely do think promises are great. It greatly simplifies code like this:

            db.find().then(function (docs) {
              return Promise.all(docs.map(function (doc) {
                return db.remove(doc);
              }));
            }).then(function () {
              // All docs removed
            });
            

            When doing async gymnastics promises are definitely the bee’s knees. If you’re not, callbacks can be just fine.

      2. To give a contrived example, what’s the difference between:

        buy.then(filterOrders).then(createOrders).then(submitOrders)
        

        and something like:

        buy(filterOrders(createOrders(submitOrders)))
        

        Both look pretty human readable to me. And the callback approach is shorter as well.

        But maybe I’ve seen too much Lisp and other functional languages in my life. A callback is just a continuation and continuations are pretty great.

        1. I think you have a good question I’d like to address, but I don’t think either of your examples would work as written. The Promise example is buying before it has orders, then filtering orders before they’re created, then creating orders, then submitting orders. The callback example is passing a submitOrders callback to createOrders, then passing the resulting orders to filterOrders, then passing the resulting orders to a buy function (When does submitOrders get called)?

          Can we agree on clearer examples to compare? Here’s my take:

          function buy() {
            return createOrders()
              .then(filterOrders)
              .then(submitOrders);
          }
          
          function buy() {
            return submitOrders(filterOrders(createOrders()));
          }
          

          And if we want to be a little less contrived, how might these look with arguments restored?

          function buy(filters, loans, amount) {
            return createOrders(loans, amount)
              .then(orders => filterOrders(orders, filters))
              .then(submitOrders);
          }
          
          function buy(filters, loans, amount) {
            return submitOrders(filterOrders(createOrders(loans, amount), filters));
          }
          

          Are there any changes you’d make? Let me know, and I can explain once we agree on the examples!

  2. Hey there,
    I might just be missing something completely. Can you show the code where you’re actually creating the promise to be used?

    1. Hey there!

      The initial promise is being created by the this.api.loans.getNewListings() function of my Lending Club API Implementation. Here’s an abstract of what’s happening:

      import request from 'superagent';
      
      api.loans.prototype.getNewListings = function getNewListings() {
        return new Promise((resolve) => {
          request({
            method: 'GET',
            url: 'https://api.lendingclub.com/api/investor/v1/loans/listing'
          }, (err, res) => resolve(res.body.loans));
        });
      };
      
  3. you guys are missing the point of promises. it has nothing to do with readability or not liking “callback hell”.

    the point of promises are that you can pass them around and resolve them at any point, now or later, in execution.

    its not just a nicety for control flow syntax, but rather a portable, storable, execution environment so to speak. you cannot take a callback, store its execution results, pass it around for a while, then resolve with some other logic.

    this is the point of promises

    1. You’re right about all of those benefits. I don’t think anybody was debating those; the discussions are around whether it’s ever beneficial (or equal) to use a callback instead of a Promise.

Leave a Reply