Thread: C code critique

I am still pretty new to C and I learned a lot from this forum. This is a working code (In windows by the way). I already learned that you don't have to cast the return of malloc. I am wondering if you can give me tips on how can I improve my C coding techniques or if you spot a bad habbit (like casting malloc's return value).

This function will create a file making sure all sub-directory exists before creating the file.

Code:

int edb_create_file(char * path, FILE *fl) {
    char *dirPath = strdup(path);
    if(!dirPath)
        return EDB_OUT_OF_MEMORY;
    // This will remove the file name so it will only have the directory
    *(strrchr(dirPath, '\\') + 1) = '\0';

    // Check if parent dir exist if not make sure
    // all sub-dir exist and created before adding the file
    if(access(dirPath, F_OK) != 0)
    {
        char *curPath = malloc(strlen(dirPath));    // Current Path
        if(!curPath)
            return EDB_OUT_OF_MEMORY;

        char *nxtPath = strstr(dirPath, "\\");      // Get the first occurance of '\\'
        do {
            ++nxtPath;                              // Skip "\\" so strstr() will not see it next time.
            // Copy dirPath until nxtPath
            strncpy(curPath, dirPath, nxtPath - dirPath);
            *(curPath + (nxtPath - dirPath)) = '\0';// Add '\0' to the end of curPath
            if(access(curPath, F_OK) != 0)          // Check if dir doesn't exist
                if(mkdir(curPath) != 0)             // Create dir
                    return EDB_CREATE_FAILED;       // Exit when failed

            nxtPath = strstr(nxtPath, "\\");        // Get the next dir
        } while(nxtPath);
        free(curPath);
    }
    free(dirPath);

    fl = fopen(path,"wb");
    if(fl)
        return EDB_SUCCESS;
    else
        return EDB_CREATE_FAILED;
}

I haven't run it, but it reads and looks, just wonderful.

Well done!!

strrchr will return NULL if it doesn't find a \, so you're code assumes at least one \. Since you're creating a dup of the path anyway, you may as well use the fact that you can definitely write to it: Find the first \ in dirPath; save the next char and write a zero there; use dirPath for dir tests; write saved char back; stir briskly.

The function does not in fact return a file to the caller. You want to reassign the FILE*, so the proper parameter type would be FILE**.

Originally Posted by oogabooga

strrchr will return NULL if it doesn't find a \, so you're code assumes at least one \. Since you're creating a dup of the path anyway, you may as well use the fact that you can definitely write to it: Find the first \ in dirPath; save the next char and write a zero there; use dirPath for dir tests; write saved char back; stir briskly.

Thanks! That makes it more efficient. I did add an extra free so it will free dirPath on every code path.

@whiteflags
Can you explain further why do I need **FILE instead of *FILE? I cannot fully grasp the idea yet. Why it will not return a file if I use *FILE?

Code:

int edb_create_file(char * path, FILE **fl) {
    char *dirPath = strdup(path);
    if(!dirPath)
        return EDB_OUT_OF_MEMORY;

    // Check if parent dir exist if not make sure
    // all sub-dir exist and created before adding the file
    if(access(dirPath, F_OK) != 0)
    {
        char *nxtPath = strstr(path, "\\");         // Get the first occurance of '\\'
        do {
            ++nxtPath;                              // Skip "\\" so strstr() will not see it next time.
            // Copy path until nxtPath
            strncpy(dirPath, path, nxtPath - path);
            *(dirPath + (nxtPath - path)) = '\0';   // Add '\0' to the end of curPath
            if(access(dirPath, F_OK) != 0)          // Check if dir doesn't exist
                if(mkdir(dirPath) != 0) {           // Create dir
                    free(dirPath);                  // Free before return
                    return EDB_CREATE_FAILED;       // Exit when failed
                }
            nxtPath = strstr(nxtPath, "\\");        // Get the next dir
        } while(nxtPath);
    }
    free(dirPath);

    *fl = fopen(path,"wb");
    if(*fl)
        return EDB_SUCCESS;
    else
        return EDB_CREATE_FAILED;
}

Thank you for the tips!

As you likely know, arguments are passed by value in C. This includes pointers, but pointers work because they contain the address of the object you want to actually change. You need to use FILE** because you want to change the file pointer, not the underlying file object, so you need another level of indirection.

If I called your function like this:

Code:

FILE* file;
endb_create_file("path/to/file.foo", file);

It will just change a copy of the file pointer, not my file pointer.

If I could do this:

Code:

FILE* file;
endb_create_file("path/to/file.foo", &file);

Since I passed the location of my file pointer, you can actually change it.

Originally Posted by whiteflags

As you likely know, arguments are passed by value in C. This includes pointers, but pointers work because they contain the address of the object you want to actually change. You need to use FILE** because you want to change the file pointer, not the underlying file object, so you need another level of indirection.

If I called your function like this:

Code:

FILE* file;
endb_create_file("path/to/file.foo", file);

It will just change a copy of the file pointer, not my file pointer.

If I could do this:

Code:

FILE* file;
endb_create_file("path/to/file.foo", &file);

Since I passed the location of my file pointer, you can actually change it.

I kept on thinking about what you said last night and I just finally realized how it works while brushing my teeth this morning . Thank you very much that will help me a lot.

@grumpy
I wont need to add an extra char this time since I am replacing the last '\\' with '\0' but I will remember what you said thanks.