1
\$\begingroup\$

I'm doing K&R C Exercises. I could use a code review to see how I can improve my logic. The excercise is:

Write a program that prints its input one word per line with getchar.

My basic idea is we need to know whether or not we're in a word. We use the IN status. Every time we hit a blank space, tab, or newline, the status is changed to OUT.

Please offer only suggestions based on what i'm learning in the book up to this point, "symbolic constants, if else, and loops only." Also thoughts on my comments would be welcome.

// Exercise1.11.cpp : Defines the entry point for the console application.
/* 
 Write a program that prints its input one word per line.
*/


#include "stdafx.h"

#define IN 1 
#define OUT 0

int _tmain(int argc, _TCHAR* argv[])
{
    int status = OUT; 
    int c; 

    while ((c = getchar()) != EOF){

        // Check to see if char is not a new line, blank or space.
        if ((c != '\n' && c !=' ' && c != '\t') ){

            if (status == OUT){
                putchar('\n');
                status = IN;
            }           

            putchar(c);
        }
        else {
            // char is a new line, blank or tab, so don't process them. 
            status = OUT;
        }
    }

    return 0;
}
\$\endgroup\$
2
\$\begingroup\$

Overall it looks like you are in the right track, though I think you have overcomplicated things a bit with that custom state flag and constants. C99 gives you the bool type, which could be used instead. Failing that, I would have settled with an integer. The defines seem overkill for such a tiny program.

You can omit the return 0 at the end of main, it is implicit (C99 standard).

_tmain and _TCHAR are Microsoft extensions, added by Visual Studio when you create a project with it. Be advised that this is a compiler extension and is therefore non-portable. The standard C entry point function is called just main and it should take no arguments, unless you need to process the command line.

Prefer exposing your #include dependencies instead of relying on the auto-generated stdafx.h that Visual Studio adds for you. Your program needs at least <stdio.h>.

This is how I would have written it, using a simple bool to keep track of the state, and, as @Loki suggested, using isspace() to test the character (always avoid reinventing the wheel, unless you really mean to):

#include <stdio.h>   // getchar() & friends
#include <ctype.h>   // isspace()
#include <stdbool.h> // bool, true, false

int main(void)
{
    int c;
    bool needNewLine = false;

    while ((c = getchar()) != EOF) {

        if (isspace(c)) {
            needNewLine = true;
            continue;
        }

        if (needNewLine) {
            putchar('\n');
            needNewLine = false;
        }

        putchar(c);
    }
}
\$\endgroup\$
  • \$\begingroup\$ If there is space before the first word you get a blank line at the top. \$\endgroup\$ – Martin York Mar 29 '15 at 16:37
  • \$\begingroup\$ @LokiAstari, yes indeed, same with the OP though. My point was mainly about simplifying the code using bool and continue. Didn't want to add extra logic to handle that special case. \$\endgroup\$ – glampert Mar 29 '15 at 17:21
  • \$\begingroup\$ thank you. i'm using K & R manual, we haven't gotten to bool yet i think. \$\endgroup\$ – runners3431 Mar 31 '15 at 0:12
1
\$\begingroup\$

It all depends how you define word.

But lets assume "white space separated". There is a function to detect white space isspace() So you can simplify your test.

if (!isspace(c))
{

Your current design places a blank line at the top. You can decide if that is a bug or not.

\$\endgroup\$
0
\$\begingroup\$

Code has 2 functional issues.

1) "If there is space before the first word you get a blank line at the top." @Loki Astari This is easily fixed in OP code with int status = IN;

2) Should the last character be a non-white-space, no following '\n' is appended. The usual definition of a line a termination '\n' and so such file do not meet the required "prints its input one word per line"

The following amends these issues.

#include <ctype.h>
#include <stdlib.h>
#include <stdbool.h>

void PrintOneWordPerLine(FILE *inf) {
  bool previous_printable = false;
  for (;;) {
    int ch = fgetc(inf); // or getchar();
    if (isspace(ch) || ch == EOF) {
      if (previous_printable) {
        previous_printable = false;
        fputc('\n', stdout);  // or putchar('\n');
      }
      if (ch == EOF) {
        break;  // or return
      }
    } else {
      previous_printable = true;
      fputc(ch, stdout);
    }
  }
}

I like to avoid negation, so instead of if (!previous_whitespace), used if (previous_printable)

Recommend using isspace()

\$\endgroup\$

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.