1
\$\begingroup\$

I spent a good couple hours troubleshooting a bug where the yelp-fusion api said I was sending too many requests per second, so I finally found a way to delay each api request to make it work. Please take a look at the code and give me any tips you have. I'd like to see how this could be done in a better way if possible.

Here is my server.js where post requests come through:

  //handle post requests for getting yelp data
  app.post('/post', async function(req, res) {
    let yelpInData = []
    let result = null;
    req.body.forEach((item) => {
      yelpInData.push(item);
    });

    result = await handleYelp(yelpInData);
    //send data back to the front end
    res.send(result);

  });

  //handle yelp api
  async function handleYelp(yelpInData) {
    let yelpData = [];

    for (let i = 0, len = yelpInData.length; i < len; i++) {
      let result = await searchPlace(yelpInData[i]);
      yelpData.push(result);
    }
    //  console.log(yelpData);
    return yelpData;

  }

  function searchPlace(city) {
    //console.log(city)
    return new Promise((resolve) => {
      setTimeout(() => {
        let coords = city.coords.lat + "," + city.coords.lng;
        let name = city.name;

        client.search({term: name, location: coords}).then(response => {

          //name, img url, review count, rating, price, location.display address, is closed,phone
          resolve({
            name: response.jsonBody.businesses[0].name,
            img: response.jsonBody.businesses[0].image_url,
            hours: response.jsonBody.businesses[0].is_closed,
            revcount: response.jsonBody.businesses[0].review_count,
            rating: response.jsonBody.businesses[0].rating,
            price: response.jsonBody.businesses[0].price,
            location: response.jsonBody.businesses[0].location.display_address.toString(),
            phone: response.jsonBody.businesses[0].display_phone,
            url: response.jsonBody.businesses[0].url
          });

        }).catch(e => {
          console.log(e);
        });
      }, 100);
    })
  }

Is setTimeout a good idea for delaying the api requests to yelp or is there a different way to delay? Here is the git repo if you want to run the web app: https://github.com/capozzic1/knockout_map

\$\endgroup\$

2 Answers 2

Reset to default
2
\$\begingroup\$

setTimeout for a delay is as good as any other approach if you're happy to keep the load on the server. Other option is to separate the processing from the scheduling e.g. offload those particular requests to a scheduling service and then drip feed those back into your app for processing.

From a code point of view, there are some small things you can do to tidy it up a bit.... but the biggest issue I see with your code at the moment is there is absolutely zero error handling. Express does have a default error handler which conveniently catches an errors thrown inside middleware, however, just be wary that you do run the risk of leaking internal server details to the client if you let you just leave your errors to bubble up.

Here's a bit of a tidy up & better error handling

// handle post requests for getting yelp data
app.post('/post', async (req, res, next) => {
  try {
    // nice use of map makes handleYelp redundant :)
    const searches = req.body.map(x => searchPlace(x));
    const results = await Promise.all(searches);
    return res.send(results);
  } catch (e) {
    console.error(e); // log internal error
    return next(new Error('Internal Server Error')); // return public error to client
  }
});

function searchPlace(city) {
  return new Promise((resolve, reject) => {
    // use async / await here too :)
    setTimeout(async () => {
      try {
        const name = city.name;
        const location = `${city.coords.lat},${city.coords.lng}`;
        const result = await client.search({ name, location });
        const business = result.jsonBody.businesses[0];
        resolve({
          name: business.name,
          img: business.image_url,
          hours: business.is_closed,
          revcount: business.review_count,
          rating: business.rating,
          price: business.price,
          location: business.location.display_address.toString(),
          phone: business.display_phone,
          url: business.url
        });
      } catch (e) {
        return reject(e);
      }
    }, 100);
  })
}
\$\endgroup\$
5
  • \$\begingroup\$ Wow this is mind-blowing. That use of .map is great. I'm definitely going to refactor like this. \$\endgroup\$ Commented Feb 14, 2018 at 0:25
  • \$\begingroup\$ I refactored to this and for some reason, I'm getting the 'too many requests per second' error even when I increase the milliseconds on setTimeout. At this point, I'm not sure if its just the yelp fusion api issue. \$\endgroup\$ Commented Feb 14, 2018 at 1:36
  • \$\begingroup\$ @Dream_Cap ah I think I see why, the speed at which the searches are queued they will still end up firing at approx. the same time. How many requests per second are you allowed? You can split the searches into the appropriate batch size. \$\endgroup\$ Commented Feb 14, 2018 at 8:06
  • \$\begingroup\$ I'm not sure if you are using apollo & graphql as the express-to-go module (or intend to) from what i saw in your repo, but did you consider batching multiple searches into one request? The following link illustrates what i mean by it: dev-blog.apollodata.com/query-batching-in-apollo-63acfd859862 \$\endgroup\$ Commented Feb 14, 2018 at 8:38
  • \$\begingroup\$ I wonder if this is the time to learn GraphQL. I just barely learned async/await haha. \$\endgroup\$ Commented Feb 14, 2018 at 8:48
2
\$\begingroup\$

Not exactly sure why you need a new array for the dat, you're not mutating the array. The only reason I would shallow-clone is if something else is mutating the array (and that should be fixed). If you've got no way around it, a shorter way to shallow-clone an array is via spread operator, or the old-school array.slice(0).

Also, you can abstract the timer as an awaitable. Have it return a promise and await it inside an async function.

Loops are very boilerplatey because it's very imperative. An alternate way would be to use recursion.

If you're concatenating strings, I recommend using template literals. JavaScript can get really funky when concating variables with +. It usually depends on what the first variable is. If it's a string, it concats as a string. But if it's a number by accident, it becomes math and you get NaN.

You can also use shorthand object properties to shorten assignment of key-value pairs. As long as the variable name is the same as the property name, you can skip the assignment portion (the : value part of the object) and just assign the key.

Here's code that might work, given the adjustments above.

const sleep = delay => new Promise(resolve => { setTimeout(resolve, delay) }, delay)

app.post('/post', async function (req, res) {
  try {
    res.send(await search([...req.body]))
  } catch (e){
    // Handle the failure
  }
});

async function search(data) {
  // No more data.
  if(!data.length) return []


  // Get the next city in line and do a request for it.
  const city = data[0]
  const term = city.name
  const location = `${city.coords.lat},${city.coords.lng}`
  const response = await client.search({ term, location })

  // Build the data
  const business = response.jsonBody.businesses[0]
  const result = {
    name: business.name,
    img: business.image_url,
    hours: business.is_closed,
    revcount: business.review_count,
    rating: business.rating,
    price: business.price,
    location: business.location.display_address.toString(),
    phone: business.display_phone,
    url: business.url
  }

  // Wait for the sleep promise to resolve
  await sleep(1000)

  // Return an array with the result of the current city plus the results of the rest.
  return [result, ...(await search(data.slice(1)))]
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ The delay most likely needs to come before the external API otherwise you don’t mitigate the initial problem. \$\endgroup\$ Commented Feb 13, 2018 at 21:41

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.