1
\$\begingroup\$

I know that multiple functions are already available. However, I thought of writing my own because I wanted to learn the logic (and also because I thought there wasn't enough confusion :P). Please review the function I wrote and suggest me efficient changes.

Without further ado, here I go:

scan(string,size)
char** string;
size_t size;
{
    string[0]=(char*)malloc(sizeof(char));
    char keystroke=' ';
    while((keystroke=getc(stdin))!='\n') {
        string[0][size++]=keystroke;
        string[0]=(char*)realloc(string[0],size+1);
    }
    string[0][size]='\0';
    return size;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Jan 29 at 8:12
4
\$\begingroup\$

Your code is horrible:

  • You don't check for end-of-file, which will lead to endless loops
  • You don't check for failure of malloc or realloc
  • You call realloc way too often, which makes the code slow for large lines of input
  • You cast the result of malloc
  • You use the outdated style of function definition from the 1980s
  • The variable string is not a string but points to an array of pointers to strings, which is confusing
  • The #include for size_t is missing
  • Using [0] instead of * is extremely confusing
  • There is no need to initialize keystroke to a space character
  • The parameter size is useless since the only possible value that makes sense for it is 0
  • The return type of the function is implicitly int, which is obsolete
  • The returned value is of type size_t, which doesn't fit into an int and additionally differs in signedness
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ @d4rk4ng31 "why would I check for EOF?" --> to know when stdin has no more data to provide. \$\endgroup\$ – chux - Reinstate Monica Jan 29 at 13:47
  • 3
    \$\begingroup\$ @d4rk4ng31 what if the input ends before you can read a '\n'? \$\endgroup\$ – L. F. Jan 29 at 13:52
  • 3
    \$\begingroup\$ @d4rk4ng31, When stdin is closed, getc(stdin) returns EOF - that is the true end of input. Your code, instead of stopping, goes into an infinite loop. '\n' is only the end of a line. \$\endgroup\$ – chux - Reinstate Monica Jan 29 at 13:53
  • 2
    \$\begingroup\$ @d4rk4ng31 It does not wait, it loops, consuming memory until an allocation failed and then UB of attempting to de-reference a NULL. Loop should quit on either '\n' and NULL. \$\endgroup\$ – chux - Reinstate Monica Jan 29 at 13:57
  • 1
    \$\begingroup\$ @d4rk4ng31 Because you have not tested it in enough different cases. \$\endgroup\$ – chux - Reinstate Monica Jan 29 at 13:58
2
\$\begingroup\$

Code does not handle end-of-file or rare input error well as there is no attempt to detect EOF.

Perhaps something like

// char keystroke=' ';
int keystroke=' ';
while((keystroke=getc(stdin))!='\n' && keystroke!= EOF) {
    string[0][size++]=keystroke;
    string[0]=(char*)realloc(string[0],size+1);
}
if (keystroke == EOF && size == 0) {
   free(string[0]);
   string[0] = NULL;
   return -1;  // Maybe use (size_t)-1 to indicate EOF.
}

Fuller example code

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ any particular reason to use int keystroke over char keystroke ? \$\endgroup\$ – d4rk4ng31 Jan 29 at 14:59
  • 2
    \$\begingroup\$ @d4rk4ng31 int getc(stdin) returns type int and typically 257 different values. To save in char loses information. Using char, cannot distinguish EOF from a valid character. \$\endgroup\$ – chux - Reinstate Monica Jan 29 at 15:20

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.