Removing trailing whitespace from input lines

Question

I'm going through the K&R C book (coming from Python) and am on exercise 1-18, which states:

Write a program to remove trailing blanks and tabs from each line of input, and to delete entirely blank lines.

I wasn't able to find another solution online that looks quite like mine so I am questioning whether it is truly correct, even though it appears to work on anything I throw at it. I let an ending \n be whitespace, removed it and added a general newline to each printed output, but I'm not sure if that is cheating.

If there are any bad practices (or bugs in the code) for standard C please let me know.

#include <stdio.h>

#define MAXLINE 1000

int getli(char line[], int maxline);


main() {

    int len;    
    int i;      
    char line[MAXLINE];


    while ((len = getli(line, MAXLINE)) > 0) {
        i = len-1;

        /* replace any trailing whitespace character with null character */
        while (line[i] == '\n' || line[i] == '\t' || line[i] == ' ') {
            line[i] = '\0';
            i--;
        }

        if (line[0] == '\0') {
            printf("Blank line\n");
        }

        else printf("%s\n", line);
    }

    return 0;
}

/* function to capture input line as char array */
int getli(char s[], int lim) {

    int i, c;

    for (i=0; i<lim-1 && (c=getchar()) != EOF && c != '\n'; i++) {
        s[i] = c;
    }

    if (c == '\n') {
        s[i] = c;
        i++;
    }

    s[i] = '\0';
    return i;

}

Answer 1

What you've got is very "old-fashioned-looking" C code, which is only natural for someone working through K&R. :) For example, declaring main() without a return type (that is, with the implicit int return type) is frowned upon these days.

Declaring all your variables — uninitialized! — at the top of your function is also frowned upon, in every language, because uninitialized variables in general are bad. Because uninitialized variables mean they have to be initialized later, which means assignment, which means mutation, which means more stuff for me to keep track of in my head. It's better to delay defining the variable at all until you know what's going to be put in it.

For example:

int len;    
int i;      
char line[MAXLINE];

while ((len = getli(line, MAXLINE)) > 0) {
    i = len-1;

could definitely be written as

int len;    
char line[MAXLINE];
while ((len = getli(line, MAXLINE)) > 0) {
    int i = len-1;

And then personally I'd avoid side effects inside the condition of a while loop. It's very K&R, but it's frequently not very easy to read!

while (true) {
    char line[MAXLINE];
    int len = getli(line, MAXLINE);
    if (len <= 0) break;
    int i = len-1;
    while (line[i] == '\n' || line[i] == '\t' || line[i] == ' ') {
        line[i] = '\0';
        i--;
    }

Now we can flatten that whole business with i into a simple for loop:

while (true) {
    char line[MAXLINE];
    int len = getli(line, MAXLINE);
    if (len <= 0) break;
    for (int i = len-1; (line[i] == '\n' || line[i] == '\t' || line[i] == ' '); --i) {
        line[i] = '\0';
    }

At this point I wonder whether it is in your function's contract that it zeroes out every char in the buffer after the last nonspace character, or if you could avoid writing a lot of those zeroes. One way to do that would be to re-expand the loop:

    int i = len-1;
    while (line[i] == '\n' || line[i] == '\t' || line[i] == ' ') {
        --i;
    }
    line[i+1] = '\0';

Another way would be to factor that code out into a helper function... or just use one of the many standard library functions for manipulating char buffers! Unfortunately, the function you're looking for in this case is spelled strrspn and is actually not standard, even though strspn is.

    *strrspn(line, "\n\t ") = '\0';

(I also notice that Oracle's documentation for strrspn contradicts itself. I assume that on failure it returns a pointer to the end of the string, as shown in their example code, not a pointer to the beginning of the string, as written in their prose description.)

Anyway, writing strrspn from scratch would be a good K&R-style exercise for you!

---

(len = getli(line, MAXLINE)) looks like a stutter. I don't know why you wouldn't write out the whole word getline... unless you're avoiding the library function, in which case something like read_one_line or get_a_line would be reasonable names to choose.

Don't write MAXLINE on that line, either. Don't give yourself a chance to misspell it or cut-and-paste-error your way into a bug. If you're calling getli with the buffer line, then the second argument should always be the size of the buffer — that is, sizeof line. Sure, that happens to be MAXLINE, just like MAXLINE happens to be 1000; but you should avoid repeating the word MAXLINE for the same reason you should avoid repeating the word 1000. If you call getli(line, sizeof line), you can see right at the callsite — without inspecting any other code — that the call is correct. It is correct by design.

---

Your getli function has very convoluted control flow. You have a side-effect in the condition again; and then you test for '\n' both inside and outside the loop. Here's an exercise: Rewrite the code using no loops except while (true). Then see how much clearer you can make the code while preserving that property.

---

Here's the first pass:

int getli(char s[], int lim) {
    int i = 0;
    int c;
    while (true) {
        if (i >= lim - 1) break;
        c = getchar();
        if (c == EOF) break;
        if (c == '\n') break;
        s[i] = c;
        ++i;
    }
    if (c == '\n') {
        s[i] = c;
        ++i;
    }
    s[i] = '\0';
    return i;
}

Do you spot an opportunity to combine codepaths now? Keep going; you'll find that the code ends up shorter than it started out!