3
\$\begingroup\$

I'm making a news aggregator using newsAPI, and have everything working - to begin with. But was wondering if the code I use to filter through an object can be made more efficient - meaning fewer lines of code.

So far, it only returns what I need from the raw JSON file, then filters it using a for loop to ignore objects with blank values.

Any help would be great.

   if (!error && response.statusCode == 200) {

        let data = JSON.parse(body);

        let articles = data.articles.map(article => {
            return {
                "title": article.title,
                "date": article.publishedAt,
                "image": article.urlToImage,
                "description": article.description,
                "link": article.url
            }
        });

        let filtered = []
        for (let i = 0; i < articles.length; i++) {
            if (articles[i].title !== null &&
                articles[i].date !== null &&
                articles[i].image !== null &&
                articles[i].description !== null &&
                articles[i].link !== null) {
                filtered.push(articles[i])
            }
        }

        console.log(filtered)

        res.render("index", { filtered: filtered });

   }
\$\endgroup\$
  • \$\begingroup\$ Too lazy to write an answer. Array to hold property names, filter then map. array.reduce to transform each object. Chain them to avoid intermediates and you are done. const keys = [["title", "title"], ["publishedAt", "date"], ["urlToImage", "image"], ["description", "description"], ["url", "link"]]; res.render("index", { filtered: JSON.parse(body).articles .filter(article => keys.some(key => article[key[0]] !== null)) .map(article => keys.reduce((art, k) => (art[k[1]] = article[k[0]], art), {})) }); BTW your code is missing a closing } \$\endgroup\$ – Blindman67 Aug 3 at 15:45
  • 2
    \$\begingroup\$ Looks like closing brace for the outer if is missing. That begs the question, is there other code missing? \$\endgroup\$ – radarbob Aug 3 at 18:51
  • \$\begingroup\$ Your code could be so much cleaner if you used the same property names throughout your code. Is that possible? \$\endgroup\$ – konijn Aug 5 at 15:10
1
\$\begingroup\$
  • let -> const
  • Use the Array methods (filter, every)
  • Use object destructuring
  • Use object short-hand
  • Use a method for the null check so you don't have to write it for every property
if (!error && response.statusCode == 200) {
  const data = JSON.parse(body);

  const filtered = data.articles
    .filter(({ title, publishedAt, urlToImage, description, url }) =>
      [title, publishedAt, urlToImage, description, url].every(
        prop => prop !== null
      )
    )
    .map(
      ({
        title,
        publishedAt: date,
        urlToImage: image,
        description,
        url: link
      }) => ({
        title,
        date,
        image,
        description,
        link
      })
    );

  res.render("index", { filtered });
}

Update: filtered first for lower memory consumption as by @FreezePhoenix suggestion.

\$\endgroup\$
  • 1
    \$\begingroup\$ You seem to miss that the source properties and the rendered properties do not have the same name \$\endgroup\$ – konijn Aug 5 at 15:08
  • \$\begingroup\$ @konijn Thanks! Fixed. \$\endgroup\$ – Mohrn Aug 5 at 15:09
  • \$\begingroup\$ @konijn Actually still had it wrong 😅 Now it's fixed. \$\endgroup\$ – Mohrn Aug 5 at 15:18
  • 2
    \$\begingroup\$ @FreezePhoenix Not quite sure what you are getting at. Do you mean to lower the space complexity? \$\endgroup\$ – Mohrn Aug 5 at 15:48
  • 1
    \$\begingroup\$ @Mohrn Your new edit in an attempt to incorporate my suggestion errors :) because none of the variables are defined. \$\endgroup\$ – FreezePhoenix Aug 5 at 16:05
0
\$\begingroup\$

Revising @Mohrn's answer:

  • Avoid creating new objects if possible.
  • As mentioned by @radarbob, you are excluding the closing if bracket.

As such, I recommend a few changes be made:

if (!error && response.statusCode == 200) {
  const data = JSON.parse(body);

  const filtered = data.articles
    .map(
      ({
        title,
        publishedAt: date,
        urlToImage: image,
        description,
        url: link
      }) => (
        ([title, date, image, description, link].every(_ => _ !== null) ?
            { title, date, image, description, link } :
            null
      )
    )).filter(_ => _ !== null);

  res.render("index", { filtered });
}

This does result in fewer objects being created per iteration.

\$\endgroup\$
  • \$\begingroup\$ You're returning objects with the shape { title, publishedAt, utlToImage, description, url } rather than { title, date, image, description, link }. The difference between our solutions is that you lack the mapping of property names. \$\endgroup\$ – Mohrn Aug 5 at 15:51
  • \$\begingroup\$ @Mohrn Edited, now the difference is that it doesn't create a second object for ones that don't need to be made. Instead of making the object and then getting values, we're getting values and then making the object if needed. \$\endgroup\$ – FreezePhoenix Aug 5 at 15:54

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.