Robustify code dealing with input

[Eric Lilja]

Hello, consider the following complete program:

#include <assert.h>
#include <ctype.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <time.h>

static int has_char(const char *, const char);
static void handle_guess(const char *, char *, const char);

int
main()
{
const char * words[] =
{
"hello", "usenet", "breakfast"
};
const char *actual = NULL;
char *current = NULL;

srand(time(NULL));

actual = words[rand() % 3];
current = calloc(strlen(actual) + 1, sizeof(char));
assert(current);
memset(current, '?', strlen(actual));

while(1)
{
char c = '\0';

if(!has_char(current, '?'))
break;

printf("The word is: %s\n", current);

printf("Type a letter: ");

c = getc(stdin);
getc(stdin);

handle_guess(actual, current, tolower(c));
}

printf("Congratulations! The word was: %s\n", current);

free(current);

return EXIT_SUCCESS;
}

static int
has_char(const char *str, const char c)
{
unsigned short i = 0;

for(i = 0; i < strlen(str); ++i)
if(str[i] == c)
return 1;

return 0;
}

static int
find_position(const char *str, const char c, const unsigned short start)
{
unsigned short i = start;

if(start > strlen(str))
return -1;

for(i = start; i < strlen(str); ++i)
if(str[i] == c)
return i;

return -1;
}

static void
handle_guess(const char *actual, char *current, const char c)
{
int position = 0;

assert(strlen(actual) == strlen(current));

if((position = find_position(actual, c, 0)) == -1)
{
printf("No match for the letter %c, sorry.\n", c);

return;
}

if(current[position] != '?')
{
printf("You already guessed the letter %c\n", c);

return;
}

current[position] = c;

while((position = find_position(actual, c, position + 1)) != -1)
{
assert(current[position] == '?');

current[position] = c;
}
}
It maintains a list of words and randomly selects a word from the list when
the program is started. The word is printed to the screen, but all
characters have been replaced with a '?'. The user is asked to guess a
letter in the word, and he if he guesses correctly the '?'s hiding that
particular letter is replaced with the letter itself. This continues until
the entire word has been revealed.

My problem is that the code for obtaining user input isn't very robust.
Say the user presses <tab>the_letter<enter>, it doesn't work anymore.
Whatever I type next, the character passed to handle_guess() is "messed up".
Here's a screen dump from such a session:
$ ./guess_word.exe
The word is: ?????????
Type a letter: b
The word is: b????????
Type a letter: a
The word is: b??a??a??
Type a letter: s <---- Here I typed <tab>s<Enter>
No match for the letter , sorry.
The word is: b??a??a??
Type a letter: i
No match for the letter
, sorry.
The word is: b??a??a??
Type a letter: t
No match for the letter
, sorry.

As you can see, it remains in a broken state because of that <tab>.
How can improve the code for dealing with user input so I can counter this
problem?

Any other comments regarding the code are welcome also.

/ E

[pete]

Eric Lilja wrote:

Hello, consider the following complete program:

#include <assert.h>
#include <ctype.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <time.h>

static int has_char(const char *, const char);
static void handle_guess(const char *, char *, const char);

int
main()
{
const char * words[] =
{
"hello", "usenet", "breakfast"
};
const char *actual = NULL;
char *current = NULL;

srand(time(NULL));

actual = words[rand() % 3];
current = calloc(strlen(actual) + 1, sizeof(char));
assert(current);
memset(current, '?', strlen(actual));

while(1)
{
char c = '\0';

if(!has_char(current, '?'))
break;

printf("The word is: %s\n", current);

printf("Type a letter: ");

c = getc(stdin);
getc(stdin);

handle_guess(actual, current, tolower(c));
}

printf("Congratulations! The word was: %s\n", current);

free(current);

return EXIT_SUCCESS;
}

static int
has_char(const char *str, const char c)
{
unsigned short i = 0;

for(i = 0; i < strlen(str); ++i)
if(str[i] == c)
return 1;

return 0;
}

static int
find_position(const char *str, const char c, const unsigned short start)
{
unsigned short i = start;

if(start > strlen(str))
return -1;

for(i = start; i < strlen(str); ++i)
if(str[i] == c)
return i;

return -1;
}

static void
handle_guess(const char *actual, char *current, const char c)
{
int position = 0;

assert(strlen(actual) == strlen(current));

if((position = find_position(actual, c, 0)) == -1)
{
printf("No match for the letter %c, sorry.\n", c);

return;
}

if(current[position] != '?')
{
printf("You already guessed the letter %c\n", c);

return;
}

current[position] = c;

while((position = find_position(actual, c, position + 1)) != -1)
{
assert(current[position] == '?');

current[position] = c;
}
}

It maintains a list of words and randomly selects a word from the list when
the program is started. The word is printed to the screen, but all
characters have been replaced with a '?'. The user is asked to guess a
letter in the word, and he if he guesses correctly the '?'s hiding that
particular letter is replaced with the letter itself. This continues until
the entire word has been revealed.

My problem is that the code for obtaining user input isn't very robust.
Say the user presses <tab>the_letter<enter>, it doesn't work anymore.
Whatever I type next, the character passed to handle_guess() is "messed up".
Here's a screen dump from such a session:
$ ./guess_word.exe
The word is: ?????????
Type a letter: b
The word is: b????????
Type a letter: a
The word is: b??a??a??
Type a letter: s <---- Here I typed <tab>s<Enter>
No match for the letter , sorry.
The word is: b??a??a??
Type a letter: i
No match for the letter
, sorry.
The word is: b??a??a??
Type a letter: t
No match for the letter
, sorry.

As you can see, it remains in a broken state because of that <tab>.
How can improve the code for dealing with user input so I can counter this
problem?

Any other comments regarding the code are welcome also.

/ E

/* BEGIN new.c */

#include <ctype.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <time.h>

static void
handle_guess(const char *, char *, const int);

int
main(void)
{
const char * words[] = {
"hello", "usenet", "breakfast"
};
const char *actual = NULL;
char *current = NULL;

srand(time(NULL));
actual = words[rand() % 3];
current = calloc(strlen(actual) + 1, sizeof(char));
if (current == NULL) {
exit(EXIT_FAILURE);
}
memset(current, '?', strlen(actual));
while (strchr(current, '?') != NULL) {
int c;

printf("The word is: %s\n", current);
printf("Type a letter: ");
fflush(stdout);
do {
c = getc(stdin);
} while (!isalpha(c));
getc(stdin);
handle_guess(actual, current, tolower(c));
}
printf("Congratulations! The word was: %s\n", current);
free(current);
return EXIT_SUCCESS;
}

static void
handle_guess(const char *actual, char *current, const int c)
{
char *position;

position = strchr(actual, c);
if (position == NULL) {
printf("No match for the letter %c, sorry.\n", c);
return;
}
if (current[position - actual] != '?') {
printf("You already guessed the letter %c\n", c);
return;
}
do {
current[position - actual] = (char)c;
position = strchr(position + 1, c);
} while (position != NULL);
}

/* END new.c */

--
pete

[Barry Schwarz]

On Sat, 4 Jun 2005 04:09:37 +0200, "Eric Lilja"
<mi****************************@gmail.com> wrote:
snip

printf("Type a letter: ");

c = getc(stdin);
Hopefully a letter.
getc(stdin);
To eat the \n produced by the enter key

snip

It maintains a list of words and randomly selects a word from the list when
the program is started. The word is printed to the screen, but all
characters have been replaced with a '?'. The user is asked to guess a
letter in the word, and he if he guesses correctly the '?'s hiding that
particular letter is replaced with the letter itself. This continues until
the entire word has been revealed.

My problem is that the code for obtaining user input isn't very robust.
Say the user presses <tab>the_letter<enter>, it doesn't work anymore.
Whatever I type next, the character passed to handle_guess() is "messed up".

You could use fgets to read into a three character string. Then
confirm the second character is \n insuring the user typed exactly one
character before pressing enter. Then use isalpha() or islower() to
verify he typed a valid character. If any of these tests fail, tell
him to follow the instructions more closely and loop back to repeat
the prompt.
<<Remove the del for email>>

[CBFalconer]

Eric Lilja wrote:

Hello, consider the following complete program:
.... snip code ...
It maintains a list of words and randomly selects a word from the
list when the program is started. The word is printed to the
screen, but all characters have been replaced with a '?'. The user
is asked to guess a letter in the word, and he if he guesses
correctly the '?'s hiding that particular letter is replaced with
the letter itself. This continues until the entire word has been
revealed.

My problem is that the code for obtaining user input isn't very
robust. Say the user presses <tab>the_letter<enter>, it doesn't
work anymore. Whatever I type next, the character passed to
handle_guess() is "messed up". Here's a screen dump from such a
session: .... snip ...
As you can see, it remains in a broken state because of that
<tab>. How can improve the code for dealing with user input so
I can counter this problem?

Any other comments regarding the code are welcome also.

Here is a slightly more robust version of your code. Note the
handling of EOF and the use of flushln. Your basic structure is
fairly good, in that you saw off portions of the problem for
handling by functions, and check the data. You could go further in
this direction. I recommend the consistent use of define before
use. This avoids unnecessary isolated prototypes and their
maintenance.

#include <stdio.h>
#include <string.h>
#include <assert.h>
#include <stdlib.h>
#include <time.h>
#include <ctype.h>

/* ---------------- */

static void handle_guess(const char *actual, char *current,
const char c)
{
int position = 0;
char *posn;

assert(strlen(actual) == strlen(current));

if (!(posn = strchr(actual, c))) {
printf("No match for the letter %c, sorry.\n", c);
return;
}
else {
do {
position = posn - actual;
if (current[position] == c) {
printf("You already guessed the letter %c\n", c);
return;
}
else if (actual[position] == c)
current[position] = c;
} while (*(++posn));
}
} /* handle_guess */

/* ---------------- */

static int has_char(const char *str, const char c)
{
unsigned i;

/* when you see strlen within a loop,
look for ways to avoid move it to initialization */
for (i = strlen(str); i-- > 0;)
if (str[i] == c) return 1;
return 0;
} /* has_char */

/* ---------------- */

static void flushln(FILE *fp)
{
int ch;

while (('\n' != (ch = getc(fp))) && (EOF != ch)) continue;
} /* flushln */

/* ---------------- */

int main(void)
{
const char * words[] = {
"hello", "usenet", "breakfast"
};
const char *actual;
char *current, ch;

srand(time(NULL));

actual = words[rand() % 3];
if (!(current = malloc(1 + strlen(actual))))
return EXIT_FAILURE;
memset(current, '?', strlen(actual));
current[strlen(actual)] = '\0';

while (has_char(current, '?')) {
printf("The word is: %s\n", current);
printf("Type a letter: "); fflush(stdin);
if (EOF == (ch = getc(stdin))) return 0;
flushln(stdin);

handle_guess(actual, current, tolower(ch));
}
printf("Congratulations! The word was: %s\n", current);

free(current);
return EXIT_SUCCESS;
} /* main */

--
"If you want to post a followup via groups.google.com, don't use
the broken "Reply" link at the bottom of the article. Click on
"show options" at the top of the article, then click on the
"Reply" at the bottom of the article headers." - Keith Thompson

[pete]

CBFalconer wrote:

char *current, ch; if (EOF == (ch = getc(stdin))) return 0;

I don't think that EOF is guaranteed
to be within the range of ch.

--
pete

[Emmanuel Delahaye]

Eric Lilja wrote on 04/06/05 :

static int has_char(const char *, const char);

'char' parameters don't really exist. There are converted to int
(probably with some extra code). Stick to simplicity:

static int has_char(const char *, int char);

--
Emmanuel
The C-FAQ: http://www.eskimo.com/~scs/C-faq/faq.html
The C-library: http://www.dinkumware.com/refxc.html

..sig under repair

[Eric Lilja]

"Eric Lilja" wrote:
[My original message snipped]

Thanks for all the replies! Now I'm sure I can improve the input code, and
other things as well, thanks!

/ Eric

[Eric Sosman]

Emmanuel Delahaye wrote:

Eric Lilja wrote on 04/06/05 :

static int has_char(const char *, const char);

'char' parameters don't really exist. There are converted to int
(probably with some extra code). [...]

6.5.2.2/7 and /8 appear to disagree with this claim.

--
Eric Sosman
es*****@acm-dot-org.invalid

[Paul Mesken]

On Sat, 04 Jun 2005 06:11:36 GMT, pete <pf*****@mindspring.com> wrote:

CBFalconer wrote:

char *current, ch;

if (EOF == (ch = getc(stdin))) return 0;

I don't think that EOF is guaranteed
to be within the range of ch.

Yes, this is a FAQ (12.1). It should be bigger than char (because EOF
is an "out of band" return value).

[CBFalconer]

pete wrote:

CBFalconer wrote:

char *current, ch;

if (EOF == (ch = getc(stdin))) return 0;

I don't think that EOF is guaranteed
to be within the range of ch.

Gulp - goofed. Should be int. I added that test at the last
minute when an EOF left the program chasing its tail.

--
"If you want to post a followup via groups.google.com, don't use
the broken "Reply" link at the bottom of the article. Click on
"show options" at the top of the article, then click on the
"Reply" at the bottom of the article headers." - Keith Thompson

[pete]

Eric Sosman wrote:

Emmanuel Delahaye wrote:

Eric Lilja wrote on 04/06/05 :

static int has_char(const char *, const char);

'char' parameters don't really exist. There are converted to int
(probably with some extra code). [...]

6.5.2.2/7 and /8 appear to disagree with this claim.

On a related topic,
I don't think it's usually a good idea to use small arithmetic types.
A common exception would be except in arrays,
and I'm sure there are many other exceptional cases.

Even though there's no promotion here:
static int has_char(const char *, const char);
the facts is that small (lower ranking than int) arithmetic types
do get automatically converted a lot.
Sometimes they can change from unsigned to signed,
in ways that aren't really that obvious.
In a situation where INT_MAX equals USHRT_MAX,
trying to increment an unsigned short until it rolls over
yields undefined behavior.

A single instance of small arithmetic type represents a potential
saving of memory,
but it is just as likely to be implemented on a int boundary
with masking operations.

--
pete

[pete]

CBFalconer wrote:

static void handle_guess(const char *actual, char *current,
const char c)
{
int position = 0;
char *posn; position = posn - actual;

On June 3, 6:22 pm, you said
"So, in general, pointer subtraction serves no useful purpose."

http://groups-beta.google.com/group/...e716628c?hl=en

Are you sticking with that story?

--
pete

[CBFalconer]

pete wrote:

CBFalconer wrote:

static void handle_guess(const char *actual, char *current,
const char c)
{
int position = 0;
char *posn;

position = posn - actual;

On June 3, 6:22 pm, you said
"So, in general, pointer subtraction serves no useful purpose."

Are you sticking with that story?

Yes. This is not "in general". This is two pointers to the same
object, namely the string *actual.

--
"If you want to post a followup via groups.google.com, don't use
the broken "Reply" link at the bottom of the article. Click on
"show options" at the top of the article, then click on the
"Reply" at the bottom of the article headers." - Keith Thompson

[Keith Thompson]

CBFalconer <cb********@yahoo.com> writes:

pete wrote:

CBFalconer wrote:

static void handle_guess(const char *actual, char *current,
const char c)
{
int position = 0;
> char *posn;

position = posn - actual;

On June 3, 6:22 pm, you said
"So, in general, pointer subtraction serves no useful purpose."

Are you sticking with that story?

Yes. This is not "in general". This is two pointers to the same
object, namely the string *actual.

Your original statement was ambiguous. It could easily be interpreted
to mean that pointer subtraction is never useful. I think what you
meant is that pointer subtraction is not always useful.

--
Keith Thompson (The_Other_Keith) ks***@mib.org <http://www.ghoti.net/~kst>
San Diego Supercomputer Center <*> <http://users.sdsc.edu/~kst>
We must do something. This is something. Therefore, we must do this.