How to think differently about your code, via an example of how I rescued myself from a horrible function.
TL:DR - Processing different data types separately and thinking hard about the key actions or fundamental operations you want to apply to data really do make for simpler code. In this example, I turned a horrible function with 5 nested forEach loops (and 3 auxillary functions) into 3 simple functions using 3 maps and a single forEach (and 2 auxillary functions).
The general problem
I won't give a full description of how the initial function works, hopefully the general idea will be enough. And why would you pollute your mind with the effort?! Also, the refactoring should make the aim much clearer.
The core problem is one of converting the result of searching in a narrow but deep set of data trees into a simple UI for selecting specific search results with a checkbox. I was using a useful library fuse to run the search and so to some degree was limited by the way it reported results (I did make some customizations).
Steps I wanted to implement:
- Build some UI from a set of simple trees of HTMLElements representing the search results for users to select
- Only add to the simple trees those parts of the data trees that where positive search results
- Loop through each set of search results relevant to each data tree
- Loop through each data tree
The basic data structure to be searched was an array with elements like the following. The search targets where the value keys of the Class3Name children:
{
colName: "Class1Name",
value: "Energy",
children: [
{
colName: "Class2Name",
value: "Electricity",
children: [
{
colName: "Class3Name",
value: "Grid",
children: [
{
colName: "factor",
value: "United Kingdom grid",
children: [
// a few more nested levels followed by a final children[]
// with a list of objects with some data details
]
}
]
}
]
}
]
}
Which was augmented by some search result "matches" that gave me arrays of the indexes of tree children that lead to positive search results.
So whilst trying to get my head around the complicated data structures I had, and contemplating the direct creation of UI, I created a rather imperative solution that successfully did everything at once. I also wrote a small essay of comments to try and explain what on earth was going on.
The Horrible Function
The following code is the eponymous "horrible function", slightly augmented with jsDocs types (see end of article). It also uses some helper functions, not included for brevity.
The trap that I fell into was trying to manipulate the data directly to create UI. It is imperative code that defines how I want the data to be processed.
const treeForUI = buildTree(searchResultsArray) // will be added to the DOM
/**
*
* The data arg will be an array of branches, each of which contain one or more positive search results (matches).
* The problem is that it isn't known which part(s) of the branch was an actual result. So we use the information in the match object.
* The match object contains an array of the specific results and their associated paths along their branch. The paths are described in two complementary ways.
* Recall that a branch has one or more children and multiple levels, hence many 'leaves' or potential search targets.
* Firstly, the sequence of search keys from creating the search library's object are re-used, in match.key.path.
* Secondly the sequence of indexes in each 'children' array that describes the "route" to a search result is in match.path.
* Therefore for each match in a branch, we want to drill down into the branch using the specific route through the path described by the search results.
* @param { searchResult[] } data
* @return { HTMLDivElement }
*/
const buildTree = (data) => {
let tree = document.createElement('div')
data.forEach(branch => {
/** @type { HTMLElement } */
const newLevelOneNode = createEfHtmlTree(branch.item)
branch.matches.forEach(match => {
/** @type { HTMLElement } */
let currentNode = newLevelOneNode
branch.matches.forEach(/** @type { match } */ match => {
/** @type { branch } */
let currentBranch = branch.item
match.path.forEach((p, index) => {
const k = match.key.path[index + 1] // + 1 because have already handled the first level
// process the next branch or a leaf (i.e. emission factor data)
match.path.forEach((childIndex, branchLevel) => { // target the specific index of each branch level's children array
/** @type { boolean } */
const isSearchResult = branchLevel + 1 === match.key.path.length - 1 // have we gotten to the last element in the search yet?
/** @type { branch } */
let branchInfo = {}
if (index + 1 < match.key.path.length - 1) {
branchInfo = currentBranch[k][p]
const branchInfo = buildBranchSearchRoute(currentBranch, match, childIndex, branchLevel, isSearchResult) // build the next level of the search route
if (isSearchResult) { // this will also be the end of the current forEach iteration
// incrementally build the HTML tree to reflect the search tree routes
newLevelOneNode.append (createEfHtmlTree (branchInfo))
} else {
branchInfo.colName = match[k]
branchInfo.value = collateEfDetails(currentBranch.children[p], match[k])
currentBranch = branchInfo // move down the tree in the data
}
/** @type { HTMLElement } */
const nextNode = createEfHtmlTree(branchInfo)
currentNode.append(nextNode)
currentNode = nextNode // move down the tree in the (soon-to-be) DOM
currentBranch = branchInfo // move down the tree in the data
})
})
})
})
tree.append(newLevelOneNode)
return tree
}
So I managed to build all this logic and get it working. It felt like I'd bashed out some code to get a job done then move on to the next thing. But it bothered me so much that I kept picking at the logic to try and see how to refactor it.
Refactoring
Some steps that got me to what I think is a much better solution:
- Call
buildTreein a map, to take out the first level offorEachloops. But I was confounded by the types, as it returns HTMLElements that don't sit well with a regular array. So I decided the data types needed to be dealt with separately - search results and UI. - Consider the operation to select the successful search results as a filter operation. Here, I considered that the process of keeping the "paths" through each tree's branches and children based on search success/fail is basically a filter. But not a straightforward one as would need to be an iterative/recursive filter down the lists of children.
- In fact, focusing on the search results separately from UI meant that I could build new successful-hits-only search data via a map then use this to create UI. The clarity provided by separated data and the filter concept led to a better solution.
So I guess I was finally following some basic data management principles of separating types out and thinking more about functional operations rather than direct UI creation.
The resulting code is spread over several functions but hopefully provides a much more obvious logic.
const collatedSearches = searchResultsArray.map(collateSearchResults)
const searchNodes = collatedSearches.map(buildSearchResultNodes) // will be added to the DOM
/**
* This function acts as a sort-of filter, only returning data for those child elements that appeared in the successful search "matches"
* @param { searchResult } searchResult
* @return { collatedSearch }
*/
const collateSearchResults = (searchResult) => {
return {
"colName": searchResult.item.colName,
"value": searchResult.item.value,
"efDetailsList": searchResult.matches.map(/** @type { match } */ match => {
const searchResultLocation = moveAlongSearchPath(searchResult.item, match.path)
return collateEfDetails(searchResultLocation)
})
}
}
/**
* Follow a search path recursively down a branch
* @param { branch } branch
* @param { number[] } path
* @return { branch }
*/
const moveAlongSearchPath = (branch, path) => {
if (path.length < 1) {
return branch
} else {
return moveAlongSearchPath(branch.children[path[0]], path.slice(1))
}
}
/**
* Build a simple UI structure from a list of successful searches
* @param { collatedSearch } collatedSearchResult
* @return { HTMLDivElement }
*/
const buildSearchResultNodes = (collatedSearchResults) => {
const rootElement = createEfHtmlTree(collatedSearchResults) // need to create top-level for Class1Name
collatedSearchResults.efDetailsList.forEach((ef) => { // add each ef from the list
const nextElement = createEfHtmlTree(ef) // create a checkbox element
rootElement.append(nextElement)
})
return rootElement
}
The jsDocs types used:
/**
* @typedef { object } searchResult
* @property { branch } item
* @property { number } refIndex
* @property { match[] } matches
*/
/**
* @typedef { object } branch
* @property { branch[] } children
* @property { string } colName
* @property { string | efValue } value
* @property { number } [efCount]
*/
/**
* @typedef { object } match
* @property { number } idx
* @property { number | undefined } indices
* @property { pathKey } key
* @property { number } norm
* @property { number[] } path
* @property { number } score
* @property { string } value
*/
/**
* @typedef { object } collatedSearch
* @property { string } colName
* @property { string } value
* @property { efValue[] } efDetailsList
*/
/**
* @typedef { object } efValue
* @property { string } name
* @property { number | string | boolean } cf
* @property { string } unit
*/
What puzzles me still as a learning developer, however, is whether given the same sort of task again I'd now leap straight to the "clean" reasonably functional approach, or still have to start imperatively.
Top comments (0)