2
\$\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
4
\$\begingroup\$
  • Utilize the query object, this type of querying is more suitable there than as params.
  • Use object destructuring.
  • Dry your code, see how we're mapping over [countryId, stateId] to check the validity of the input. Ideally, you would have been using a module like Joi for that.
  • Maintain a happy path and break out of the function early when erroring.
  • Use async/await instead of .then().
  • Return a 400 when the input is invalid.
  • User proper camelCase (in both the DB and the backend).
  • Check the array length instead of checking the existence of the first element. So if (arr.length) not if (arr[0]). The reason for that is at some cases the first element of the array could be 'falsy' (such as '' and 0) which can be valid sometimes.
  • When erroring, return an error field, not a msg one.
  • Study javascript's array methods such as map, reduce, filter, and every. They will save your life more often than you thought.
router.get("/citylist", async (req, res) => {
  try {

    const { countryid, stateId } = req.query

    const validInput = [countryid, stateid].map(mongoose.Types.ObjectId.isValid).every(Boolean)
    if (!validInput) return res.status(400).json({ error: "Country or State Id is not valid" })

    const cities = await CityModel.find({ countryId, stateId })
    if (!cities || cities.length) return res.status(404).json({ error: "City not found" })

    res.json(cities);

  } catch (error) {
    res.status(500).json({ error })
  }
});
\$\endgroup\$
2
\$\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 city, 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.