1
\$\begingroup\$

I have to calculate costs based on country code and city(optional). There is also a condition that countries in Asia have separate pricing logic depending on both country and city and they are stored in a different table (for segregating regional price variances). For the rest, it's only dependent on country code. Let's assume we must have separate tables Asia & the rest.

Please suggest some improvements in the below code.

def get_cost_by_location(country_code: str, city=None) -> Optional[models.OperatorCost]:
    cost = None

    # check if the country is in Asia
    if models.OperatorCostAsia.objects.filter(country=country_code).exists():
        cost = models.OperatorCostAsia.objects.filter(country=country_code, city__icontains=city).first()

        if cost is None:
            cost = models.OperatorCostAsia.objects.filter(country=country_code).first()
    else:
        cost = models.OperatorCost.objects.filter(country=country_code).first()

    return cost

I have two different tables to calculate the prices. The pricing table for Asia has three columns: Country code, city name and the price itself. The pricing table for rest of the countries just has country code and price columns.
I get a mandatory country code and an optional city name as parameters from the frontend. First I check these params against the Asia pricing table. if the selected country happens to be Asian, I query against country code and city name (if there is one). If the city param is empty, i just pick the first match against the country.

If the country is not an Asian country, I simply query the second (Rest of the world) table to find the pricing against the country code.

\$\endgroup\$
  • \$\begingroup\$ @Linny I came here for help. Whether it's my code or not, I have been given the responsibility to clean up my project written by 10 other developers in past few years. What's the point of having this forum if I someone can't review a piece of code just because he/she is too busy finding the original author of the code. Also, how are you so sure that all the post in this forum are 100% verified that they are written by the author? It's pointless and impractical. \$\endgroup\$ – saran3h Sep 2 '19 at 11:11
  • 1
    \$\begingroup\$ I don't completely get the Asian country and city thing… Per your description, each country in Asia (found by its country code) is split into several city. But your code seems to imply that a country could have no city attached to it. Either that, or you pick the first city out of the given country any time an invalid city is provided. Can you explain a bit more what's going on here? \$\endgroup\$ – 301_Moved_Permanently Sep 2 '19 at 12:48
  • \$\begingroup\$ @mathias added more info in the post. Please check now. \$\endgroup\$ – saran3h Sep 2 '19 at 16:04
2
\$\begingroup\$

The first thing that would benefit this code, beside removing the useless initialization for cost, is to save the models.OperatorCostAsia.objects.filter(country=country_code) queryset for reuse instead of rebuilding it each time. It feels easier to understand and filtering twice is identical to using two parameters in a single filter anyway:

def get_cost_by_location(country_code: str, city=None) -> Optional[models.OperatorCost]:
    asian_country = models.OperatorCostAsia.objects.filter(country=country_code)
    if asian_country.exists():
        cost = asian_country.filter(city__icontains=city).first()
        if cost is None:
            cost = asian_country.first()
    else:
        cost = models.OperatorCost.objects.filter(country=country_code).first()

    return cost

Now there are a few other changes that I would perform but these may change the behavior of the function so it's a bit of guessing territory on how you use this function.

For starter I would use iexact instead of icontains when searching for the city name, it feels less error prone.

Second, I would use .get(...) instead of .filter(...).first() when applicable to check for wrongly constructed inputs that would return more than the one result they are expected to return:

def get_cost_by_location(country_code: str, city=None) -> Optional[models.OperatorCost]:
    asian_country = models.OperatorCostAsia.objects.filter(country=country_code)
    if asian_country.exists():
        try:
            return asian_country.get(city__iexact=city)
        except models.OperatorCostAsia.DoesNotExist:
            return asian_country.first()
    else:
        return models.OperatorCost.objects.get(country=country_code)
\$\endgroup\$
  • \$\begingroup\$ Great. just what i was looking for. I was just wondering is there any performance difference between iexact and icontains? \$\endgroup\$ – saran3h Sep 2 '19 at 18:23
  • 1
    \$\begingroup\$ @saran3h iexact is a WHERE city ILIKE 'city' while icontains is a WHERE city ILIKE '%city%' clause. Now I'm no SQL expert but I'd say the second preforms more work and is thus slower. \$\endgroup\$ – 301_Moved_Permanently Sep 2 '19 at 18:33

Not the answer you're looking for? Browse other questions tagged or ask your own question.