1
\$\begingroup\$

I've just implemented a search box, so I can search on employee object by (name, part of the name, phone, email ...). Ex. if the name: Moamen Abdulraouf Mansour, I can search with ( moamen | moa | moamen mansour ...). The Idea I've followed is:

  • split some values from the object to be not included in the search.
  • convert each object of the array to one string,
  • then replace extra spaces and some special Arabic letters with another letter (آ أ إ => ا) and so on.
  • then convert the search text string to an array of words.
  • After that, going through each object and find if it includes all words on the search text
  • finally, return this object to be the result.

the JSON looks:

{
      "id": "0",
      "ORGANIZATION_NAME": "ORGANIZATION NAME",
      "ORGANIZATION_NAME_LINK": "<a href='...' target='_blank' title='..'>....</a>",
      "ORGANIZATION_TYPE_CODE": "MIN",
      "ORGANIZATION_TYPE": "type",
      "pid": "99999",
      "ORIGINAL_ORGANIZATION_LEVEL": 0,
      "DeptUrl": "MI0",
      "IsCreated": true,
      "ManImageUrl": "https://....jpg",
      "EMPLOYEECODE": "90000419",
      "EMP_NO_ERP": "90000419",
      "TITLE": "السيد",
      "USER_FULL_NAME": "employee name",
      "USER_POSITION_TITLE": "title",
      "USER_EMAIL": "[email protected]",
      "SEX_CODE": "M",
      "USER_EXTENSION": "4999",
      "USER_MOBILE": "1234",
      "USER_HOME_PHONE": "1234"
    },...

and this is the JS:

let searchBtn = document.getElementById('searchBtn');
let myList = document.getElementById('myList');
let arr = [];
let searchKeyword;
var resultsSize = 5;

async function getUserAsync(keywords) {
  let response = await fetch('http://localhost:3000/users');
  let data = await response.json();
  return data;
}

searchBtn.addEventListener('keyup', (e) => {
  // get serach text, remove spaces from start and end and remove more than one space between, then convert to array of words
  searchKeyword = e.target.value
    .replace(/\s+/g, ' ')
    .replace(/[\u0623|\u0622|\u0625]/g, 'ا')
    .replace(/[\u0649]/g, 'ي')
    .replace(/[\u0629]/g, 'ه')
    .trim()
    .split(' ');

  getUserAsync(searchKeyword).then((data) => {
    arr = data;

    if (searchKeyword[0].length > 2) {
      let filtered = arr.filter(function (el) {
        let {
          id,
          DeptUrl,
          pid,
          IsCreated,
          TITLE,
          SEX_CODE,
          ManImageUrl,
          ORGANIZATION_TYPE,
          ORGANIZATION_TYPE_CODE,
          ORIGINAL_ORGANIZATION_LEVEL,
          fortags,
          ...rest
        } = el;

        let elCompind = Object.values(rest)
          .join()
          .replace(/\s+/g, ' ')
          .replace(/[\u0623|\u0622|\u0625]/g, 'ا')
          .replace(/[\u0649]/g, 'ي')
          .replace(/[\u0629]/g, 'ه');

        let checkAllWords = searchKeyword.map((item) => {
          return elCompind.includes(item);
        });

        return checkAllWords.includes(false) ? false : true;
      });
      appendFiltered(filtered.slice(0, resultsSize));
    } else {
      appendFiltered([]);
    }
  });
});

function appendFiltered(filteredArray) {
  // reset container HTML
  myList.innerHTML = '';

  filteredArray.forEach((element) => {
    // console.log(element);
    let node = document.createElement('div'); // Create a node

    node.innerHTML = `
      <ul>
        <li>${element.USER_FULL_NAME}</li>
        <li>${element.ORGANIZATION_NAME}</li>
        <li>${element.USER_EMAIL}</li>
        <li>${element.USER_POSITION_TITLE}</li>
      </ul>
    `;
    myList.appendChild(node); // Append to body
  });
}

the code works well, but I believe this code can be improved.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a follow-up question instead. \$\endgroup\$
    – Mast
    Commented Sep 13, 2020 at 13:12

1 Answer 1

3
\$\begingroup\$

You didn't post your HTML, but the element with the id searchBtn doesn't seem to be a button, so it shouldn't be called "btn" in it's id or variable name.

There are many variables that don't change their value, so you should be using const instead of let (or var).

The variables arr and searchKeyword are only used inside the event handler, so they should be declared in there and not outside.

The function getUserAsync doesn't use its parameter.

You should use the event input instead of keyup, because the value of the input can change without a keypress.

The same .replace calls are used on the keywords and the content being searched, so that should be extracted into their out function.

The requirement that only the first keyword must be longer than two characters seems a bit arbitrarily. The user won't get any results, even if there are more valid (longer) keywords, but they will get results from invalid (shorter) keywords if they are not in the first position. It probably would make more sense to filter out words shorter than three out of the list of keywords.

Using a deconstructing assignment to create variables that you don't use and in order to exclude certain object properties seems strange. It would probably make more sense to do something like:

const IGNORED_PROPERTIES = ["id", "DeptUrl", "pid" /* etc. */ ]; 
const elCompind = Object.entries(el)
  .filter(([k, v]) => !IGNORED_PROPERTIES.includes(k))
  .map(([k, v]) => v)
  .join()
  /* etc. */;

The expression checkAllWords.includes(false) ? false : true can be simplified to !checkAllWords.includes(false), however it is not necessary at all, because instead of mapping all keywords to a boolean and then checking of there is a false item, you can use the .every array method. This also has the advantage, that it will stop once a single false has been found and won't necessarily check more keywords:

return searchKeyword.every(item => elCompind.includes(item)});

(Also notice the simplified lambda.)

\$\endgroup\$
0

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.