You shouldn't use atoi
since it does not allow you to know if it wasn't able to parse its input, but returns an int anyway. But for the sake of the exercise, let us suppose it's ok since the input is good (i.e. a signed integer).
Then, some notes about your code:
split
does not need to return char *
, in fact you call it as it would return void, so change split into void split…
; in any case, keeping char *split…
, it should return NULL
, not 0
.
These lines
char* words_dup = malloc(strlen(words)+1);
strcpy(words_dup,words);
can be simply
char *words_dup = strdup(words);
Then,
while (words!='\0')
words
is a pointer (char *
), not a char, so it should be *words
or maybe you meant words != NULL
, but words
isn't changed into the loop, so you will loop forever.
I suppose then you mean to parse a "stream" of "chunks" containing word, year, definition, synonym, in a continuous stream.
If it is so, you have to word = strtok(words, "_#_")
only once, and then use word = strtok(NULL, "_#_")
. You could achieve this with something like
char *word;
// use words_dup to avoid changing words.
for (word = strtok(words_dup, sep); word != NULL; word = strtok(NULL, sep))
{
check_word(word);
// ...
}
(You should in general check if strtok returned NULL each time you try to get a new piece of information; you can exit from the for loop using break
, if you need).
If it is not so, you don't need the loop at all: call "split" for each line of your input, if you have several lines.
The use of "_#_"
works as you wanted not for the reason that likely you think (that argument is a set of character, each considered a separator alone), but since:
A sequence of two or more contiguous delimiter bytes in the parsed
string is considered to be a single delimiter
The code
char *year=strtok(NULL, "_#_");; // assigning NULL…
i=atoi(year);
check_year(i);
is fine, provided that
- we ignore
atoi
, since its usage is not recommended: input must be "good"
- we don't need to know the outcome of the check — which sounds strage (why we check?)
If you are worried about corrupting words
, use words_dup
in the first strtok
, and at the end you need only to free
it!
free(words_dup);
functions
check_word
is a misleading name: you check just the first character (in the other functions too).
check_*
functions do not return anything, so you can't take decision in the caller about the outcome of the check: the "word" is "not legal", then what it is supposed the caller (split
) should do? Your program goes on, and the user read not legal
, without s/he can realize exactly what is not legal: the "word" or the "year", or the "definition" (again, its first char)…?
Is the string all lower case?
(Do you wanted this? Then…)
You should write a function like this
int is_lowecase(const char *str)
{
while (*str != '\0') {
if (*str < 'a' || *str > 'z') return 0;
str++;
}
return 1; // anything != 0 is "read" as true
}
which returns 1 iff all the chars are lowercase (this is not the better way of doing it, but I am supposing you can't use islower
).
Then the caller, e.g. check_word
, could be:
int check_word(const char *word)
{
// only all lowercase words are legal
if (!is_lowercase(word)) {
// fprintf(stderr, "...");
// indeed I think it should be a caller duty to print
// such messages
printf("word: not legal\n");
return 0;
}
return 1;
}
The caller can take into account the return code and decide to stop the parsing (break
) on an illegal part. If you don't need it, change back to your void func(...)
signature. The idea otherwise is:
if (!check_A_Test(datum)) {
// false (0) is Wrong, true (!= 0) is Right, thus the !
// message here, if you want
break; // stop parsing
}
(If const
is unknown to you, remove it).
You can use the very same function is_lowercase
for check_definition and check_synonyms.