4
\$\begingroup\$

I built a function in JavaScript which executes a search algorithm to find a town in a particular state. The dataset, referenced by the variable townsAndStates is an array of object pairs, each of which has a town and a state, and only returns the first few results. An example of an object in this array is {"state": "Rhode Island", "town": "Quahog"}.

The search algorithm is quite simple:

  1. Check if the search input is a town and a state, OR just the name of a single town or a single state.
  2. If it's the former, then filter through the dataset finding both a matching town name AND state name.
  3. If it's the latter, then filter through the dataset finding either a matching town name or a matching state name.

Is there a way to make this function perform better and improve the readability of the code?

Here is the function:

const searchTownsAndStates = (input) => {
  let townsAndStates = getRawTownsAndStates();
  let regex = /(?<town>[A-Z ]+),(?<state>[A-Z ]*)/gi;
  if (regex.test(input)) {
    const [town, state] = input.split(",");
    return townsAndStates
      .filter((row) => {
        return row.town.includes(town) && row.state.includes(state?.trim(" "));
      })
      .slice(0, 5);
  } else {
    return townsAndStates
      .filter((row) => {
        return row.town.includes(input) || row.state.includes(input);
      })
      .slice(0, 5);
  }
};
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

It seems a bit odd that you've defined named capturing groups in your regular expression but then you don't use them because you only use it with the test method, and retrieve the different groups through a subsequent use of split. You could leverage these groups to retrieve the particular town and state values from your search query, since you're already matching your regular expression against it anyway.

(Thanks for using them though, because I didn't recognise the syntax and learned something by looking it up)

There's also some work you could do up front. Currently, you're retrieving your search index and constructing your regular expression fresh each time your function is called. If your search index might change from call to call, then that makes sense, but since retrieving it isn't asynchronous I'm guessing it's a static object so you could move it outside your function and cache the result. You can also call trim the state part just once instead of each time you check if your query matches against an entry in your search index.

As a minor nitpick, I'd recommend using const instead of let for variables with values that never change. This can lower the cognitive load when reading your code, since once you've seen how a variable was defined you typically don't have to keep in mind whether or not it's been modified since then. Really it is a question of code style, but I find it a useful strategy for keeping code clear and readable, and it is a common linting rule.

I think the most impactful way this function could be improved, though, is by not using filter and then slice in order to limit the number of results you're returning. Using this approach means you're still checking your search query against every entry in your index, even if you hit your maximum number of results to return very early on in your search. Instead, you can use a loop to fill up an array of results only until you hit the maximum, at which point you can break out of the loop.

Since you don't seem to have any sort of relevance score you're sorting by, just finding the first results based on the order of entries in your search index, you don't need to match against the entire index before you decide which results to return.

Because your strategy for collecting results is the same for both matching strategies, I think it would be useful to move that branch in your code so it directly affects just the matching strategy. To do that, I've defined an isMatch function that changes based on the structure of the input, and then gets called the same way regardless when the results are being collected. Reducing how deeply nested your code gets can also help with reducing mental load when reading it, and with this approach it's clearer how much the form of the input affects the way the function works - once the matching strategy has been determined, the input's format isn't checked anymore.

I've added a few JSDoc comments, mainly because I'm predominantly a TypeScript developer and I find type definitions useful. But documentation in general is also helpful, and you may like to extend these comments with descriptions, particularly on your searchTownsAndStates function itself if it's likely to be made available elsewhere in your codebase and/or called by other developers. For example, you could describe how the input argument is expected to be structured, or you could use the @example tag to provide an example of how this function could be called.

JSDoc will get picked up automatically by a lot of IDEs, such as VS code, and will provide hover tooltips. Including type information can also help your IDE provide more useful autocomplete suggestions.

I've also pulled the magic number 5 out into an optional parameter maxResults, but if this shouldn't be configurable on the function you could have it as a const variable instead. Even if it never changes, defining magic numbers like this as configuration higher up can make it easier to see at a glance how something behaves, rather than having it defined within the implementation itself.

/**
 * @typedef {Object} TownAndStateIndexEntry
 * 
 * @property {string} town
 * @property {string} state
 */

/**
 * @type {Array<TownAndStateIndexEntry>}
 */
const townsAndStates = getRawTownsAndStates();

const queryPattern = /(?<town>[A-Z ]+),(?<state>[A-Z ]*)/gi;

/**
 * @param {string} input
 * @param {number} maxResults
 */
const searchTownsAndStates = (input, maxResults = 5) => {
  const groups = input.match(queryPattern).groups;

  const town = groups?.town;
  const state = groups?.state?.trim(" ");

  // Determine match strategy based on query structure
  const isMatch = groups
    ? (row) => row.town.includes(town) && row.state.includes(state)
    : (row) => row.town.includes(input) || row.state.includes(input);

  // Collect results
  const results = [];
  for (const row of townsAndStates) {
    if (isMatch(row)) {
      results.push(row);
    }

    if (results.length >= maxResults) {
      break;
    }
  }

  return results;
};
```
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to CodeReview, very nice first answer. I would look in to Array.find as a solution to find the first match. \$\endgroup\$
    – konijn
    Aug 21, 2023 at 11:42

Your Answer

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

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