2
\$\begingroup\$

I just finished my very simple and beginner-friendly C project and wanted to hear your thoughts and opinions about ways to improve it or something.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

void getFile();
void createFile();

int main()
{
    createFile();
    getFile();
    return 0;
}

void createFile()
{
    char userfilename[40];
    char filecontent[1000];
    FILE *filePointer;

    printf("How do you want to name your file? ");
    scanf("%s", userfilename);
    filePointer = fopen(userfilename, "w");

    printf("Enter file content: ");
    scanf ("%999[^\n]", filecontent);
    getchar();
    fgets (&filecontent, 1000, stdin);
    fputs(filecontent, filePointer);

    fclose(filePointer);
    printf("Written file successful.\n");
}

void getFile()
{
    int c;
    char fileName[100];
    FILE *filePointer;

    printf("\nEnter filename: ");
    gets(fileName);
    printf("\n");

    filePointer = fopen(fileName, "r");
    if(filePointer == NULL)
    {
        printf("File not found.\n");
    }
    else
    {
        int count = 0;
        printf("File contents:\n");
        while ((c = getc(filePointer)) != EOF)
        {
            putchar(c);
            if(c == ' ')
            {
                count++;
            }
        }
        printf("\n");
        printf("Number of words: %d", count + 1);
        fclose(filePointer);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Are you sure this is how the actual indentation looks? Remember this site eats the first four spaces you type. \$\endgroup\$ – jacwah Nov 23 '16 at 19:33
  • \$\begingroup\$ Yeah I know! I have indented the code correctly in my IDE but this site is always giving me trouble when I input code as I then have to manually push some parts of the code 4 spaces right. Will edit it! thx \$\endgroup\$ – user123850 Nov 23 '16 at 19:54
  • \$\begingroup\$ It's simple: Paste it as you have it, then mark it all and select the code-button {} or press ctrl+k to mark as code. \$\endgroup\$ – Deduplicator Nov 23 '16 at 20:02
  • \$\begingroup\$ Oh thank you!!! It was driving me nuts lol \$\endgroup\$ – user123850 Nov 23 '16 at 20:09
2
\$\begingroup\$

Nice first question. It can definitely be expanded on for additional learning in the future.

Improve Error Handling

Both the functions createFile() and getFile() could be changed to return int. This would allow the program to exit with the correct status.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int createFile();                 // return EXIT_SUCCESS or EXIT_FAILURE
int getFile();                    // return EXIT_SUCCESS or EXIT_FAILURE

int main()
{
    int status = EXIT_SUCCESS;    // EXIT_SUCCESS and EXIT_FAILURE are defined in stdlib.h

    status = createFile();
    if (status != EXIT_SUCCESS)
    {
        return status;
    }

    status = getFile();

    return status;
}

In getFile() the variable filePointer is tested after fopen(). This is a good practice. In createFile() the variable filePointer is not tested, this is a bad practice. The C library function fopen() for write can fail for a number of reasons:

  • File exists and is protected.
  • File exists and it is a directory.
  • The program doesn't have sufficient privilege to write to the target directory.

There are more ways it can fail, this is a partial list. See full list of possible error codes. The C library function perror() can be used to report program error in the case of I/O failures or the include file errno.h can be included and the global variable errno will contain the error code for better error messages as shown in this stackoverflow question.

These same error messages can be used to improve the error reporting in getFile() as well.

The existing error message in getFile() might be improved by adding the file name to the error message.

fprintf(stderr, "File %s not found.\n", fileName);

Error messages should generally be reported using fprintf(stderr, FMT); rather than reporting the error to stdout using printf().

Inconsistent File Name Size

In getFile() fileName can be up to 99 characters (strings must be null terminated), in createFile() it can be 39 characters, this is inconsistent and may confuse the user. The code also doesn't tell the user how long the file name can be and that could be a problem for the user. In neither case is the length of the string checked prior to use, and that could lead to a number of errors including buffer overflow.

A better way to handle the file name size might be to define a symbolic constant using #define.

#define MAX_FILE_SPEC_LENGHT 1024

    char userfilename[MAX_FILE_SPEC_LENGHT];

    char fileName[MAX_FILE_SPEC_LENGHT];

The file name variable should be large enough to handle a full path as well as the file name.

Many systems define the maximum file name size based on the file system implementation. On Linux the include file linux/limits.h defines that maximum file name size using PATH_MAX. Most operating systems will have some way of defining it.

Possible Bug

This line was flagged by my C compiler with a warning:

    fgets (&filecontent, 1000, stdin);

The warning is Incompatible pointer types passing char (*)[1000] to parameter of type char *. Since file content is an array of chars it only needs the name filecontent rather than the address of filecontent, to use the address, it should be &filecontent[0].

Inconsistent Variable Naming Conventions

In the function createFile() there are 3 variables, userfilename, filecontent and filePointer, the variable filePointer is camelCase and is clear, userfilename and filecontent are not camelCase and not quite as clear.

\$\endgroup\$

Your Answer

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