2
\$\begingroup\$

I've written the following function to parse a configuration file. I did my best to make it simple, but it's still over 30 lines and has too much indentation caused by a series of if statements.

Any suggestions make it easier to read are welcome.

int
parse_config (FILE * conf_file, module_initializer_t init)
{
  char *line; /* Current line */
  char *type; /* Type of current module */
  char *name; /* Name of current module */
  char *envz;
  size_t envz_len;
  size_t lineno = 0;

  line = type = name = envz = NULL;
  while (getline (&line, NULL, conf_file) != EOF) 
    {
      lineno++ ;
      if (!is_comment(line))
        if (line[0] == '[')
          if (update_header(line, &type, &name)) {
            if (type)
              if ((*init)(type, name, envz, envz_len)) {
                fprintf("failed to initialize module %s with name %s", type, name);
                return 1;
              }
            clear_envz(&envz, &envz_len);
            }
          else {
            fprint("syntax error on line %d", lineno);
            return 1;
          }
      else
        argz_add(&envz, &envz_len, line);
    }
  if ((*init)(type, name, envz, envz_len)) {
    fprintf("failed to initialize module %s with name %s", type, name);
    return 1;
  }
}

It parses a configuration file with the following syntax:

   #comment and empty lines ignored
   [module-type]{module-name}
   var1=val1
   #comment
   var2=val2
   [another-type]{another-name}
   var3=val3
\$\endgroup\$
  • \$\begingroup\$ Does it work? What is it supposed to do? Note that you have init called with envz and envz_len uninitialized, and the function falls off the end without a return value. Also, which if is the else argz_add supposed to belong to? \$\endgroup\$ – William Morris Oct 12 '12 at 14:15
  • \$\begingroup\$ Why uninitialized? NULL and 0 are valid. According standard and indentation. \$\endgroup\$ – KAction Oct 13 '12 at 4:01
  • \$\begingroup\$ Does fprintf not need a file stream as first argument? cplusplus.com/reference/clibrary/cstdio/fprintf \$\endgroup\$ – miracle173 Oct 13 '12 at 4:26
  • \$\begingroup\$ Yes, you are right. But that's not the point. \$\endgroup\$ – KAction Oct 13 '12 at 4:41
  • \$\begingroup\$ Sorry, I didn't notice envz is set to NULL. GCC will warn you of the ambiguous 'else'. According to the indentation, argz_add() is called for comments. \$\endgroup\$ – William Morris Oct 13 '12 at 15:02
2
\$\begingroup\$

I would do some re-arranging:
Use the quick exit pattern.

This is basically if it fails the useful criteria exit (or in this case start the next loop). Basically you test for conditions that don't satisfy your working conditions. If the data does not match then you move on.

int
parse_config (FILE * conf_file, module_initializer_t init)
{
  char *line; /* Current line */
  char *type; /* Type of current module */
  char *name; /* Name of current module */
  char *envz;
  size_t envz_len;
  size_t lineno = 0;

  line = type = name = envz = NULL;
  while (getline (&line, NULL, conf_file) != EOF) 
  {
      lineno++ ;
      if (is_comment(line))
      {    continue;        // Ignore comments;
      }

      if (line[0] != '[')
      {
          argz_add(&envz, &envz_len, line);
          continue;         // Processes arguments and continue
      }

      if (!update_header(line, &type, &name))
      {
          fprint("syntax error on line %d", lineno);
          return 1;         // Not an argument and not a header
                            // must be an error.
      }

      // Processes a header
      if (type)
      {
          if ((*init)(type, name, envz, envz_len))
          {
            fprintf("failed to initialize module %s with name %s", type, name);
            return 1;
          }
          clear_envz(&envz, &envz_len);
      }     
  }

  if ((*init)(type, name, envz, envz_len)) {
    fprintf("failed to initialize module %s with name %s", type, name);
    return 1;
  }

  // You should also return a value here.
  return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Your code leaks memory. If i do not need to free memory, things would be much easier. \$\endgroup\$ – KAction Oct 13 '12 at 4:02
  • \$\begingroup\$ Not sure how my code differs from yours (apart from looking nicer) :-) \$\endgroup\$ – Martin York Oct 13 '12 at 14:32
  • \$\begingroup\$ Yeah. My bad. I forgot free too. =) \$\endgroup\$ – KAction Oct 14 '12 at 7:25
1
\$\begingroup\$

Rather than 'removing' ifs, it may be better to move them around!

But please, put {} for every if, even if it has only one statement (another if). It makes it a lot easier to read.

  1. Treat the !is_comment as a precondition for the loop contents. It's right at the start, so it's obvious that you're doing something 'special' with it. Maybe even change the while to a for to push it further up.

    for ( lineno = 1; getline(&line, NULL, conf_file) != EOF; lineno++ ) 
    {
        /* skip over comments */
        if ( is_comment( line ) ) { continue; }
    
  2. Put your "header processing" part into another function, since it performs a single task. It's a little awkward with the number of parameters even so, but you're removing three levels of indentation while reading it.

You'll be left with the following (if I've got it right); it's longer, but may be more readable.

#define PARSE_FAILURE 1
#define SUCCESS 0

int parse_module_header( module_initializer_t init, char* line, int lineno, char** penvz, size_t* penvz_len )
{
    char *type; /* Type of current module */
    char *name; /* Name of current module */

    type = name = NULL;
    if ( update_header( line, &type, &name ) ) {

        if ( type ) {

            if ( (*init)( type, name, *penvz, *penvz_len ) ) {

                fprintf("failed to initialize module %s with name %s", type, name);
                return PARSE_FAILURE;
            }
        }
        clear_envz( penvz, penvz_len );
    }
    else {

        fprint( "syntax error on line %d", lineno );
        return PARSE_FAILURE;
    }
    return SUCCESS;
}

int
parse_config (FILE * conf_file, module_initializer_t init)
{
    char *line; /* Current line */
    char *envz;
    size_t envz_len;
    size_t lineno = 0;

    line = envz = NULL;
    for ( lineno = 1; getline( &line, NULL, conf_file ) != EOF; lineno++ ) 
    {
        /* skip over comments */
        if ( is_comment( line ) ) { continue; }

        if (line[0] == '[') {

            /* begin a new module section */
            int err;
            err = parse_module_header( init, line, lineno, &envz, &envz_len );
            if ( err != SUCCESS ) {

                return err;
            }
        }
        else {

            /* set a config variable for the current module */
            argz_add( &envz, &envz_len, line );
        }
    }
    return SUCCESS;
}
\$\endgroup\$
  • \$\begingroup\$ You too missed memory leak. But idea of moving lineno to for is awesome. Thanks! And, also, I do not like this braces. \$\endgroup\$ – KAction Oct 13 '12 at 4:05
1
\$\begingroup\$

You can pack together single line if sentences, but remember to put an empty line after the last one:

if (cond1) continue;
if (cond2) {printMessage(); continue;}

// Put empty line after last if block!
doRealWork();
\$\endgroup\$
  • \$\begingroup\$ Why is the empty line so important? \$\endgroup\$ – svick Oct 12 '12 at 15:50
  • \$\begingroup\$ Not loving single line if statements. \$\endgroup\$ – Martin York Oct 12 '12 at 18:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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