File parsing and data management

Question

This is part of a two-part post (Part 2).

---

Here, I have two recent projects that parse a file. The first uses a loop that's kind of hard to follow, and the second uses "modes" to decide what to do.

Project 1: A "bookmark manager" for managing URLs easier than a text editor.

The data structures:

typedef struct {
 char *text;
 int length;
} string;
typedef struct {
 string *path;
 string *title;
} url;
typedef struct {
 url **urls; // An array of pointers to urls
 int length;
 int sel_url;
 int modified;
 char *filename;
} urllist;

For the data, I am considering using a linked list instead of an array.

The parser:

void parse_file(char *fname, urllist *ul){
 /* Dumb char-by-char parser that follows the syntax "URL[Title]"
 * [Title] is optional. If left unspecified, then the url is used as
 * a title instead.
 *
 * There's some basic idiot-proofing:
 * 
 * 1. Empty lines are ignored (lazy-ignore)
 * Rather, '\n' and ' ' are skipped if they begin a line.
 * " http://google.com" is still valid; it'll just take two
 * iterations to extract anything from it
 *
 * 2. lines beginning in '[' or '#' are fully ignored (full-ignore).
 * So you can write comments like "#this" or "[this]". The
 * 'comment char' has to be the first character on the line. If
 * there is a space before it, for example, the parser will think
 * it's a url.
 */
 int c;
 url *u;
 FILE *f = fopen(fname, "r");
 while(1){
 c = fgetc(f);
 if(c == -1){ /* Check EOF */
 break;
 }
 if(c != '\n' && c != ' ' && c != '[' && c != '#'){
 /* Thanks to the EOF check, I have to add the first char
 * NOW, while I still have it. This is also an opportunity
 * for a sanity check */
 u = new_url();
 string_push_char(u->path, c);
 } else if(c == '#' || c == '['){ /* full-ignore */
 while((c = fgetc(f)) != '\n');
 continue;
 } else { /* lazy-ignore */
 continue;
 }
 while(1){ /* Step through individual line */
 c = fgetc(f);
 if(c == '\n'){
 /* Line ends without title, copy path to title */
 string_copy(u->title, u->path);
 break;
 }
 if(c == '['){
 /* Title specified. */
 while(1){ /* Get it. */
 c = fgetc(f);
 if(c == ']'){
 while(fgetc(f) != '\n');
 break;
 }
 string_push_char(u->title, c);
 }
 break;
 }
 string_push_char(u->path, c);
 }
 url_push(ul, u);
 }
 fclose(f);
}

It works, but I feel like it could be done better. As I said earlier, it's hard to follow. You can tell because it's littered with comments, which I personally try to avoid unless describing a block of code as a whole.

An example file

# Cool sites
http://google.com/search?q=help+with+bananas[Google: banana help]
http://stackoverflow.com[Stack Overflow]
# YouTube stuff
http://youtube.com

Result

[Google: banana help ]
 Stack Overflow
 http://youtube.com
http://google.com/search?q=help with bananas

Answer 1

The code sure must be streamlined. I didn't find any better way to review but to rearrange the code and put my notes as comments.

 // First, you test the character against the same condition twice.
 // A more direct way would be to do it once
 if (c == '#' || c == '[') {
 while (fgetc(f) != '\n')
 ;
 continue;
 }
 if (c == ' ' || c == '\n')
 continue;
 // Notice that here you still have `c` intact (so the comment about
 // NOW is misleading). Time to initialize the `url`:
 u = new_url();
 // Since you have the first character of the `u->path`, a correct
 // idiom is a `do {} while ();` loop:
 do {
 string_push_char(u->path, c);
 c = fgetc(f);
 } while(c != '\n' && c != '[');
 // Here `c` is the reason of loop termination. Test it and proceed accordingly:
 if (c == '[') {
 while ((c = fgetc(f)) != ']') {
 string_push_char(u->title, c);
 }
 while(fgetc(f) != '\n')
 ;
 } else {
 string_copy(u->title, u->path);
 }

This almost it. The code parses the well-formed file, but will fall into an infinite loop if, for example, the title-closing bracket is missing.

As a side note, isspace(c) is strongly preferred to a manual testing.