Re-thinking my if-thens - a software engineering question

[Steven D'Aprano]

On Wed, 24 Jan 2007 11:51:27 -0800, metaperl wrote:

> Ok, I have a module called textgen.py. The point of this module is to
> generate a csv file from an array of dictionaries.

You probably should use the csv module to do the grunt work, leaving your
module just to massage the dicts into a form that csv can handle.


> As I iterate through
> each dictionary, I "massage" the dictionary values before writing them
> out to csv. Now, for one dictionary entry, I have the following code:
> 
>             if dict_key == 'PCN':
> 		fields = dict_val.split("/")
>                 mo = re.match( '(\w{2})(\d{2})(\d{2})' , fields[1] )
> 		if mo:
> 		    dict_val = "%s/%s%s/%s" % (fields[0], mo.group(1), mo.group(3),
> fields[2][1:])
> 		else:
> 		    dict_val = dict_val

Your indentation seems very inconsistent -- the lines starting with
"fields = ..." and "mo = ..." should have the same indentation.

The line "dict_val = dict_val" is a no-op. You could just as easily write
that as "pass", or even better, leave out the entire else clause.

Reading between the lines, it sounds like you have a mass of if...elif
clauses, like this:

if dict_key == 'PCN':
    # some processing
elif dict_key == 'NCP':
    # some different processing
elif ...
    # blah blah blah
else:
    # default processing


This is an excellent candidate for dictionary-based dispatching. First,
write a function for each processing block:

def process_PCN(value):
    # do stuff to value
    return result

and then create a dispatch table:

dispatcher = {"PCN": process_PCN, "NCP": process_NCP, ...}

Now your huge if...elif block becomes one line:

# no default processing
dispatcher[dict_key](dict_value)

# with default processing
dispatcher.get(dict_key, process_default)(dict_value)


> Ok, so now here is what has happened. This code was based on the
> assumption that dict_val would have 2 forward slashes in it. It turns
> out that I need to follow a different process of massaging when no
> slashes are present. A naive solution would be something like:
> 
> if dict_key == 'PCN':
>                     fields = dict_val.split("/")
>                     if fields == 3:

That can't possibly work. fields is a list, not an integer.

I think you want if len(fields) == 3.

>                         dict_val = pcn_three(fields) # where pcn_three
> is the code above
>                 else:
>                     # new logic
> 
> But I am wondering if I should abstract the flow of control into a
> class or something.

Nope. Abstract it into functions.



-- 
Steven.