1
\$\begingroup\$

The code below is working properly but how do I make this code faster and more efficient. How do I perfect this code?

Let's assume I have 1000 country, state and city records in a mongodb collection. I want to retrieve all the city data by matching country id and state id. Please tell me the perfect way to code.

// @route   GET citylist/:countryid/:stateid
// @desc    get all the city from mongodb for countryid and stateid

router.get("/citylist/:countryid/:stateid", (req, res) => {
  const isValidCountryid = mongoose.Types.ObjectId.isValid( req.params.countryid);
  const isValidStateid = mongoose.Types.ObjectId.isValid(req.params.stateid);

  if (isValidCountryid && isValidStateid) {
    CityModel.find({
      country_id: req.params.countryid,
      state_id: req.params.stateid
    })

      .then(cities => {
        if (!cities || !cities[0]) {
          return res.status(404).json({ msg: "City not found" });
        } else {
          res.json(cities);
        }
      })
      .catch(error => res.status(500).send(error));

  } else {
    res.json({ msg: "Country or State Id is not valid" });
  }
});

how to make this code highly fast, efficient, great time complexity and perfect way of code.

\$\endgroup\$
  • 1
    \$\begingroup\$ Please have a look at Titling your question in the Help center and change your title accordingly! Namely, describe what your task is, not what you expect from the review. \$\endgroup\$ – AlexV May 16 at 8:18
0
\$\begingroup\$

From a short review,

  • Make sure that your database has an index: https://docs.mongodb.com/manual/indexes/
  • For the 404, since most countries have more than 1 country, I would return "Cities not found", I might even include the provided country and city in the return message so that the admin can be provided with an informative message
  • I would have 2 different messages when the input data is bad, one for City, and one for country. Furthermore, for bad input data, I would use return code 400.
  • It's very minor, but CountryId looks better than countryid
  • Instead of if (isValidCountryid && isValidStateid) { I would check for bad data, and exit immediately, it makes for a lower cyclomatic complexity
\$\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.