0
\$\begingroup\$

I need to handle configuration errors and ask the user for the right credentials if something is wrong. I cannot decide between three implementations. Which style is better in Python and why?

A:

while not backend.check_config():
    click.echo('Invalid Configuration parameters!')
    for param_name, question in backend.config.check_config_requires:
        value = prompt(question, default=getattr(backend.config, param_name))
        setattr(backend.config, param_name, value)

B:

while True:
    try:
        backend.check_config()
        break
    except ConfigurationError:
        click.echo('Invalid Configuration parameters!')
        for param_name, question in backend.config.check_config_requires:
            value =prompt(question, default=getattr(backend.config, param_name))
            setattr(backend.config, param_name, value)

C:

while True:
    try:
        backend.check_config()
    except ConfigurationError:
        click.echo('Invalid Configuration parameters!')
        for param_name, question in backend.config.check_config_requires:
            value =prompt(question, default=getattr(backend.config, param_name))
            setattr(backend.config, param_name, value)
    else:
        break
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I am assuming that backend.check_config() is different between your implementations. In one case, it seems to return a boolean, but the other two make it look like it might raise an exception. My answer is based on that assumption.


The name of the function is check_config. If it were, parse_config, for example, I would say that it should throw an error because it is expected to do something, but can't because of a problem. Since it is merely checking, I think a boolean fits better, so my preference goes to A.


Between B and C, it looks like the only difference is whether break is in the try block or the else block. I am happy to see that you are aware of the else block, but my opinion is that it isn't necessary. The else block is useful for the cases when you are trying to exclude a certain piece of code from the try block. That makes sense for when the try is in another try, or for when you don't expect an error and want to know the error happened. In this case, it is impossible for break to throw an error, so I believe the else block is an unnecessary complication.

I did notice that you took out the space between = and prompt(...) in B and C. PEP 8, the Python style guide, recommends a space on each side of = in assignments. (It recommends no spaces in a function call such as dict(x=4, y=6)) That could be just a typo, but this is a review.


Based on how you use it, I would think that backend.config would be better as a dictionary. That way, you could do backend.config[param_name] instead of the more complicated getattr(backend.config, param_name) (or settattr(...)). If config is created externally, you might consider using vars(config) to create a dictionary of its attributes. Without further information, I couldn't guarantee that it would be easier, but that's how it looks.

\$\endgroup\$
1
  • \$\begingroup\$ Awesome review, thank you! Yes, check_config() is different. I didn't even thought of making config dict-like. I introduced a method config.set_param() and config.get_param() but it's not really better than setattr and getattr. Space typo :) \$\endgroup\$
    – kissgyorgy
    Commented Aug 14, 2016 at 9:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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