12
\$\begingroup\$

In my BlogList.vue component I made a search input field:

<input type="text" placeholder="Enter key word  ..." v-model="search">

And a computed property:

computed: {
    getfilteredData() {
     return this.blogs.filter(blog =>
        blog.name.toLowerCase().includes(
          this.search.toLowerCase()
        ) ||
        blog.category.toLowerCase().includes(
          this.search.toLowerCase()
        )
      ) 
    }
  },

The user can search on multiple values, i.e. blog.name and blog.category, which updates the list accordingly.

Can I write the above JavaScript (computed property) in a cleaner way?

\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jun 15 '20 at 17:04
  • \$\begingroup\$ How many blog posts do you search? Hundreds at most wouldn't be a problem but it shouldn't iterate over thousands and thousands of posts. In this case, you'd need another data type (e.g. database, Trie, ...) \$\endgroup\$ – Eric Duminil Jun 17 '20 at 13:03
9
\$\begingroup\$

It is really hard to say, but at least for me, your code is fine. It seems readable and has no issues. Perhaps if you want to change the style and make it more "canonical", you could write instead:

computed: {
    getfilteredData() {
      return this.blogs.filter(blog =>
        blog.name.toLowerCase().includes(this.search.toLowerCase()) ||
          blog.category.toLowerCase().includes(this.search.toLowerCase())
      )
    }
  },

But for me (and I believe, most readers of this post) your code is just fine.

Note: for "canonical" I mean fewer lines of code and more compactness. Clearly, it should be done rationally, and you should not write a 200 characters line. The point is write less and allow readability.

Edit

As Sᴀᴍ Onᴇᴌᴀ answered, it could be substantially optimized if you do:

    getfilteredData() {
      const lowerCaseSearch = this.search.toLowerCase();
      return this.blogs.filter(blog =>
        blog.name.toLowerCase().includes(lowerCaseSearch) ||
          blog.category.toLowerCase().includes(lowerCaseSearch)
      )
    }

because you are creating the lowercase string of your search field only once and using it twice instead of creating it twice and use it twice.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ thanks totally clear! \$\endgroup\$ – meez Jun 15 '20 at 16:52
  • 2
    \$\begingroup\$ If you write the || in front of the following line it gets even clearer, because you are less likely to overlook it. Also, if you have multiple (read many) lines, you can immediately see, if you screwed up an operator. Much easier at least, than if it was at the end of the line. \$\endgroup\$ – infinitezero Jun 16 '20 at 17:51
  • \$\begingroup\$ i would use JavaScripts some array method and avoid joining the "or" || operator \$\endgroup\$ – Robert Rocha Feb 22 at 21:23
13
\$\begingroup\$

One improvement would be factoring out the repeated call to this.search.toLowerCase() and storing the result in a block-scoped constant so it:

  1. doesn't need to be calculated on every call to .includes() for each iteration of filter()
  2. can decrease the length of each line where that value is used

That may be a small optimization but it is in line with the D.R.Y. principle

getfilteredData() {
  const lowerTerm = this.search.toLowerCase()
  return this.blogs.filter(blog =>
    blog.name.toLowerCase().includes(lowerTerm) ||
    blog.category.toLowerCase().includes(lowerTerm)
  ) 
},

Additionally, it could further be simplified by using Object destructuring to eliminate the need for the identifier blog with the callback to the filter method:

getfilteredData() {
  const lowerTerm = this.search.toLowerCase()
  return this.blogs.filter({name, category} =>
   name.toLowerCase().includes(lowerTerm) ||
   category.toLowerCase().includes(lowerTerm)
  ) 
},
\$\endgroup\$
8
\$\begingroup\$

Another way is to create an array from name and category and combine some method with include:

const result = blogs.filter(blog =>
    [blog.name, blog.category].some(s => s.toLowerCase().includes(searchRow.toLowerCase()))
);

It would be very convenient to scale your method by adding into array just necessary property. Let me show an example:

let blogs = [
    { name: "name 1", category: "category 1"},
    { name: "name 2", category: "category 2"},
    { name: "name 3", category: "category 3"},
];

let searchRow = "name 1";
let result = blogs.filter(blog =>
    [blog.name, blog.category].some(s => s.toLowerCase().includes(searchRow.toLowerCase()))
);
console.log(result);

\$\endgroup\$
6
\$\begingroup\$

It doesn't look like this has been suggested yet but you can use object destructuring so you don't have to repeat blog a couple of times.

computed: {
  getfilteredData() {
    const term = this.search.toLowerCase()
    
    return this.blogs.filter(({ name, category }) => {
      return name.toLowerCase().includes(term) || category.toLowerCase().includes(term)
    })
  }
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ You're missing two .toLowerCase() calls though \$\endgroup\$ – Bergi Jun 16 '20 at 18:24
4
\$\begingroup\$

As already mentioned, the code is ok, but if you want to make it cleaner by removing code duplication, you can introduce local variables and helper functions:

getfilteredData() {
  const term = this.search.toLowerCase();
  const isMatched = str => str.toLowerCase().includes(term);
  return this.blogs.filter(blog => isMatched(blog.name) || isMatched(blog.category));
}
\$\endgroup\$

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.