current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

I'm trying to cover the following 4 scenarios:

  1. No parameters
  2. DB param
  3. DB param and 1 option
  4. No DB param and 1 option

So far I have the program working for all scenarios. However, I'm not happy with the way it's implemented. Is it possible to change this to a cleaner solution, rather than checking if there's only 1 parameter passed or catching an exception?

def do_work(database=None):
    if database:
        print 'Doing work for {0}'.format(database)
    else:
        print 'Doing work all database'

def do_other_work(database=None):
    if database:
        print 'Doing other work for {0}'.format(database)
    else:
        print 'Doing other work all database'

def create_parser():
    parser = ArgumentParser(description='Parser')
    parser.add_argument('--db', '-d', dest='database',
                        default=None, required=False, help='Database name')

    option_group = parser.add_mutually_exclusive_group(required=False)
    option_group.add_argument('-a', dest='cmd',
                              action='store_const',
                              const=lambda args: do_other_work(args.database)
    return parser

if len(sys.argv) == 1:
    do_work()

parser = create_parser()
parsed_args = parser.parse_args(sys.argv[1:])

try:
    parsed_args.cmd(parsed_args)
except TypeError:
    do_work(parsed_args.database)
  • No parameters passed: do_work() for all databases
  • db parameter passed and no option: do_work() for just that database
  • db parameter and an option: do_other_work() just for that database
  • No db parameter and an option: do_other_work() for all databases
share|improve this question
2  
I have rolled back the last edit. Please see What to do when someone answers. – Mathias Ettinger Jun 2 at 12:00
up vote 4 down vote accepted
  • You do not have to check len(sys.argv) yourself. A properly constructed ArgumentParser will sort that out by itself.
  • Likewise, you do not need to pass sys.argv[1:] to parse_args(), argparse will figure it out.
  • default=None and required=False are default values for an optional argument (those starting with '-'), you do not need to supply them.
  • You do not need to use a lambda to specify the function to call, just pass in the name of the function: it will simplify handling this parameters.
  • You can add do_work as a default value for '-a': no more TypeError as the content of parsed_args.cmd will alway be a callable.
  • There is no need in using a mutually exclusive group if you only ever put one option in it: remove it.
  • You should use if __name__ == '__main__': and put your top-level code under it.
  • In do_*work you should test database using is None or is not None.
def do_work(database=None):
    if database is not None:
        print 'Doing work for {0}'.format(database)
    else:
        print 'Doing work all database'

def do_other_work(database=None):
    if database is not None:
        print 'Doing other work for {0}'.format(database)
    else:
        print 'Doing other work all database'

def create_parser():
    parser = ArgumentParser(description='Parser')
    parser.add_argument('--db', '-d', dest='database', help='Database name')
    parser.add_argument('-a', dest='cmd', action='store_const',
                        const=do_other_work, default=do_work)
    return parser


if __name__ == '__main__':
    parsed_args = create_parser().parse_args()
    parserd_args.cmd(parsed_args.database)
share|improve this answer
    
Hi. thanks for the input. The mutually exclusive group is in fact a multiple option group. I just removed the other options for the sake of the post. Is it possible to use this default=do_work option for all mutually exclusive items and go there if there's no mutually exclusive parameter passed ? – tehjoker Jun 2 at 11:51
3  
I knew I should have voted to close this post as off-topic for hypothetical code. If you want advices on your real code, post your real code… – Mathias Ettinger Jun 2 at 11:53
    
The real code doesn't belong to me. This is a short, working snippet creating the exact same scenario in a generic form. – tehjoker Jun 2 at 11:54
2  
Which isn't since the group had more arguments than what you showed us. Anyway, option_group.set_defaults(cmd=do_work) should do the job. – Mathias Ettinger Jun 2 at 11:58
    
sounds like it will do the job, thanks! – tehjoker Jun 2 at 12:00

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.