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;
}
3 Answers 3
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);
}
}
-
\$\begingroup\$ If there is space before the first word you get a blank line at the top. \$\endgroup\$Loki Astari– Loki Astari2015年03月29日 16:37:08 +00:00Commented Mar 29, 2015 at 16:37
-
\$\begingroup\$ @LokiAstari, yes indeed, same with the OP though. My point was mainly about simplifying the code using
bool
andcontinue
. Didn't want to add extra logic to handle that special case. \$\endgroup\$glampert– glampert2015年03月29日 17:21:24 +00:00Commented Mar 29, 2015 at 17:21 -
\$\begingroup\$ thank you. i'm using K & R manual, we haven't gotten to bool yet i think. \$\endgroup\$runners3431– runners34312015年03月31日 00:12:16 +00:00Commented Mar 31, 2015 at 0:12
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.
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()