How can I break up this into more than two functions?

Question

How can I divide those two functions to more than two?

The functions read the file line by line.

An instruction will appear in a line in the file (at the end of each instruction there will be a newline character). At the start of the running, the program will read the instruction line by line. Then it will decode the required action and parameters and will call to perform the action with the appropriate parameters.

I tried to put the foor loop, array, getc() to another function but it doesn't work.

void read_line(FILE *fp, char *orders, char *book_name, char *book_name_2, int *num)
{
    int i = 0;
    char c ;
    *num = 0;
    c = getc(fp);
    while ((c != '\n') && (!feof(fp))) {
        for (i = 0; (c != '$') && (c != '\n') && (!feof(fp)); i++) {
            orders[i] = c;
            c = getc(fp);
        }
        orders[i] = '\0';
        if (c != '\n' && (!feof(fp))) {
            fseek(fp, 3, 1);
            c = getc(fp);
        }
        else break;
        for (i = 0; (c != '$') && (c != '\n'); i++) {
            book_name[i] = c;
            c = getc(fp);
        }
        book_name[i] = '\0';
        if (c != '\n' && (!feof(fp))) {
            fseek(fp, 3, 1);
            c = getc(fp);
        }
        else break;
        if (strcmp(orders, "Rename ") != 0) {
            for (i = 0; c != '\n'; i++) {
                *num = (*num) * 10 + (c - '0');
                c = getc(fp);
            }
        }
        else {
            for (i = 0; c != '\n'; i++) {
                book_name_2[i] = c;
                c = getc(fp);
            }
            book_name_2[i] = ' ';
            book_name_2[i + 1] = '\0';
        }
        return;
    }
}

Book* read_file_books(FILE *fp, Book *head, char *book_name, int *copies)
{
    int i = 0;
    char c ;
    *copies = 0;
    c = getc(fp);
    while ((c != '\n') && (!feof(fp))) {
        for (i = 0; (c != '$') && (c != '\n'); i++) {
            book_name[i] = c;
            c = getc(fp);
        }
        book_name[i] = '\0';
        if (c != '\n') {
            fseek(fp, 3, 1);
            c = getc(fp);
        }
        else break;
        for (i = 0; (c != '\n') && (!feof(fp)); i++) {
            *copies = (*copies) * 10 + (c - '0');
            c = getc(fp);
        }
        return add(head, book_name, *copies);
    }
    return head;
}

Two comments: 1) `char c = 0;` -->> `int c = 0;` 2) dont use `feof()` — wildplasser, Jun 08 '20 at 23:26
@wildplasser how I change the code to code without using feof() can you help me ? — , Jun 09 '20 at 00:19
`feof` is for figuring out the reason you stopped after you had to stop, not for figuring out if you should stop now. See https://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong — aschepler, Jun 09 '20 at 00:36
Jas06 - Very bad form to [delete your post](https://stackoverflow.com/questions/62410487/how-to-check-if-a-binary-number-contains-digits-different-from-0-and-1-in-c) after having several people try to help in comments, and an answer. Total slap in the face. — ryyker, Jun 16 '20 at 14:30

klutt · Accepted Answer · 2020-06-09T08:38:10.550

The most streamlined way of extracting a code block is to basically just copy the block to a function and then use pointers for variables that are declared outside that block. Let's take the for loop:

    for (i = 0; (c != '$') && (c != '\n'); i++) {
        book_name[i] = c;
        c = getc(fp);
    }
    book_name[i] = '\0';

So, we will need access to i, c, book_name and fp. The simplest (but not the best) is this:

void foo(int *i, char *c, char *book_name, FILE *fp) 
{
    for (*i = 0; (*c != '$') && (*c != '\n'); (*i)++) {
        book_name[*i] = *c;
        *c = getc(fp);
    }
    book_name[*i] = '\0';
}

And then replace the for loop with:

foo(&i, &c, book_name, fp);

That's an easy procedure. But it gives you quite a lot of pointers. That's actually nothing wrong with the method itself. But you could get rid of some of them by considering in which scope you're declaring the variables. For instance, you can declare variables inside the for header, and you should unless you want to keep the last value. If you had done that, you could remove one parameter and get

void foo(char *c, char *book_name, FILE *fp) 
{
    for (int i = 0; (*c != '$') && (*c != '\n'); i++) {
        book_name[i] = *c;
        *c = getc(fp);
    }
    book_name[i] = '\0';
}

Note that I had to peek forward until the next usage if i to determine that this was safe. Your code is simply not written in a way that makes it suitable for block extraction.

You should also try to make the c variable local. Speaking of which, it should be an int and not a char. Read the documentation of getc to understand why.

Using for loop here is not wrong per se, but it's non idiomatic. For loops are typically used when you know how many times the loop should execute before the loop starts. When you want to loop until something happens, a while is more suitable. But what would be much better here is do-while. Because, when reading files, you want to try to read, and then check if you succeeded. You are technically doing that, but what makes this code hard to extract is that each block ends with reading a character that the next loop should process.

To correct this, we need to start from the beginning. First remove the very first instance to getc. Also, remove the conditions from the loop headers.

while (1) {
    i=0;
    // Each block takes care of 100% of their input
    while((c = getc(fp)) != '$' && c != '\n' && c != EOF) {
        orders[i] = c;
        i++;
    }
    orders[i] = '\0';

    // Negated your condition to make a cleaner if and get rid of else                                             
    if (!(c != '\n' && (!feof(fp)))) break;

    fseek(fp, 3, 1);

    ...

The main thing I have corrected here is that every code block starts from scratch. It does not care about what happened before. In your code, you always started by checking the values from the previous blocks. This change makes it MUCH easier to extract code. In most cases when people ask how to extract code, the question they really should have asked is to how to change the code so that extraction becomes trivial.

Also, read Why is while(!feof(fp)) always wrong?

How can I break up this into more than two functions?

1 Answers1