HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Style and Suggestions for K&R2 1-18

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
andstylesuggestionsfor

Problem

I just completed K&R (2nd Ed.) ex 1-18. I'm fairly new to C, but have had experience in other languages (Perl, C++, Java, Common Lisp, PHP, etc) and really want to learn the C idiom as opposed to translating the idioms I already know.

What do you think about this approach to 1-18?

/*
   Exercise 1-18 in K&R 2nd Edition
   Removes trailing blanks & tabs & blank lines from each line of input
   Written by Z. Bornheimer (provided as is without warranty).
*/

#include 
#define MAXLINE 1000

/* removes trailing blanks and tabs...deletes null lines */
main()
{
    int len; /* current line length */
    int i = -1, j; /* two counters */
    int c; /* current char */
    char line[MAXLINE]; /* current input line */

    while ((c=getchar()) != EOF) {
        line[++i] = c;
        if (c == '\n') {
            j = i + 1;
            while (line[--j] == ' ' || line[j] == '\n' || line[j] == '\t')
                line[j] = '\0';

            printf("%s", line);

            for (j = 0; j <= i; j++)
                line[j] = '\0';

            i = -1;
        }
    }
    return 0;
}


https://github.com/zachbornheimer/programming-exercises/blob/master/c/1-18.c

Thank you for your help!

Solution

First the obvious: if a line is too long, your program will seriously fail (unknown memory gets overwritten, or the program crashes). Without dynamic memory allocation, I see no way to fix it. However, you could improve the situation significantly.

Since you're reading the input character by character, and you need to remove only trailing blanks and tabs, you can printout any character that is not one of these, and put only blanks and tabs in the string. Once you encounter another non-(blank or tab), you printout that string, and reset it back to a blank string. If \n is encountered, you write it out without the content of the string, as these are the trailing blanks and tabs.

You'll also need to keep track of printout and, in case nothing was printed in the line (i.e., there were no non-(blanks or tabs)), you also skip printing of \n, to delete the entirely blank lines.

If you had to remove only the trailing blanks (or only the trailing tabs), you wouldn't need a string at all -- just a counter of how many of those you've encountered. Unfortunately, your task is a bit more complex, and so is a solution.

My modification does not solve the "line is too long" problem, but it does reduce it to "a line has more than 1000 blanks+tabs in a row" problem.

Now, I give you some comments on your actual code.

First, I have one simple remark:

int i = -1, j; /* two counters */
    ...
    while ((c=getchar()) != EOF) {
        line[++i] = c;
        if (c == '\n') {
            j = i + 1;
            ...
            i = -1;


I think this would be more natural if i was the next character to be written to (instead of the last), i.e.,

int i = 0, j; /* two counters */
    ...
    while ((c=getchar()) != EOF) {
        line[i++] = c;
        if (c == '\n') {
            j = i;
            ...
            i = 0;


I think that this is also more customary approach when working on strings, since after the loop you often get to do something like s[i] = '\0' instead of s[i+1] = '\0'. This also makes it possible to use the unsigned variable, although I don't see much practical use for that.

Second, this is unnecessary:

while (line[--j] == ' ' || line[j] == '\n' || line[j] == '\t')
                line[j] = '\0';


You simply need to remember the value of i when you encounter blank or tab the first time after you encountered a character that is not one of these. You can do so easily by keeping the old value of c in a variable, let's call it oc:

oc = 'x'; /* anything non-(blank or tab), so it works properly for the empty lines */
    while ((c=getchar()) != EOF) {
        line[i++] = c;
        if ((c == ' ' || c == '\t') && (oc != ' ' && oc != '\t'))
            start_of_trailing_blanks_and_tabs = i;
        ...
        c = 'x';
    }


You could event replace oc by a properly set Boolean flag (in C, this is, of course, of type int).

And, as my last remark, this is also unnecessary:

for (j = 0; j <= i; j++)
                line[j] = '\0';


You set the end of string elsewhere in the code, and it's only the first \0 that counts.

I also have one suggestion on the style: I'd make a function to identify these characters, or just use isspace() from the ctype library. The later is not exactly what you need, as it also identifies the feed and the carriage return, but I think it is in the spirit of what is asked in the exercise.

Code Snippets

int i = -1, j; /* two counters */
    ...
    while ((c=getchar()) != EOF) {
        line[++i] = c;
        if (c == '\n') {
            j = i + 1;
            ...
            i = -1;
int i = 0, j; /* two counters */
    ...
    while ((c=getchar()) != EOF) {
        line[i++] = c;
        if (c == '\n') {
            j = i;
            ...
            i = 0;
while (line[--j] == ' ' || line[j] == '\n' || line[j] == '\t')
                line[j] = '\0';
oc = 'x'; /* anything non-(blank or tab), so it works properly for the empty lines */
    while ((c=getchar()) != EOF) {
        line[i++] = c;
        if ((c == ' ' || c == '\t') && (oc != ' ' && oc != '\t'))
            start_of_trailing_blanks_and_tabs = i;
        ...
        c = 'x';
    }
for (j = 0; j <= i; j++)
                line[j] = '\0';

Context

StackExchange Code Review Q#31262, answer score: 3

Revisions (0)

No revisions yet.