C program for whitespace normalization

Question

I have made a beginner program using C that uses file handling and formats the given file. Formatting includes removing trailing blank spaces, replacing one or more blanks by a single blank and also wrapping lines. It would be extremely helpful if you could point out the mistakes I have made and also if I add any other features to file formatting.

MAIN.C

#include <stdio.h>
#include <stdlib.h>
#include "whiteSpacing.c"
#include "lineFolding.c"

FILE *p;
FILE *q;
char c;

int main(int argc, char *argv[])
{
    if(argc != 2)
    {
        printf("Error! Check number of arguements.\n");
        exit(-1);
    }

    p = fopen(argv[1], "r");                //original file containing input 
    q = fopen("temporary.txt", "w");

    if(p == NULL || q == NULL)          //error checking, also asks user if file is to be created.
    {
        printf("ERROR! Unable to open file\n");
        printf("Do you want to create '%s' file ? (y/n)\n", argv[1]);
        c = getchar();

        if(c == 'y' || c == 'Y')
        {
            p = fopen(argv[1], "w+");
            printf("Type here to write to the file\n");

            while((c = getchar()) != EOF)
            {
                putc(c, p);
            }
            fclose(p);

            p = fopen(argv[1], "r");
        }

        else
            exit(1);
    }

    while((c = getc(p)) != EOF)   // for line wrapping
    {
        folding();
    }

    fclose(p);
    fclose(q);

    p = fopen("temporary.txt", "r");  // original file will be overwritten
    q = fopen(argv[1], "w");

    if(p == NULL || q == NULL)
    {
        printf("ERROR! Unable to open file\n");
        exit(1);
    }

    while((c = getc(p)) != EOF)         //removes trailing spaces/tabs, also replaces two or more white space by a single space.
    {
        whiteSpacing();
    }

    remove("temporary.txt");

    printf("Text in \"%s\" was formatted and overwritten.\n", argv[1]);
    fclose(p);
    fclose(q);

    return 0;
}

linefolding.c

#define MAXLEN 65       //length for wrapping
#define MAXLINE 1000    // maximum length for one line

extern FILE *p;
extern FILE *q;
extern char c;

int getLine(char *);
void trim(char *);

void folding(void)
{
    char line[MAXLINE];
    int len;

    while((len = getLine(line)) > 1) // proceed only if length of line is greater than 1
    {
        if(len > MAXLEN)    // fold the length only if length of line is greater than the MAXLEN.
        {
            trim(line);
        }
        fprintf(q, "%s", line);
    }

}

int getLine(char line[])
{
    int nc = 0;
    while(((c != EOF)) && c != '\n')  // store each character of a line in an array.
    {
        line[nc] = c;
        nc++;
        c = getc(p);
    }
    if(c == '\n')
        line[nc] = '\n';

    line[nc + 1] = '\0';

    return nc;
}

void trim(char line[])
{
    int i = 0;
    int lastBlank = 0;
    int j = 1;

    for(i = 0; line[i] != '\0'; i++)
    {
        if(line[i] == ' ' || line[i] == '\t' || line[i] == '\n' || line[i] == ',' || line[i] == '.')   // kees track of the last white Spacing or non alphabetical character .
        {
            lastBlank = i;
        }

        if(i == MAXLEN * j)    //prints a new line at every MAXLEN interval
        {
            line[lastBlank] = '\n';
            j++;
        }
    }

    line[i] = '\n';
    line[i + 1] = '\0';
}

whiteSpacing.c

#include <stdio.h>

extern FILE *p;
extern FILE *q;
extern char c;

void whiteSpacing()
{
    while(c == ' ')   //replaces two or more spaces by a single space
    {
        c = getc(p);
        if (c != ' ' && c != EOF)
            putc(' ', q);

    }
    putc(c, q);
}

Answer 1

I see a number of things that could help you improve your code.

Don't #include .c files

The code has these two lines

#include "whiteSpacing.c"
#include "lineFolding.c"

Generally speaking, we divide code into .h files which define an interface and .c files which contain an implementation. For example, a lineFolding.h file might look like this:

#ifndef LINEFOLDING_H 
#define LINEFOLDING_H 
#include <stdio.h>
void folding(FILE *p, FILE *q, char c);
#endif // LINEFOLDING_H 

Although there are other functions within the lineFolding.c file, they are only used internally to that module and so they aren't listed in the interface. Also note that the include of <stdio.h> is required because the interface includes two FILE * parameters. It also incorporates the next suggestion.

Eliminate global variables where practical

Having routines dependent on global variables makes it that much more difficult to understand the logic and introduces many opportunities for error. Eliminating global variables where practical is always a good idea, whether programming for desktop machines or for embedded systems. In this particular case p, q and c should be passed to the other functions (if needed) and local to main.

Use better naming

Function names like getLine are good because the names suggest what they're doing. Names like folding are not as good because the name is very generic and doesn't describe very well what the function is doing. Names like p and q are not good at all because they don't tell the reader anything at all about what the variable means in the context of the program. I'd rename them infile and outfile and then the program is much easier to read and understand.

Fix the bug

In the getLine function, what happens if line doesn't happen to contain a '\n' character? The answer is that the program continues to march through memory well beyond the allocated size of the buffer. This is an error and it must be fixed. Fortunately, it's not that hard to do. One way to do that is to add it as a condition for the while loop:

while(((c != EOF)) && c != '\n' && nc < MAXLINE-2) 

Consider the user

If the user of this program does not supply an input filename, the program prints this unhelpful message:

Error! Check number of arguements.

I'd suggest that something a little more instructive (and without the spelling error) might be better. Consider instead using something like this:

printf("Usage: %s filename\n", argv[0]);

Also, instead of having a hardcoded filename temporary.txt and doing the formatting in several steps, it might be nice to allow the user to control the name and location of a sepaate output file. For this, it would make sense to use another command line argument. Also, while having the user type the file if the file doesn't exist might be handy, I'd suggest that doing just one thing well is a better idea. In particular, I'd write this using only stdin and stdout so that the user could call it with redirected input and output if they care to deal with files, or without if they'd like to type directly.

Reconsider the function interfaces

Consider the whiteSpacing function. Its basic function is to replace double spaces with a single space. However, it's called after the file has already been adjusted to introduce line wrapping. For this reason, I'd suggest that a better way to do this would be to pass a buffered line to the function instead. This would allow the program to do a single pass through the file instead of requiring two.

Be careful with function sequencing

The last lines of the code include this:

remove("temporary.txt");
printf("Text in \"%s\" was formatted and overwritten.\n", argv[1]);
fclose(p);

It will probably work, but it contains a flaw in that the file p is removed and then the file is closed. I'd suggest closing it first.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.