6
\$\begingroup\$

I've been following Wes Bos challenge, I wrote the code myself then looked at his finished example. Here's my code (I only wrote the JS):

const cities = [];

const endpoint = 'https://gist.githubusercontent.com/Miserlou/c5cd8364bf9b2420bb29/raw/2bf258763cdddd704f8ffd3ea9a3e81d25e2c6f6/cities.json';
fetch(endpoint)
  .then(blob => blob.json())
  .then(json => cities.push(...json))
  .catch(err => console.log(err.message));

const input = document.querySelector('.search');
const suggestions = document.querySelector('.suggestions');

function findMatches(word, cities) {
  const regex = new RegExp(word, 'gi');
  return cities.filter(city => city.city.match(regex) || city.state.match(regex));
}

function displayMatches(value) {
  const matchingCities = findMatches(value, cities);
  for (const city of matchingCities) {
    const spanCity = document.createElement('span');
    spanCity.setAttribute('class', 'name');

    const regex = new RegExp(value, 'gi');
    const cityName = `${city['city']}, ${city['state']}`.replace(regex, `<span class="hl">${value}</span>`);
    spanCity.innerHTML = cityName;

    const spanPopulation = document.createElement('span');
    spanPopulation.setAttribute('class', 'population');
    spanPopulation.textContent = numberWithCommas(city['population']);

    const li = document.createElement('li');
    li.appendChild(spanCity);
    li.appendChild(spanPopulation);

    suggestions.appendChild(li);
  }
}

function numberWithCommas(x) {
  return x.toString().replace(/\B(?=(\d{3})+(?!\d))/g, ',');
}

input.addEventListener('input', e => {
  while (suggestions.lastChild) {
    suggestions.removeChild(suggestions.lastChild);
  }

  const value = input.value.toLowerCase();
  const isEmpty = value.length === 0;
  if (isEmpty) {
    const li1 = document.createElement('li');
    li1.textContent = 'Filter for a city';
    suggestions.appendChild(li1);
    const li2 = document.createElement('li');
    li2.textContent = 'or a state';
    suggestions.appendChild(li2);
    return;
  }

  displayMatches(value);
});
html {
  box-sizing: border-box;
  background: #ffc600;
  font-family: 'helvetica neue';
  font-size: 20px;
  font-weight: 200;
}

*,
*:before,
*:after {
  box-sizing: inherit;
}

input {
  width: 100%;
  padding: 20px;
}

.search-form {
  max-width: 400px;
  margin: 50px auto;
}

input.search {
  margin: 0;
  text-align: center;
  outline: 0;
  border: 10px solid #F7F7F7;
  width: 120%;
  left: -10%;
  position: relative;
  top: 10px;
  z-index: 2;
  border-radius: 5px;
  font-size: 40px;
  box-shadow: 0 0 5px rgba(0, 0, 0, 0.12), inset 0 0 2px rgba(0, 0, 0, 0.19);
}

.suggestions {
  margin: 0;
  padding: 0;
  position: relative;
}

.suggestions li {
  background: white;
  list-style: none;
  border-bottom: 1px solid #D8D8D8;
  box-shadow: 0 0 10px rgba(0, 0, 0, 0.14);
  margin: 0;
  padding: 20px;
  transition: background 0.2s;
  display: flex;
  justify-content: space-between;
  text-transform: capitalize;
}

.suggestions li:nth-child(even) {
  transform: perspective(100px) rotateX(3deg) translateY(2px) scale(1.001);
  background: linear-gradient(to bottom, #ffffff 0%, #EFEFEF 100%);
}

.suggestions li:nth-child(odd) {
  transform: perspective(100px) rotateX(-3deg) translateY(3px);
  background: linear-gradient(to top, #ffffff 0%, #EFEFEF 100%);
}

span.population {
  font-size: 15px;
}

.hl {
  background: #ffc600;
}
<form class="search-form">
  <input type="text" class="search" placeholder="City or State">
  <ul class="suggestions">
    <li>Filter for a city</li>
    <li>or a state</li>
  </ul>
</form>

Please review it.

And one question. In his finished code, displayMatches() function looks like this. I wonder which option is better (of course this one is shorter):

function displayMatches() {
  const matchArray = findMatches(this.value, cities);
  const html = matchArray.map(place => {
    const regex = new RegExp(this.value, 'gi');
    const cityName = place.city.replace(regex, `<span class="hl">${this.value}</span>`);
    const stateName = place.state.replace(regex, `<span class="hl">${this.value}</span>`);
    return `
          <li>
            <span class="name">${cityName}, ${stateName}</span>
            <span class="population">${numberWithCommas(place.population)}</span>
          </li>
        `;
  }).join('');
  suggestions.innerHTML = html;
}

\$\endgroup\$
4
\$\begingroup\$

Asynchronous loading The population of the cities variable looks suspicious because all it does is push to an array after the request finishes. On seeing that, readers of the code could very easily worry that the script, once run, isn't going to run in the right order due to the extremely common bug caused by a variable being populated asynchronously. Your code happens to work anyway, but it looks worrying on the first glance.

That aside, what if the githubusercontent endpoint is not reachable? Then the script will appear to do nothing when stuff is typed in, and the user will be confused.

Fix it by introducing something like a loading screen, which indicates to the user that data is being fetched, and display an error if fetching fails. (Maybe add a check to see if fetching takes an unexpectedly long amount of time and show an error in that case too.)

Responses aren't blobs You do fetch(endpoint).then(blob => blob.json()). The variable named blob is actually a Response object. (You could get a Blob out of the Response stream if you called .blob() on it, but you're calling .json() on it.) Probably best to change the variable name to response or something like it.

JSON is a string format You do .then(json => cities.push(...json)). JSON is a way of representing data in string format. After deserialization, what you have is no longer JSON - it's just a plain array or object. This is a common issue. There's no such thing as a JSON object. I'd call it something like responseCities instead.

Regular expression special characters Some characters have a special meaning in regular expressions. For example, . will match any character, and foo+ will not match foo followed by a literal plus, but foo followed by one or more os. Whenever using new RegExp, make sure you're accounting for these sorts of cases, else you'll probably run into problems when unexpected characters are encountered. For example, someone searching for st. will see the following:

enter image description here

To escape special characters, you can use .replace to escape the input before passing it to the RegExp constructor: https://stackoverflow.com/q/3561493

If you were just just trying to see if one string exists inside another, without case sensitivity, I think the code would be easier to understand if you called toLowerCase() on both strings and then used .includes. But since you want to insert a <span> at certain points, there's no getting around a regular expression somewhere.

To simplify it a bit, consider using a regular expression just once, rather than twice - take the value and try to replace it with the <span>s. If the resulting string is different from the original string, it's a match, so you can use the result; otherwise, if the resulting string is the same, it's not a match.

cityName? You concatenate the city and state together when creating the string to display to the user. Rather than call it cityName, consider calling it locationName - it's not just a city anymore, after all.

for (const city of cities) {
  const baseLocationName = `${city['city']}, ${city['state']}`;
  const highlightedLocationName = baseLocationName.replace(
    new RegExp(escapeRegex(value), 'gi'),
    `<span class="hl">${value}</span>`
  );
  if (highlightedLocationName !== baseLocationName) {
    insertLocation(highlightedLocationName, city);
  }
}

More concise object navigation You can change:

spanCity.setAttribute('class', 'name') to spanCity.className = 'name'; (or remove this class entirely, since the class name isn't referenced anywhere else)

city['population'] to city.population (dot notation looks nicer and is probably preferable when possible)

Clearing the HTML can be done easily by just setting the textContent of the container to the empty string - no need to iterate over the lastChilds until no more exist.

Placeholder text When you're waiting for the user to start typing, rather than inserting elements with document.createElement / appendChild / textContent and so on (which is a bit verbose), consider just setting the HTML content.

suggestions.innerHTML = `
  <li>Filter for a city</li>
  <li>or a state</li>
`;

Suggestion limit There are a lot of possible cities. If more than, say, 20 get rendered, the page gets quite large and can take a while to load. Consider rendering only 20 at most.

Implementing all of these gives you:

const endpoint = 'https://gist.githubusercontent.com/Miserlou/c5cd8364bf9b2420bb29/raw/2bf258763cdddd704f8ffd3ea9a3e81d25e2c6f6/cities.json';

const setupListener = (cities) => {
  const escapeRegex = s => s.replace(/[-\/\\^$*+?.()|[\]{}]/g, '\\$&');
  const input = document.querySelector('.search');

  function displayMatches(value) {
    let matchCount = 0;
    for (const city of cities) {
      const baseLocationName = `${city['city']}, ${city['state']}`;
      const highlightedLocationName = baseLocationName.replace(
        new RegExp(escapeRegex(value), 'gi'),
        `<span class="hl">${value}</span>`
      );
      if (highlightedLocationName !== baseLocationName) {
        insertLocation(highlightedLocationName, city);
        matchCount++;
        if (matchCount >= 20) {
          return;
        }
      }
    }
  }
  function insertLocation(locationNameHTML, city) {
    const li = suggestions.appendChild(document.createElement('li'));
    const locationSpan = li.appendChild(document.createElement('span'));
    locationSpan.innerHTML = locationNameHTML;
    const locationPopulationSpan = li.appendChild(document.createElement('span'));
    locationPopulationSpan.className = 'population';
    locationPopulationSpan.textContent = numberWithCommas(city.population);
  }

  function numberWithCommas(x) {
    return x.toString().replace(/\B(?=(\d{3})+(?!\d))/g, ',');
  }
  const handleInput = () => {
    suggestions.textContent = '';

    const value = input.value.toLowerCase();
    const isEmpty = value.length === 0;
    if (!isEmpty) {
      displayMatches(value);
      return;
    }
    suggestions.innerHTML = `
      <li>Filter for a city</li>
      <li>or a state</li>
    `;
  };
  input.addEventListener('input', handleInput);
  // Remove the loading screen:
  handleInput();
};


const suggestions = document.querySelector('.suggestions');
fetch(endpoint)
  .then(response => response.json())
  .then(setupListener)
  .catch(err => {
    suggestions.textContent = 'There was an unexpected error: ' + err.message;
  });
html {
  box-sizing: border-box;
  background: #ffc600;
  font-family: 'helvetica neue';
  font-size: 20px;
  font-weight: 200;
}

*,
*:before,
*:after {
  box-sizing: inherit;
}

input {
  width: 100%;
  padding: 20px;
}

.search-form {
  max-width: 400px;
  margin: 50px auto;
}

input.search {
  margin: 0;
  text-align: center;
  outline: 0;
  border: 10px solid #F7F7F7;
  width: 120%;
  left: -10%;
  position: relative;
  top: 10px;
  z-index: 2;
  border-radius: 5px;
  font-size: 40px;
  box-shadow: 0 0 5px rgba(0, 0, 0, 0.12), inset 0 0 2px rgba(0, 0, 0, 0.19);
}

.suggestions {
  margin: 0;
  padding: 0;
  position: relative;
}

.suggestions li {
  background: white;
  list-style: none;
  border-bottom: 1px solid #D8D8D8;
  box-shadow: 0 0 10px rgba(0, 0, 0, 0.14);
  margin: 0;
  padding: 20px;
  transition: background 0.2s;
  display: flex;
  justify-content: space-between;
  text-transform: capitalize;
}

.suggestions li:nth-child(even) {
  transform: perspective(100px) rotateX(3deg) translateY(2px) scale(1.001);
  background: linear-gradient(to bottom, #ffffff 0%, #EFEFEF 100%);
}

.suggestions li:nth-child(odd) {
  transform: perspective(100px) rotateX(-3deg) translateY(3px);
  background: linear-gradient(to top, #ffffff 0%, #EFEFEF 100%);
}

span.population {
  font-size: 15px;
}

.hl {
  background: #ffc600;
}
<form class="search-form">
  <input type="text" class="search" placeholder="City or State">
  <ul class="suggestions">App is loading...</ul>
</form>

And one question. In his finished code, displayMatches() function looks like this. I wonder which option is better (of course this one is shorter):

I strongly prefer your version because it's safer. Direct concatenation of input to create an HTML string can result in arbitrary code execution, which is a security risk. If the data happened to come from an API, and you didn't trust the API 100%, there could be problems.

His code could be fixed by assigning to the .textContent of the <span>s afterwards, instead of interpolating.

\$\endgroup\$
  • \$\begingroup\$ thanks for valuable comments. i have a few questions: 1) i create async function fetchData(endpoint) { const res = await fetch(endpoint); if(!res.ok) throw new error('Unable to fetch data.');, and when endpoint is not reachable, why this error isn't returned? instead there's TypeError: Failed to fetch 2) i remember someone advised to never ever use innerHTML. how else can i bypass it? \$\endgroup\$ – user3132457 Nov 2 '20 at 16:38
  • 1
    \$\begingroup\$ Failed to fetch will be thrown if the request can't get started in the first place - the await fetch(..) threw, so it never got to the res.ok check. There is absolutely nothing wrong with innerHTML as long as the text is trustworthy and you don't interpolate (both of which are often easy to manage) - but if you really didn't want to use it for some reason, you could go back to the createElement / appendChild methods in your original code. \$\endgroup\$ – CertainPerformance Nov 2 '20 at 16:46
  • \$\begingroup\$ how should i catch await fetch(..) then? \$\endgroup\$ – user3132457 Nov 2 '20 at 16:53
  • 1
    \$\begingroup\$ I wouldn't bother, I'd use .then instead of await, and put a .catch at the end of the Promise chain to handle any errors thrown above, like in my answer. (adding in the res.ok check too, if you want) \$\endgroup\$ – CertainPerformance Nov 2 '20 at 16:56
  • 1
    \$\begingroup\$ Works, but it'd be better to initialize listeners once the response comes back instead of just pushing to the cities array, and described in the answer. Also .then(arr => storeCities(arr)) simplifies to .then(storeCities) \$\endgroup\$ – CertainPerformance Nov 2 '20 at 17:23
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ thanks for your helpful feedback. \$\endgroup\$ – user3132457 Nov 2 '20 at 16:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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