6
\$\begingroup\$

The code works but I want to split it into functions and make it more reusable. The way it is now, the code is not reusable. I want the parts which replaces the query string and the part that populates with the number of items to be reusable for other classes, so ideally I want to make'em functions but then I suppose I must pass the template variable to those functions?

class RegionSearch(SearchBaseHandler):
    """Handles regional search requests."""

    def get(self):
        """Handles a get request with a query."""
        category = None
        cityentity = None
        country = ''
        if self.request.host.find('hipheap') > -1: country = 'USA'
        elif self.request.host.find('koolbusiness') > -1: country = 'India'
        elif self.request.host.find('montao') > -1: country = 'Brasil'
        regionID = 0
        cityID = 0
        categoryID = 0
        regionname = None
        cityname = None
        categoryname = None
        region = None
        cursor = self.request.get("cursor")
        uri = urlparse(self.request.uri)
        query = ''
        regionid = self.request.get("regionid")
        cityid = self.request.get("cityid")
        categoryid = self.request.get("category")
        if uri.query:
            query = parse_qs(uri.query)
            try:            
                query = query['query'][0]
            except KeyError, err:
                query = ''
                #logging.info('KeyError')

        #Try find region ID and/or cityID and categoryID a.s.a.p.
        if regionid or query.find('regionID') > -1:
            regionID = re.sub("^regionID=(\d+).*", r'\1', query)
            region = Region.get_by_id(long(regionID))
            regionname = region.name

        if regionid:
            regionID = regionid
            region = Region.get_by_id(long(regionID))
            regionname = region.name

        if cityid or query.find('cityID') > -1:
            cityID = re.sub("^.*cityID=(\d+).*", r'\1', query)
            if cityid: cityID = cityid
            city = montaomodel.City.get_by_id(long(cityID))
            cityID = city.key().id()
            cityentity = city
            cityname = city.name
            region = Region.get_by_id(long(city.region.key().id()))
            regionID = region.key().id()
            regionname = region.name
            regionentity = region
            regionname = region.name

        if categoryid or query.find('category') > -1:
            categoryID = re.sub("^.*category=(\d+).*", r'\1', query)
            if categoryid: categoryID = categoryid

        logging.debug('categoryID %s' , str(categoryID))
        #logging.debug('region id %s' , regionID)
        #logging.debug('city id %s' , cityID)
        #logging.debug('regionname %s' , regionname)
        #logging.debug('cityname %s' , cityname)

        if cursor: results = find_documents(query, 50, search.Cursor(cursor))
        else: results = find_documents(query, 50, search.Cursor())

        next_cursor = None
        if results and results.cursor: next_cursor = results.cursor.web_safe_string
        number_returned = 0
        if results: number_returned = len(results.results)
        namedquery = query  
        query = query.replace(' and company_ad=0','').replace(' and company_ad=1','').replace(' and category:(6010 OR 6020 OR 6030 OR 6040 OR 6090)','').replace(' and category:(6010 OR 6020 OR 6030 OR 6040 OR 6090)','').replace(' and category:(1020 OR 1010 OR 1030 OR 1050 OR 1080 OR 1100 OR 1090)','').replace(' and category:(2010 OR 2030 OR 2040 OR 2080 OR 2070)','').replace(' and category:(3040 OR 3050 OR 3030 OR 3060)','').replace(' and category:(4010 OR 4020 OR 4040 OR 4030 OR 4090 OR 4060 OR 4070)','')

        query = re.sub("regionID=\d+", "", query)
        query = query.replace('category and','')
        query = query.replace('type=s','')
        query = query.replace('type=w','')
        query = query.replace('type=r','')
        query = query.replace('type=b','')
        query = query.replace('cityID and','')
        query = query.replace('and','')
        query = query.replace('regionID','')
        query = query.replace('=','%3D')

        namedquery = namedquery.replace('=','%3D')

        query = re.sub("cityID%3D\d+", "", query)
        query = re.sub("category%3D\d+", "", query)
        query = query.replace('  ',' ')


        if regionID == '4703187': regionname = 'Andaman & Nicobar Islands'
        elif regionID == '4694186': regionname = 'Andhra Pradesh'
        elif regionID == '4699188': regionname = 'Arunachal Pradesh'
        elif regionID == '4692186': regionname = 'Assam'
        elif regionID == '4702186': regionname = 'Bihar'
        elif regionID == '4698185': regionname = 'Chandigarh'
        elif regionID == '4676188': regionname = 'Chhattisgarh'
        elif regionID == '4691190': regionname = 'Dadra & Nagar Haveli'
        elif regionID == '4704183': regionname = 'Daman & Diu'
        elif regionID == '4699183': regionname = 'Delhi'
        elif regionID == '4702187': regionname = 'Goa'
        elif regionID == '4691189': regionname = 'Gujarat'
        elif regionID == '4700186': regionname = 'Haryana'
        elif regionID == '4703185': regionname = 'Himachal Pradesh'
        elif regionID == '4694187': regionname = 'Jammu & Kashmir'
        elif regionID == '4699189': regionname = 'Jharkhand'
        elif regionID == '4701185': regionname = 'Karnataka'
        elif regionID == '4695189': regionname = 'Kerala'
        elif regionID == '4700189': regionname = 'Lakshadweep'
        elif regionID == '4697186': regionname = 'Madhya Pradesh'
        elif regionID == '4694184': regionname = 'Maharashtra'
        elif regionID == '4700187': regionname = 'Manipur'
        elif regionID == '4703186': regionname = 'Meghalaya'
        elif regionID == '4698184': regionname = 'Mizoram'
        elif regionID == '4692187': regionname = 'Nagaland'
        elif regionID == '4696185': regionname = 'Orissa'
        elif regionID == '4676189': regionname = 'Pondicherry'
        elif regionID == '4693185': regionname = 'Punjab'
        elif regionID == '4701186': regionname = 'Rajasthan'
        elif regionID == '4701187': regionname = 'Sikkim'
        elif regionID == '4701188': regionname = 'Tamil Nadu'
        elif regionID == '4697187': regionname = 'Tripura'
        elif regionID == '4699190': regionname = 'Uttaranchal'
        elif regionID == '4692188': regionname = 'Uttar Pradesh'
        elif regionID == '4700188': regionname = 'West Bengal'
        elif regionID and query.find('cityID') < 1: 
            region = Region.get_by_id(long(regionID))
            regionname = region.name

        form = SearchForm()

        form.w.choices = [
        ('4703187', u'Andaman & Nicobar Islands'),
        ('4694186', u'Andhra Pradesh'),
        ('4699188', u'Arunachal Pradesh'),
        ('4692186', u'Assam'),
        ('4702186', u'Bihar'),
        ('4698185', u'Chandigarh'),
        ('4676188', u'Chhattisgarh'),
        ('4691190', u'Dadra & Nagar Haveli'),
        ('4704183', u'Daman & Diu'),
        ('4699183', u'Delhi'),
        ('4702187', u'Goa'),
        ('4691189', u'Gujarat'),
        ('4700186', u'Haryana'),
        ('4703185', u'Himachal Pradesh'),
        ('4694187', u'Jammu & Kashmir'),
        ('4699189', u'Jharkhand'),
        ('4701185', u'Karnataka'),
        ('4695189', u'Kerala'),
        ('4700189', u'Lakshadweep'),
        ('4697186', u'Madhya Pradesh'),
        ('4694184', u'Maharashtra'),
        ('4700187', u'Manipur'),
        ('4703186', u'Meghalaya'),
        ('4698184', u'Mizoram'),
        ('4692187', u'Nagaland'),
        ('4696185', u'Orissa'),
        ('4676189', u'Pondicherry'),
        ('4693185', u'Punjab'),
        ('4701186', u'Rajasthan'),
        ('4701187', u'Sikkim'),
        ('4701188', u'Tamil Nadu'),
        ('4697187', u'Tripura'),
        ('4699190', u'Uttaranchal'),
        ('4692188', u'Uttar Pradesh'),
        ('4700188', u'West Bengal'),
        ]

        jobs_count = None
        estate_count = None
        electronics_count = None
        home_count = None
        leisure_count = None
        vehicles_count = None

        if (region or cityentity): 
            #to do:use memcache
            form.area.choices = [] # to do: use memcache for the list
            for cityitem in City.all().filter('region =', region.key()).order('-vieworder').order('name').fetch(99999):
                form.area.choices.append([str(cityitem.key().id()), cityitem.name])
            if cityentity: form.area.data = str(cityentity.key().id())

            if self.request.host.find('hipheap') > -1: 
                if region and (str(region.key().id()), region.name) in form.w_us.choices: form.w_us.choices.remove((str(region.key().id()), region.name))
            else:
                if region and (str(region.key().id()), region.name) in form.w.choices: form.w.choices.remove((str(region.key().id()), region.name))

        if cityID and int(cityID) > 0:

            jobs_count_gql = db.GqlQuery("SELECT * FROM Ad WHERE category IN ('6010', '6020', '6030', '6040', '6090') AND cities = KEY('City', :1) AND published = True AND modified > :2 ", cityID, datetime.now() - timedelta(days=609))
            jobs_count= jobs_count_gql.count(limit=40000)

            estate_count_gql = db.GqlQuery("SELECT * FROM Ad WHERE category IN ('1010', '1020', '1030', '1050', '1080', '1090', '1100') AND cities = KEY('City', :1) AND published = True AND modified > :2 ", cityID, datetime.now() - timedelta(days=609))
            estate_count= estate_count_gql.count(limit=40000)

            electronics_count_gql = db.GqlQuery("SELECT * FROM Ad WHERE category IN ('5010', '5020', '5030', '5040') AND cities = KEY('City', :1) AND published = True AND modified > :2 ",  cityID, datetime.now() - timedelta(days=609))
            electronics_count= electronics_count_gql.count(limit=40000)

            home_count_gql = db.GqlQuery("SELECT * FROM Ad WHERE category IN ('3030', '3040', '3050', '3060') AND published = True AND cities = KEY('City', :1) AND modified > :2 ",  cityID, datetime.now() - timedelta(days=609))
            home_count= home_count_gql.count(limit=40000)

            leisure_count_gql = db.GqlQuery("SELECT * FROM Ad WHERE category IN ('4010', '4020', '4030', '4040', '4060', '4090') AND cities = KEY('City', :1) AND published = True AND modified > :2 ",  cityID, datetime.now() - timedelta(days=609))
            leisure_count= leisure_count_gql.count(limit=40000)

            vehicles_count_gql = db.GqlQuery("SELECT * FROM Ad WHERE category IN ('2010', '2030', '2040', '2070', '2080') AND cities = KEY('City', :1) AND published = True AND modified > :2 ",  cityID, datetime.now() - timedelta(days=609))
            vehicles_count= vehicles_count_gql.count(limit=40000)

        elif regionID and int(regionID) > 0:
            logging.debug('regionID: %d', int(regionID))
            regionID = int(regionID)
            jobs_count_gql = db.GqlQuery("SELECT * FROM Ad WHERE category IN ('6010', '6020', '6030', '6040', '6090') AND regions = KEY('Region', :1) AND published = True AND modified > :2 ", regionID, datetime.now() - timedelta(days=609))
            jobs_count= jobs_count_gql.count(limit=40000)

            estate_count_gql = db.GqlQuery("SELECT * FROM Ad WHERE category IN ('1010', '1020', '1030', '1050', '1080', '1090', '1100') AND regions = KEY('Region', :1) AND published = True AND modified > :2 ", regionID, datetime.now() - timedelta(days=609))
            estate_count= estate_count_gql.count(limit=40000)

            electronics_count_gql = db.GqlQuery("SELECT * FROM Ad WHERE category IN ('5010', '5020', '5030', '5040') AND regions = KEY('Region', :1) AND published = True AND modified > :2 ",  regionID, datetime.now() - timedelta(days=609))
            electronics_count= electronics_count_gql.count(limit=40000)

            home_count_gql = db.GqlQuery("SELECT * FROM Ad WHERE category IN ('3030', '3040', '3050', '3060') AND published = True AND regions = KEY('Region', :1) AND modified > :2 ",  regionID, datetime.now() - timedelta(days=609))
            home_count= home_count_gql.count(limit=40000)

            leisure_count_gql = db.GqlQuery("SELECT * FROM Ad WHERE category IN ('4010', '4020', '4030', '4040', '4060', '4090') AND regions = KEY('Region', :1) AND published = True AND modified > :2 ",  regionID, datetime.now() - timedelta(days=609))
            leisure_count= leisure_count_gql.count(limit=40000)

            vehicles_count_gql = db.GqlQuery("SELECT * FROM Ad WHERE category IN ('2010', '2030', '2040', '2070', '2080') AND regions = KEY('Region', :1) AND published = True AND modified > :2 ",  regionID, datetime.now() - timedelta(days=609))
            vehicles_count= vehicles_count_gql.count(limit=40000)




        template_values = {
                'results': results,'cursor':next_cursor, 'country' : country,'user': self.current_user,
                'number_returned': number_returned,'loggedin': self.logged_in, 'VERSION': VERSION,
                'region' : region,'regionname' : regionname,'jobs_count':jobs_count,'estate_count':estate_count,'electronics_count':electronics_count,
            'home_count':home_count,'leisure_count':leisure_count,'vehicles_count':vehicles_count,
         'cityentity': cityentity, 'request' : self.request, 'categoryID' : categoryID,
         'form' : form, 'query' : query, 'namedquery' : namedquery, 'cityname' : cityname,'category': category, 'jobs_count': jobs_count,
            }
        self.render_template('q.htm', template_values)

Update 1

I could make functions that call memcache for the GQL parts so it's a bit better now.

\$\endgroup\$
6
\$\begingroup\$

Start with making a dictionary for the regionId-to-name part:

region_id_to_name = {'4694186': 'Andhra Pradesh', ... } #very long

And put it, along the form.w.choices list (which is actually exactly the same dictionary) outside the class, or as an instance variable. It should probably be read from a file, but that's a different story.

You can turn most of the local variables into instance variables.

The most basic thing is to ask "what am I doing here" in each part of this function. if you can answer, take this description, add a def before and arguments after, and you'll have a function/method.

Here's a simple example:

def mutate_query(self, query):
    query = query.replace(...) # whatever you are doing here
    query = re.sub("regionID=\d+", "", query)
    to_remove = ['category and', 'type=s', 'type=w', 'type=r','type=b','cityID and','and','regionID']
    for s in to_remove:
        query = query.replace(s,'')
    query = query.replace('=','%3D')
    query = re.sub("cityID%3D\d+", "", query)
    query = re.sub("category%3D\d+", "", query)
    query = query.replace('  ',' ')
    return query
\$\endgroup\$
  • \$\begingroup\$ Thank you but if I put the form.w.choices outside the class it won't refresh and I need it to refresh since I'm putting things in and out of the options. So I think the variable must be in the class anyhow. The region_id dictionary is whwat I'm working with now doing as you advice here, and I've also made functions that use memcache and updated the question with the refactored code. Thanks a lot for all the help and do comment if I said something here that you want to elaborate on or discuss. \$\endgroup\$ – Niklas Rosencrantz Jun 24 '13 at 2:25
3
\$\begingroup\$

Without checking any further, you can already save yourself 10 lines if you don't first initialize all local variables and afterwards override all of them anyway by assigning the (potentially defaulted) result from the request. E.g. as in regionid = self.request.get("regionid", 0).

While being at it, you should plan what you want to do with all these local variables afterwards. If you don't want to process them already in get() (and you probably don't want this) and if you don't want to pass them all as separate args to another processing function (probably not wanted too), you might consider putting them into instance variables, a dict, another helper object, a named tuple, or whatever fits your follow up processing best.

And the country and url processing should be factorized out in helpers, so that all processing in get() is on one logical level - fetch a (potentially defaulted) result from the request and assign it, no more details here.

\$\endgroup\$
  • \$\begingroup\$ That is very good advice indeed thanks a lot. You saved me many lines. I could also update the question with a new version of the class where it's using functions and memcache so it should be both faster and more modular now. I've made the changes that you mention put they are not part of an update yet, the changes will be part of the next update. \$\endgroup\$ – Niklas Rosencrantz Jun 24 '13 at 2:27
  • 1
    \$\begingroup\$ Happy to hear that, you're very welcome. \$\endgroup\$ – user2512319 Jun 24 '13 at 4:28

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.