Thank you for sharing your code. I have written an implementation of my own and I will comment below on the places where my design decisions diverged from yours.
Use Promises for Asynchronous Control Flow
When fetching data asynchronously - as cities is fetched - I would use a Promise object to handle the asynchronous flow. As your code is currently, there is technically a race condition because there is code that assumes that cities is fully-populated without checking if the fetch has actually completed.
I would assign the Promise returned by fetch to a variable and then put any code that relies on the value of cities into a .then handler. In the case of this example, the change in application behavior may be negligible, but I still like this approach because it is more explicit and correct. It allows me, as a programmer reading the code, to know that cities is fetched asynchronously without my having to search through the code to see when/how the Array is populated.
The code becomes:
const getCitiesPromise = fetch('https://gist.githubusercontent.com/Miserlou/c5cd8364bf9b2420bb29/raw/2bf258763cdddd704f8ffd3ea9a3e81d25e2c6f6/cities.json')
.then(response => {
if (!response.ok) { throw new Error('Failed to fetch cities'); }
return response.json();
})
.catch(err => {
console.error(err);
});
Notice, also, that I omitted the json => cities.push(...json). I find this to be unnecessary work to push each element of the json array onto the cities array. json is already the array of cities we want, so why not just point cities to it, as in: cities = json?
Decouple Functions
If I could change one thing about this code, I would take the findMatches call out of displayMatches. By having displayMatches call findMatches, it makes the application more difficult to debug. If, for some reason, we were not rendering any matches, it would be difficult to determine if the problem were with the finding or the displaying. Ideally, we would want to be able to feed a hard-coded list of cities to displayMatches so that we could quickly assess where our issue lay. Additionally, a separation would allow us to feed a hard-coded search term to findMatches and simply console.log the output without needing the DOM. This is much easier both to debug and to unit-test.
In my implementation, I split these into functions called "findMatches" and "renderCitiesList", and they look like:
const findMatches = (cities, searchTerm) => {
if (!searchTerm) { return []; }
const regex = new RegExp(searchTerm, 'gi');
return cities.filter(city => city.city.match(regex) || city.state.match(regex));
};
const renderCitiesList = ($el, cities, searchTerm) => {
const regex = new RegExp(`(${searchTerm})`, 'gi');
const html = cities.map(city => {
const formattedCity = formatCity(city).replace(regex, '<strong>$1</strong>');
return `<li>${formattedCity} - ${formatPopulation(city.population)}</li>`;
}).join('');
$el.innerHTML = html || '';
};
I decided to make these functions more "functional" by having them take cities as a parameter to facilitate the debugging and unit-testing as I mentioned above. My render function does, however, rely on some formatting functions, which I shall discuss later.
Avoid Unnecessary Loops
The most curious piece of the code to me was the while loop in the input event listener. There is no need to remove the child nodes of the suggestions element one at a time. suggestions.innerHTML = '' would do the trick in one fell swoop.
In my implementation, I did not even include code to explicitly empty the suggestions element (which I called $cities). My renderCitiesList function (above) maps cities to formatted <li> strings and injects the concatenated result into the $cities element. If there are no cities, an empty string is injected.
As the matching and rendering had already been decoupled into independent functions, the only duty left to my event listener was to invoke the find and pass the result to the render:
const $search = document.getElementById('Search');
$search.addEventListener('input', () => {
const searchTerm = ($search.value || '').toLowerCase();
getCitiesPromise.then((cities) => {
renderCitiesList($cities, findMatches(cities, searchTerm), searchTerm);
});
});
It is worth noting that I did not include the default messaging, "Filter for a city or a state", but, if I had, I would have done this in the renderCitiesList function as it is presentation logic.
Function Names should describe the What, Not the How
I did not like the name "numberWithCommas" for the function that formats the population number. If we ever wanted to change how we formatted this number - say, we wanted to use spaces instead of commas - then we would need to rename our function because it would become a lie. This is why we want to give our functions names that describe what they do, but not how they do it. A function name like "formatPopulation" could be repeatedly updated with new formatting rules and its name would remain honest and descriptive.
Additionally, in my implementation I avoided the Regular Expression. I am not saying that the Regular Expression is bad, I just found it a little tricky for my tastes and I wanted to see if I could arrive at a solution that was succinct and simple. Basically, I used String.prototype.slice to get the parts of the value that needed to be separated into, pushed them to an Array, and then joined them with a ",".
const formatPopulation = population => {
const parts = [];
for (let i = population.length; i > 0; i -= 3) {
parts.push(population.slice(Math.max(0, i - 3), i));
}
return parts.reverse().join(',');
};
Extra Credit
I thought it would improve the UI if the returned list of matched cities was sorted alphabetically. As I was already handling the cities with a Promise, this just required adding an extra then handler to do the sorting*:
const getCitiesPromise = fetch('https://gist.githubusercontent.com/Miserlou/c5cd8364bf9b2420bb29/raw/2bf258763cdddd704f8ffd3ea9a3e81d25e2c6f6/cities.json')
.then(response => {
if (!response.ok) { throw new Error('Failed to fetch cities'); }
return response.json();
})
.then(cities => {
return cities.sort((city1, city2) => {
const formattedCity1 = formatCity(city1);
const formattedCity2 = formatCity(city2);;
if (formattedCity1 < formattedCity2) { return -1; }
if (formattedCity1 > formattedCity2) { return 1; }
return 0;
});
})
.catch(err => {
console.error(err);
});
I wanted to ensure that the cities were rendered in sorted order (alphabetically, ascending). For this reason, I needed to make sure that the compare function was comparing cities in the same format as they were being rendered. For this reason, I created a formatCity function so that both the sort and the renderCitiesList functions could use the same logic without code duplication.
The Result
My completed solution is as follows. Note that I implemented only the JavaScript and did not concern myself with the styling.
const $cities = document.getElementById('Cities');
const $search = document.getElementById('Search');
const findMatches = (cities, searchTerm) => {
if (!searchTerm) { return []; }
const regex = new RegExp(searchTerm, 'gi');
return cities.filter(city => city.city.match(regex) || city.state.match(regex));
};
const formatCity = city => {
return `${city.city}, ${city.state}`;
};
const formatPopulation = population => {
const parts = [];
for (let i = population.length; i > 0; i -= 3) {
parts.push(population.slice(Math.max(0, i - 3), i));
}
return parts.reverse().join(',');
};
const renderCitiesList = ($el, cities, searchTerm) => {
const regex = new RegExp(`(${searchTerm})`, 'gi');
const html = cities.map(city => {
const formattedCity = formatCity(city).replace(regex, '<strong>$1</strong>');
return `<li>${formattedCity} - ${formatPopulation(city.population)}</li>`;
}).join('');
$el.innerHTML = html || '';
};
const getCitiesPromise = fetch('https://gist.githubusercontent.com/Miserlou/c5cd8364bf9b2420bb29/raw/2bf258763cdddd704f8ffd3ea9a3e81d25e2c6f6/cities.json')
.then(response => {
if (!response.ok) { throw new Error('Failed to fetch cities'); }
return response.json();
})
.then(cities => {
return cities.sort((city1, city2) => {
const formattedCity1 = formatCity(city1);
const formattedCity2 = formatCity(city2);;
if (formattedCity1 < formattedCity2) { return -1; }
if (formattedCity1 > formattedCity2) { return 1; }
return 0;
});
})
.catch(err => {
console.error(err);
});
$search.addEventListener('input', () => {
const searchTerm = ($search.value || '').toLowerCase();
getCitiesPromise.then((cities) => {
renderCitiesList($cities, findMatches(cities, searchTerm), searchTerm);
});
});
I have created a fiddle for reference.
As to your question of whose implementation of displayMatches is better: Wes Bos's displayMatches is very similar to mine. Ours differ from yours in two ways. First, we use Array.prototype.map to map matched cities to HTML element strings whereas you use a for...of loop. Array.prototype.map is a more "functional" way to produce the output and I prefer it for all the standard reasons the advocates of functional programming emphasize. What I particularly like about this implementation is that, as it just spits-out a String without manipulating the DOM, I can console.log the result to do my debugging and can completely ignore what is rendered to the page. The second difference is that Wes and I are creating an HTML string and using Element.innerHTML whereas you are calling Node.appendChild for each item in your for...of. I find the former cleaner for its lack of .setAttributes and .textContents. Also, as yours updates the DOM twice for each match (.appendChild is called twice in the loop body), it is possible that it is less performant. The performance impact is probably negligible (one would need to measure to be sure); it is the readability that guides my design decision.
Frankly, it is the call to findMatches that I dislike most about yours and Wes's implementations. For the reasons I stated above, I think these actions are best left independent (decoupled). I think code is most comprehensible when it decisively separates the processing from the rendering.
Thank you. I hope you are able to find something in my response that helps you.
* I owe a Thank You to the author of this StackOverflow post for a refresher on sorting Arrays of Strings in JavaScript.
