4
\$\begingroup\$

The code below calls the parse_config() function which populates the global variables user, passwd and host which are later used in main() function:

#!/usr/bin/env python3

import sys
import configparser


def parse_config(cfg):
    """
    Read the configuration file.
    """

    global user, passwd, host


    if cfg.has_option('login', 'username'):
        user = cfg['login']['username']
    else:
        sys.exit(1)

    if cfg.has_option('login', 'password'):
        passwd = cfg['login']['password']
    else:
        sys.exit(1)

    if cfg.has_option('server', 'host'):
        host = cfg['server']['host']
    else:
        host = 'localhost'


def main():
    config = configparser.ConfigParser()
    with open('conf.ini', encoding='utf-8') as conf_f:
        config.read_file(conf_f)

    parse_config(config)

    print(f'username: {user}, password: {passwd}, host: {host}')


if __name__ == '__main__':
    main()

As using the global variables is not encouraged, then how should one rewrite this script? Perhaps by returning the local variables from the parse_config() function? Like this:

#!/usr/bin/env python3

import sys
import configparser


def parse_config(cfg):
    """
    Read the configuration options.
    """

    if cfg.has_option('login', 'username'):
        user = cfg['login']['username']
    else:
        sys.exit(1)

    if cfg.has_option('login', 'password'):
        passwd = cfg['login']['password']
    else:
        sys.exit(1)

    if cfg.has_option('server', 'host'):
        host = cfg['server']['host']
    else:
        host = 'localhost'

    return user, passwd, host


def main():
    config = configparser.ConfigParser()
    with open('conf.ini', encoding='utf-8') as conf_f:
        config.read_file(conf_f)

    user, passwd, host = parse_config(config)

    print(f'username: {user}, password: {passwd}, host: {host}')


if __name__ == '__main__':
    main()

Or perhaps it's fine to use global variables in this case?

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Global variables

Don’t use them. The second version of your code is better.

Function names & doc strings

If you typed help(parse_config), the displayed help might say:

>>> help(parse_config)

parse_config(cfg): 

    Read the configuration options.

>>>

This is truly confusing. The help text describes the function as reading the configuration, but nowhere does it say where it would be reading it from. Maybe it reads the information from a hard-coded filename? And it must read it into a the cfg structure that is passed to the function, or else what is the purpose of the cfg argument?

And is there “parsing” going on? Reading text, and extracting information from it?

No! The configuration reading and parsing has already been done by configparser.read_file(config_f). This function is neither reading nor parsing the configuration. It should be renamed, and the docstring expanded to say what it is doing, what it expects, and what it returns.

Raise exceptions

Don’t use sys.exit(). Raise an exception, instead. sys.exit() immediately terminates the Python interpreter. Test frameworks often are written in Python, so exiting the Python interpreter makes testing much, much harder.

As a bonus, the exception can describe the problem better. “Configuration is missing the ‘user’ element in [login’]” is much clearer than an exit code of 1, which is also the the error code for a missing password!

God function

The second version of the function returns everything: username, password, and host. Unfortunately, it returns it as a tuple of three values, with no clear indication as to what those values are. If you needed just the host name at another point, you can’t just fetch the one piece you need; you have to get everything.

You could split it up into 3 functions:

def username_from_config(cfg):
   ...

def password_from_config(cfg):
   ...

def host_from_config(cfg):
   ...

The main() function can query each required value, instead of getting all the values.

Alternately, you could return a namedtuple of the information:

from typing import NamedTuple

class Config(NamedTuple):
    user: str
    passwd: str
    host: str

def parse_config(cfg) -> Config:
    ...
    return Config(user, passwd, host)

def main():
    ...
    config = parse_config(cfg)
    print(f'username: {config.user}, password: {config.passwd}, host: {config.host}')

You are still returning a “god” object, but at least the values are stored in named fields.

\$\endgroup\$

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.