0
\$\begingroup\$

Sample code below - it works, but feels clunky.

clients is provided from a DB as a json_encoded array of objects (in the real app this is several hundred items.)

Objective: filter the list based on user input, which can be a partial case-insensitive string of code or name.

The user knows in advance what the id or name is - this is to quickly reduce the list to possible matches, to save them from scrolling through hundreds.

 
document.addEventListener("DOMContentLoaded", function (event) {
    const clients = [{ id: 1, name: "Bob Marley", code: "BM1" }, { id: 2, name: "Elton John", code:"EJ1" }, { id: 3, name: "Beach Boys", code: "BB1" }, { id: 4, name: "Boyzone", code: "BO1" }];
    const clientsList = document.getElementById("clientsList");
    const search = document.getElementById("search");

    // handle empty client array?

    if (clients.length === 0) {
        clientsList.innerHTML = '<li>No clients were found.</li>';
    } else {

        // populate the list initially
        const listArray = clients.map((element) =>
            '<li><a href="/clients/view/' + element.id + '">' + element.name + '</a></li>'
        ).join("");

        clientsList.innerHTML = listArray;

        search.addEventListener("keyup", function (event) {
            clientsList.innerHTML = clients.filter((value) =>
                value.name.toLowerCase().includes(event.target.value.toLowerCase()) ||
                value.code.toLowerCase().includes(event.target.value.toLowerCase())
            ).map((element) =>
                '<li><a href="/clients/view/' + element.id + '">' + element.name + '</a></li>').join("");
        });
    }
});
<input type="text" id="search" placeholder="Search...">
<ul id="clientsList"></ul>

<!-- Sample input / output
bo => bob marley, beach boys, boyzone
on => elton john, boyzone
bm1 => bob marley -->

I don't like that there is repetition in generating the li element and url - I thought about creating a function and then using that in the map?

const generateListElement = (client_id, name) => `<li><a href="/clients/view/${client_id}">${name}</a></li>`;


...map((element) => generateListElement(element.id, element.name) 

but that doesn't look much better.

good, bad, ugly?

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Remove the outer if else

if (clients.length === 0) {
  clientsList.innerHTML = '<li>No clients were found.</li>';
  return;
}  

Decouple const clients

Make a function. For now clients is hard coded, later rewritten to work with the JSON object. The function call will not change

 const clients = fetchWhateverThatDataIs();

 function fetchWhateverThatDataIs() {
    return [{ id: 1, name: "Bob Marley", code: "BM1" }, { id: 2, name: "Elton John", code:"EJ1" }, { id: 3, name: "Beach Boys", code: "BB1" }, { id: 4, name: "Boyzone", code: "BO1" }];
 }

Objects organize functionality then simplify client code

A Clients object will encapsulate filter and HtmlList.

function Clients (theClients) {
  this.clients = theClients ? theClients : [];
  
  this.isEmpty = function() { return this.clients.length === 0; }

  this.HtmlList = function () {
     return this.clients.map((element) =>
        '<li><a href="/clients/view/' + element.id + '">' + element.name + '</a></li>'
    ).join("");
  } //HtmlList

  // intended as an event handler
  this.filter = function (event) {
    this.clients.filter((value) =>           
      value.name.toLowerCase().includes(event.target.value.toLowerCase()) ||
      value.code.toLowerCase().includes(event.target.value.toLowerCase())
    ).map((element) =>
       '<li><a href="/clients/view/' + element.id + '">' + element.name + '</a></li>').join("");
  } //filter

} //Clients

The <li> building is repeated. For now I'm inclined to follow my rule "repeated once, think about refactoring; repeated twice (or more), refactor." In any case make sure this initial refactoring works first.


it works, but feels clunky

Not any more.

document.addEventListener("DOMContentLoaded", function (event) {
  const clients = new Clients(fetchWhateverThatDataIs());
  const clientsList = document.getElementById("clientsList");
  const search = document.getElementById("search");

  if (clients.isEmpty()) {
    clientsList.innerHTML = '<li>No clients were found.</li>';
    return;
  }

  clientsList.innerHTML = clients.HtmlList();

  search.addEventListener("keyup", clients.filter);

});
\$\endgroup\$
1
  • \$\begingroup\$ wow - very helpful, fhank you! \$\endgroup\$ Commented Oct 20, 2022 at 14:16

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.