7
\$\begingroup\$

Recently I have started learning C++ and I decided to learn C along the way to improve my understanding of how computers work. For C I am using "The C Programming Language Book" by Brian Kernighan and Dennis Ritchie.

The exercise in question is to print all input lines disregarding trailing whitespace and empty lines.

Here is my code:

#include <stdio.h>
#include <stdlib.h>
#define MAX_BLANKS 5

int main()
{
    int c; /*holds current char to be printed*/
    int* wsbuffer; /*buffer to hold possible trailing whitespace*/
    int buffer_size; /*size of wsbuffer*/
    int nws; /*number of whitespaces in buffer*/
    int nonempty; /*0 if current line is empty, 1 else*/
    int i; /*for loop*/
    nonempty  = nws = 0;
    buffer_size = MAX_BLANKS;
    wsbuffer = (int*) malloc(buffer_size*sizeof(int));

    while (EOF != (c=getchar()))
    {
        if (c==' ' || c=='\t')
        {
            ++nws;
            if (nws>buffer_size)
            { 
                buffer_size *=2;
                wsbuffer = (int*) realloc(wsbuffer,buffer_size*sizeof(int));
            }
            wsbuffer[nws-1] = c;
        }
        else if (c=='\n')
        {
            if (nonempty==1) {
                putchar(c);
                nws = nonempty = 0;
            }
        }
        else
        {
            nonempty = 1;
            for (i=0;i<nws;++i)
                putchar(wsbuffer[i]);
            nws = 0;
            putchar(c);
        }
    }
    free(wsbuffer);
    return 0;
}

I compiled this using gcc -std=c90 -Wall -Wextra -pedantic to adhere to the ISO protocol used by the book. It compiled fine with no warnings.

I deliberately set MAX_BLANKS to 5 to test the code. I tested a few cases (empty lines, lines with only space, spaces at first etc) and the result seemed okay.

Since memory allocation is not yet covered in the book, I'd particularly like it if someone could take a look and let me know if I have any issues particularly with memory leaking or something. Of course, any other suggestions or improvements would be welcome!

\$\endgroup\$
  • \$\begingroup\$ Didja use a profiling tool? Those work well for discovering leaks. \$\endgroup\$ – Richard Barker Sep 4 at 6:32
  • \$\begingroup\$ @Richard I'm not sure what this is? \$\endgroup\$ – John Sep 4 at 13:24
  • \$\begingroup\$ Bugs are to the debugger as performance is to the profiler. \$\endgroup\$ – Richard Barker Sep 4 at 19:34
  • \$\begingroup\$ @RichardBarker How do you use a profiler? I never figured out how. \$\endgroup\$ – JL2210 Sep 5 at 15:20
  • \$\begingroup\$ Depends on the language and tool but it's mostly universal. visual studio has one built in and there are plenty of resources out there about it \$\endgroup\$ – Richard Barker Sep 5 at 15:31
7
\$\begingroup\$

Don't cast the return value from malloc and family - provided there's a prototype in scope (which there is here, due to our #include <stdlib.h>), the void* result will convert to any pointer type.

Always check whether the returned pointer is a null pointer before using it:

wsbuffer = malloc(buffer_size * sizeof *wsbuffer);
if (!buffer) {
    fputs("Memory allocation failure\n", stderr);
    return EXIT_FAILURE;
}

Robust code needs to take extra care when using realloc(). If we write something like p = realloc(p, new_size); then we have a problem when it fails - p is assigned null, and we have nothing pointing to the memory any more (i.e. a memory leak). The usual pattern looks something like:

void *new_buf = realloc(wsbuffer, buffer_size * sizeof *wsbuffer);
if (!new_buf) {
    free(wsbuffer);
    fputs("Memory allocation failure\n", stderr);
    return EXIT_FAILURE;
}
wsbuffer = new_buf;

Minor things:

  • Instead of c==' ' || c=='\t', we might consider isspace(c), remembering that this test includes newlines and other whitespace.
  • We can store whitespace in an array of char rather than int, since we never need to store EOF into that buffer.
  • The logic might be simpler if use a larger buffer and read a whole line at a time. Then we just overwrite the start of the last whitespace found in the line, and print it. That's likely a bit more efficient than our character-at-a-time operation, too.
\$\endgroup\$
  • \$\begingroup\$ Thanks Toby! I didn't get the last part about the larger buffer? Do you mean just storing all input and printing them out when I encounter \n or EOF? \$\endgroup\$ – John Sep 4 at 13:24
  • \$\begingroup\$ Might be better to make isspace(c) a link to cplusplus.com/reference/cctype/isspace \$\endgroup\$ – pacmaninbw Sep 4 at 13:31
  • 1
    \$\begingroup\$ I meant that we should consider reading a whole line (using fgets() - N.B. not gets() which is irretrievably flawed), and then process and print the line before reading the next line. \$\endgroup\$ – Toby Speight Sep 4 at 13:37
  • 1
    \$\begingroup\$ No need to rush; you can work your way through the book and then revisit earlier chapters and self-review your earlier work in the light of the later knowledge. I find that a good learning technique. \$\endgroup\$ – Toby Speight Sep 4 at 13:44
  • 1
    \$\begingroup\$ OP's getchar() approach has an advantage over fgets(): it well handles reading/writing rare null characters. With fgets(), those readily are lost. \$\endgroup\$ – chux - Reinstate Monica Sep 5 at 9:05
5
\$\begingroup\$

This is in addition to @TobySpeight review.

Standard Symbolic Constants
Since stdlib.h is included, you have access to EXIT_SUCCESS and EXIT_FAILURE which are standard macros implemented on all systems. These work in C++ as well as C. These are more portable than return 0 or return 1. Most modern C compilers will append return 0; so it really isn't necessary in this particular case. You may have noticed the use of EXIT_FAILURE in @TobySpeight answer.

Sizeof in malloc and realloc
A safer practice is to use what the variable points to rather than a specific type. This will allow you to change the type of the array without changing each malloc or calloc.

wsbuffer = malloc(buffer_size * sizeof(*wsbuffer));

As @TobySpeight mentioned the return value of every memory allocation should be tested for NULL.

Boolean Values
Somewhere in the book you should find:

#define TRUE 1
#define FALSE 0

This might make the code more readable. The C standard has progressed; since the second version of the book was written there is an additional header file that can be used for Booleans, <stdbool.h>.

Programming Style
Generally it is better to wrap operators in expressions in spaces to make the code more readable.

    for (i = 0; i < nws; ++i)

    if (c == ' ' || c == '\t')

For code maintenance reasons, a safer practice when using control constructs (loops, if, else) is to always enclose even a single statement within a block ({..}):

        for (i = 0; i < nws; ++i) {
            putchar(wsbuffer[i]);
        }

That reduces bugs introduced during code maintenance where a second line needs to be added within the control block.

Variable Declarations
When the book was written, all variables had to be declared at the top of the function. This isn't true anymore.

In C, as in C++, variables can be declared as they are needed. For instance a loop control variable such as i can be declared just before the loop.

\$\endgroup\$
  • \$\begingroup\$ "These are more portable than return 0 or return 1" That's not true, where did you get that idea from? \$\endgroup\$ – Lundin Sep 5 at 11:47
  • \$\begingroup\$ "This wasn't true when the second version of the book came out" This is wrong, the 2nd edition was published in 1988. It barely conforms to the C90 standard. It wasn't possible to declare variables anywhere until C99 in 1999. \$\endgroup\$ – Lundin Sep 5 at 11:52
  • \$\begingroup\$ @Lundin I have removed the the comment about the second version of the book. I'm looking for the reference about the portability, however, I believe it was an answer on stack overflow. \$\endgroup\$ – pacmaninbw Sep 5 at 12:05
  • \$\begingroup\$ @Lundin stackoverflow.com/questions/8867871/… \$\endgroup\$ – pacmaninbw Sep 5 at 12:08
  • 1
    \$\begingroup\$ @JL2210 you are correct, not enough coffee. Fixed \$\endgroup\$ – pacmaninbw Sep 8 at 1:25

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.