Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've been on stackoverflow looking for an alternative to the ugly if-elif-else structure shown below. The if-else structure takes three values as input and returns a pre-formatted string used for an SQL query. The code works right now, but it is difficult to explain, ugly, and I don't know a better way to do re-format it.

I need a solution that is more readable. I was thinking a dictionary object, but I can't wrap my mind how to implement one with the complex if/else structure I have below. An ideal solution will be more readable.

def _getQuery(activity,pollutant,feedstock):



   if feedstock.startswith('CG') and pollutant.startswith('VOC'):
        pass

    if feedstock == 'FR' and activity == 'Harvest':
        return 'FR'

    elif feedstock == 'FR':
        return 'No Activity'

    elif(activity == 'Fertilizer' and pollutant != 'NOx' and pollutant != 'NH3'):
        return 'No Activity'

    elif(activity == 'Chemical' and pollutant != 'VOC'):
        return 'No Activity'        

    elif( (feedstock == 'CS' or feedstock == 'WS')
        and 
        (activity == 'Non-Harvest' or 
        activity == 'Chemical')):     
        return 'No Activity'

    elif( ( ( pollutant.startswith('CO') or pollutant.startswith('SO') ) 
                and 
            ( activity == 'Non-Harvest' or 
            activity == 'Harvest' or 
            activity == 'Transport' ) 
            )
            or 
            ( pollutant.startswith('VOC') 
                and not
            ( feedstock.startswith('CG') or feedstock.startswith('SG') )
            )
        ): 

        rawTable = feedstock + '_raw'
        return """
    with
    activitySum as (select distinct fips, sum(%s) as x 
        from %s where description ilike '%s' group by fips),

    totalSum as (select distinct r.fips, sum(r.%s) as x 
        from %s r group by r.fips),

    ratios as (select t.fips, (a.x/t.x) as x from activitySum a, totalSum t 
        where a.fips = t.fips),

    maxRatio as (select r.fips, r.x as x from ratios r 
        group by r.fips, r.x order by x desc limit 1),

    minRatio as (select r.fips, r.x as x from ratios r 
        group by r.fips, r.x order by x asc limit 1)

    select mx.x, mn.x from maxRatio mx, minRatio mn;
    """ % (pollutant, rawTable, '%'+activity+'%',
       pollutant, rawTable)


    elif( (pollutant[0:2] == 'PM')
            and 
            (activity == 'Non-Harvest' or 
            activity == 'Harvest' or 
            activity == 'Transport')): 



        rawTable = feedstock + '_raw'
        return """
    with
    activitySum as (select distinct fips, (sum(%s) + sum(fug_%s)) as x 
        from %s where description ilike '%s' group by fips),

    totalSum as (select distinct r.fips, (sum(r.%s) + sum(r.fug_%s)) as x 
        from %s r group by r.fips),

    ratios as (select t.fips, (a.x/t.x) as x from activitySum a, totalSum t 
        where a.fips = t.fips),

    maxRatio as (select r.fips, r.x as x from ratios r 
        group by r.fips, r.x order by x desc limit 1),

    minRatio as (select r.fips, r.x as x from ratios r 
        group by r.fips, r.x order by x asc limit 1)

    select mx.x, mn.x from maxRatio mx, minRatio mn;
    """ % (pollutant, pollutant, rawTable, '%'+activity+'%',
       pollutant, pollutant, rawTable)

.
.
.
Lots more complex elif statements
share|improve this question
1  
I'm not sure it's possible to answer this without knowing more about the range of possible parameters and return values. It might be worth creating more variables with meaningful names that would help make clear why certain combinations of parameter values result in a particular output. –  Stuart Dec 18 '12 at 21:03
    
@Stuart, should I include the full if/elif structure in the question? –  nfisher Dec 18 '12 at 22:19
    
It might help if it's not terribly long. To start with, though, I would note that there is a lot of repetition in the long query string """with activitySum etc. which appears twice with only minor changes. Maybe see if you can use the if/elif structure to set values of variables with meaningful names which are then sanitised and fed into a single 'template' query string at the end. –  Stuart Dec 19 '12 at 0:03
    
@Stuart, thanks for pointing out the code duplicate, by removing the duplicates, the code is looking more reasonable. Thanks! –  nfisher Dec 19 '12 at 16:24
    
It can't be right to return a SQL query in some cases, and the string 'No Activity' in others. –  Gareth Rees Dec 19 '12 at 20:40

1 Answer 1

up vote 6 down vote accepted

My thought would be to create a collection of objects that can be iterated over. Each object would have an interface like the following.

def _getQuery(activity,pollutant,feedstock):
  for matcher in MATCHERS:
    if matcher.matches(activity,pollutant,feedstock):
      return matcher.get_result(activity,pollutant,feedstock)
  raise Exception('Not matched')

Then you build up the set of matcher classes for the various cases and ad an instance of each to MATCHERS. Just remember that earlier elements in the list take precedence to later ones. So the most specific cases should be first in the list ans the most general cases should be at the end.

NOTE: Your string building code is vulnerable to SQL-injection. If you do go forward with manually building your selection strings, your input needs to be more thoroughly sanitized.

share|improve this answer
    
thanks for your comments, especially the part about SQL-injection! I was hoping for a more object-oriented approach and this seems quite reasonable! –  nfisher Dec 19 '12 at 16:24

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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