Is this the perfect input function?

Since 'scanf()' is really ugly dealing with inputs , i decided to write a function that will hopefully be perfect when asking for numbers from the user.

The function is called 'getl()' (get long), it has 3 arguments and need no extra header files to be included. First and Second argument let you specify the range of max\min integer allowed to be input. Third takes a costumized message that will be displayed in these cases :

1. When the user immdiatly hit <Enter> without entering anything.
2. When the input doesn't belong to the range you specified.
3. When the user enters an invalid input i.e : a mix of numbers and letters.
4. When the user enters a real number (i.e: 4.5)

The function will keep displaying your custom message and asking for another input untill you enter a valid one.

here is the code :

Code:

---

#include <stdio.h>

long getl(long, long , char *);

int main(void)
{
  long number;

  printf("Please enter a number :\n");
  number = getl(0,500,"Invalid Input.\n\nPlease enter a number :");

  printf("You've entered %d\n\n", number);

  system("PAUSE");

  return 0;
}

long getl(long min_int, long max_int, char *message)
{
  char input[BUFSIZ];
  char *p;
  long result;

  for(;;)
  {

  if( fgets(input, sizeof(input), stdin) == NULL )
      printf("\nCould not read from stream\n");

  result = strtol(input, &p, 10);

      if(result <= max_int && result >= min_int && input[0]!='\n' && ( *p == '\0' || *p == '\n' ))
            return result;
      else
          puts(message);
  }
}

---

Let me know what you guys think about it. And if it has any weak points , feel free to modify or add to it to make it even better.

p.s : i know nothing is perfect , i use perfect as in "as good as possible".

A few things. First of all, your funtion doesn't return anything... Second, uf you manually handled the string to number conversion you could add an overflow check which would make it even better. BTW, correct me if I'm wrong but I don't think scanf was designed for keyboard input in the first place.

This may be of interest: Bullet Proof Integer Input Using strtol()

Quote:

---

First of all, your funtion doesn't return anything...

---

it does , 'return' doesn't neccessarly need to be placed at the end of the function.

Quote:

---

Second, uf you manually handled the string to number conversion you could add an overflow check which would make it even better.

---

I don't clearly get what you mean here , but fgets() limits the input length and the function itself (getl()) wouldn't allow a number out of the range you specify anyway. :)

Quote:

---

BTW, correct me if I'm wrong but I don't think scanf was designed for keyboard input in the first place

---

I'm not sure about what you say, but i doubt it.


and thanks for the link Dave.

Anyone else?

Errr, right. NM about the returning. Regarding the overflow check though, if you just limit the number of charectors you can input then you won't be able to get the full range of numbers unless your funtion uses hex or another compatible base for input, which you are not doing. Speaking of different bases, it would also allow you to set the base of the input which would be handy for inputting octal or hex numbers. And also, the reason why I say scanf probobly wasn't intended for kb input is because I can't possibly see how any programmer in their right mind would think a function that can crash you program depending on a user's input was a good idea.

Quote:

---

Regarding the overflow check though, if you just limit the number of charectors you can input then you won't be able to get the full range of numbers

---

the character limit equals BUFSIZ wich is 512 in my machine. 'long' is mostly 4 bytes wich gives a range from -2147483648 to 2147483647. Any input thats greater than 2147483647 or\and has more than 10 digits would be considred as an invalid input.

how wouldn't i get the full range of numbers? and will you show me how would this overflow in an example?

and regarding the octal and hex inputs , the function has been named 'get long' for a purpose :).

Quote:

---

the reason why I say scanf probobly wasn't intended for kb input is because I can't possibly see how any programmer in their right mind would think a function that can crash you program depending on a user's input was a good idea.

---

Thats not a reason to think it wasn't build for taking inputs from the keyboard. Think 'gets()' :)

Quote:

---

and regarding the octal and hex inputs , the function has been named 'get long' for a purpose :).

---

Adding this functionality would make your method even more complete. Allowing the user to spcify their input preference (0x for hex, o for octal, etc) would allow an even greater range of uses. Just because its signed long doesn't mean it can't be represented in anything other than decimal.

I think it would also be more useful to allow inputs from other streams besides stdin. Maybe that should be a fgetl() function or something though :)

Ok, I actually took a look at what strtol does and I guess your right, it's not a problem. I was assuming that 10 was the the max amount of chars you were allowing when actually it was the base. This pretty much solves the base handling too since all you'd have to do is make it a parameter in your function and pass it through. Actually strtol seems to be an extemely nice function. It handles pretty much every problem that could come up in number entry. And here I thought that all the stdio functions were crap.

Nice suggestions pablo615 and itsme86 , I think fgetl() would be cool. I'll work on it soon.

How about the current getl()? do you think it has any flaws or bad practices?


appreciate your replies..

>How about the current getl()?

I don't know any of the modifications you may have made, but I wonder why you process data that you didn't receive...

Code:

---

  if( fgets(input, sizeof(input), stdin) == NULL )
      printf("\nCould not read from stream\n");

  result = strtol(input, &p, 10);

---

DUH!! how did i miss that :confused:

this will do :

Code:

---

if(fgets(input, sizeof(input), stdin)==NULL)
  {
      printf("\nCould not read from stream\n");
      return 0;
  }

---

i guess its bug-free now :o

Thanks Dave :)

Actually, that change is not bug free either, because 0 is a valid value that a user may enter. You can fix that a variety of ways.

1.) Use exceptions to indicate a failure. I usually try to avoid exceptions because they slow things down sometimes.

2.) Instead of returning a long, return an enum of possible return values, or a bool true false, and take the destination for the input in the argument list as a reference.

Either method above will help make your function even more error proof.

Also, I'd say forget the infinite for loop and allow the user to escape the entering of a value by pressing escape or typeing quit or something like that. That way the user isn't trapped.

Quote:

---

Originally Posted by pablo615

...exceptions...reference...

---

You seem to be lost. Are you looking for the C++ board?

DOH! I didn't even realize...my bad, too used to C++ hahaha

Well, instead of pass by reference, pass a pointer to the long that will contain the input. And forget about the exception thing...

IMO an input function should NEVER EVER output an error message. You should simply return a value that indicates an error and possibly what error and let the programmer decided what to do with it.

Another thing is when checking the range, the min value must be less than or equal to the max value. If these values are switched by accident, you will never get good input since a value cannot be less than 0 and greater than 500 in your example :)

And I agree with Thantos, let the caller of the function output the appropriate error message. An exception in your case, maybe if the range check fails you can print out the valid range as well as the user defined error message, but that could be left to the user to define in their error message as well!

Quote:

---

Originally Posted by Thantos

IMO an input function should NEVER EVER output an error message. You should simply return a value that indicates an error and possibly what error and let the programmer decided what to do with it.

---

you mean like NULL or for example -1? if you mean a long value that indicates the error then lets say its -1 , what if the user entered -1 as input and an error occured , how would he know about it? he'd think that the function returned his value successfly ,right?

Quote:

---

Another thing is when checking the range, the min value must be less than or equal to the max value. If these values are switched by accident, you will never get good input since a value cannot be less than 0 and greater than 500 in your example

---

fixed :

Code:

---

long getl(long min_int, long max_int, char *message)
{
  char input[BUFSIZ];
  char *p;
  long result , temp;

  if(max_int < min_int)
  {
    temp = max_int;
    max_int = min_int;
    min_int = temp;
  }

  for(;;)
  {

  if(fgets(input, sizeof(input), stdin)==NULL)
  {
      printf("\nCould not read from stream\n");
      return 0;
  }

  result = strtol(input, &p, 10);

      if(result <= max_int && result >= min_int && input[0]!='\n' && ( *p == '\0' || *p == '\n' ))
            return result;
      else
          puts(message);
  }
}

---

thanks for notifying me about this. :)

Quote:

---

you mean like NULL or for example -1? if you mean a long value that indicates the error then lets say its -1 , what if the user entered -1 as input and an error occured , how would he know about it? he'd think that the function returned his value successfly ,right?

---

Which is why the conversion functions are not input functions. Also giving an error message to the screen still doesn't tell the programmer that there is an error.

I would change it to:

Code:

---

/*
 * Function returns 0 on error, or positive value on success.
 */
int getl(long *lpinput, long min_int, long max_int, char *message)
{
  char input[BUFSIZ];
  char *p;
  long result , temp;

  if(max_int < min_int)
  {
    temp = max_int;
    max_int = min_int;
    min_int = temp;
  }

  for(;;)
  {

      if(fgets(input, sizeof(input), stdin)==NULL)
      {
        return 0;
      }

      result = strtol(input, &p, 10);

      if(result <= max_int && result >= min_int && input[0]!='\n' && ( *p == '\0' || *p == '\n' ))
      {
            *lpinput = result;
            return 1;
      }
      else
          puts(message);
  }
}

---

>Since 'scanf()' is really ugly dealing with inputs
Debatable. There's really nothing wrong with scanf as long as you know how and when to use it.

>i decided to write a function that will hopefully be perfect when asking for numbers from the user.
>The function is called 'getl()' (get long),
What if I want a float? ;)

I don't really get your post thantos , whats the proper value to be returned on error?

I agree on the message thing though , the programmer should deal with the return value the way it fits him.

Quote:

---

Originally Posted by Prelude

What if I want a float? ;)

---

I'd add an extra argument to specify wether i want a float or an integer then use either 'strtol()' or 'strtod()' within the function depending on that argument. I just wanna discuss the concept of 'getl()' for now ;)


sorry for the late response , i've been away for a while..

>I just wanna discuss the concept of 'getl()' for now
Okie dokie, let's discuss the issues:

1) Returning a long

The biggest problem with returning a long is that the only way to notify the client code of an error is by setting a global flag (henceforth to be known as "The errno effect") or to pass an error flag to the function, thus making it harder to call. If you want to return an error code and valid data in the same type then it takes more planning. For example, strtol returns a valid long, but also sets errno, and makes use of an end pointer. This isn't a bad idea until you try to write bulletproof code for strtol. It's tricky and convoluted.

So perhaps it would be a better idea to separate the error handling and the valid data handling. You can do this a number of ways, but one that seems to work well is to pass a variable to getl and return a status flag:

Code:

---

int getl ( long *val );

---

An alternative is to return a long, but also give the user the option of handling errors by accepting a null pointer in place of the flag:

Code:

---

long getl ( int *err );

---

Or you could go by the "don't worry, be happy" mantra and just let errors go undetected, but I don't recommend that. You could also set errno and use that.

In the end there's no good way around the problem, but for the rest of this discussion I'll assume that you prefer returning a status flag and taking a pointer to long as the argument.

2) Internal buffer

Presently your implementation uses an internal buffer. fgets and strtol are used to parse and convert the line of input into a valid long. This is bad for several reasons. The first reason is that fgets will take a line of input. In other words, if the client program wants to take an age and a name on the same line, tough. Do you really want your library functions to be making formatting decisions for you? I don't.

The second reason is that the buffer could be filled before a complete line is read. As we all know, fgets returns success even if the line is too long for the buffer and the only way to test for it is to look for a newline in the buffer. Ignoring the fact that you don't do that, consider the possible ramifications of what would happen. Well, it really depends on how you react to it. Do you try to read all of the line and brute force your way through the operation? Or do you terminate with an error and boo-hoo, a chunk of the user's input goes to never-never land?

IMO, an input function should only take what it absolutely needs to do the job, and leave everything else for other input functions to deal with. This tends to work best when you have several functions working in conjunction or when the input data is well formatted. We'll get to possible solutions in a moment.

3) Passing maximum and minimum limits as parameters

I don't like this because testing for limits is very often a client task. Sure, getl can do it and it will work nicely, but this goes against the concept of having a function do one operation. How about getl gets a long, and the client tests for other limits as needed? Much like strtol converts a long, but we almost always use it as an int. ;)

The solution:

Okay, so how to implement getl? Well, we've worked out several ways of error handling and chosen one at random. We want to avoid a greedy input algorithm, so fgets is out, and we'll leave boundary checks to the client. Two immediate solutions come to mind: use getchar and fill a buffer, then use strtol as you did before; use scanf because it does all of the dirty work and pretty much handles what you want.

The first option is tricky because you need to consider how big the buffer can be. Do you want to read all digits and let strtol handle overflow on the assumption that this is what will be needed? Do you want to limit the buffer size to the exact number of characters that the largest long value can be? For a decent implementation, the former requires a resizable buffer and thus, another point of failure if allocation requests fail. The latter requires a tricky test to get the size of LONG_MAX. You also need to consider + and - as valid characters and make room for them as well as handle them accordingly.

While either of these are valid options, they imply too much work for too little gain. The other option is to use scanf. Now we have several questions about error handling again. Do we simply return failure if scanf fails and let the client code figure out what went wrong? This is certainly a viable option because feof and ferror are readily available. It also saves us the effort of developing several error codes, and thus we can simply use true and false.

Another question is how to deal with invalid input. Does getl want to remove invalid input and try again in an attempt to recover? Or does it just want to give the client more flexibility. Naturally the latter will open up opportunities for infinite loops where the client is too naive to remove invalid data from the stream before calling getl again. But that isn't nearly as bad as getl taking possibly valid input from the stream. For example, what if the client wanted to read either an age or a name and uses getl's return code to determine which it is? Flexibility is good, and you can't guess the legitimate uses for getl that the client will think up. Better to avoid recovery attempts.

With all of these questions and possible answers in mind, perhaps getl is best implemented like so?

Code:

---

int getl ( long *val )
{
  return scanf ( "%ld", val ) == 1;
}

---

:)

Thanks for the great reply Prelude , i'd like to argue on some points though :

Quote:

---

if the client program wants to take an age and a name on the same line, tough. Do you really want your library functions to be making formatting decisions for you? I don't.

---

Sorry but this wasn't clear for me , maybe an example of that situation would help ..

Quote:

---

As we all know, fgets returns success even if the line is too long for the buffer and the only way to test for it is to look for a newline in the buffer. Ignoring the fact that you don't do that, consider the possible ramifications of what would happen.

---

Since the max input would be 11 digits (including the - and + )then what would the ramifications be? the user wouldn't enter a number larger\smaller than 'long' can handle because the program would store gibberish in the variable.

Quote:

---

For example, what if the client wanted to read either an age or a name and uses getl's return code to determine which it is?

---

I don't this either but you don't really have to explain it lol (what you've done is really enough)

Code:

---

int getl ( long *val )
{
  return scanf ( "%ld", val ) == 1;
}

---

what if the user wanted to enter '444' but he typed 'r444' by mistake, it would return gibberish .. that wouldn't be so nice ,would it?


thanks so much for the reply :)

and sorry for the late respone .. i was busy with college

Quote:

---

Code:

---

int getl ( long *val )
{
  return scanf ( "%ld", val ) == 1;
}

---

what if the user wanted to enter '444' but he typed 'r444' by mistake, it would return gibberish .. that wouldn't be so nice ,would it?

---

But the programmer would know that there is gibberish because of scanf()'s return value wouldn't be 1 thus you would get a return value from getl() of 0.

But it would return 1 for '555a5' and treat it as '555' :)

Quote:

---

Originally Posted by Brain Cell

But it would return 1 for '555a5' and treat it as '555' :)

---

strtol, which is used in the "perfect version", has the same side effect:

Code:

---

#include <stdio.h>
#include <stdlib.h>
int main( void )
{
        char buf[BUFSIZ];
        long int l;
        printf("Number: ");
        fflush( stdout );
        fgets( buf, BUFSIZ, stdin );
        l = strtol( buf, NULL, 10L );
        printf("Output: %ld\n", l );

        return 0;
}

/*
~/programming/cboard$ ./stl
Number: 555d5
Output: 555
*/

---

Quzah.

well thats because you didn't take advantage of the second argument and used NULL instead. This works strictly :

Code:

---

#include <stdio.h>
#include <stdlib.h>


int main(void)
{
  char buff[BUFSIZ], *p;
  long num;

  fgets(buff, sizeof(buff), stdin);

  num = strtol(buff, &p, 10);

      if( buff[0]!='\n' && ( *p=='\0' || *p=='\n'))
          printf("%d\n", num);
      else
          printf("Invalid input\n");

  return 0;
}

---

But Prelude is saying its not good within another function. How about if i keep repeating that in 'main' for every input? will it be better?

I think most of the points i'd like to make have already been made those being scanf when used how intended work fine. And "what if I want a double"
but more importantly: you forgot to make it platform independant.

Code:

---

system("PAUSE");

---

:)
but you knew that already didn't you?
By the way, what's wrong with just accepting the input as a string and parsing it accordingly? I see nothing wrong with that, unless this exercise is one of intellectual curiousity.

>Sorry but this wasn't clear for me , maybe an example of that situation would help ..
Write a program using your getl that allows the user to type five numbers on a line, and then print out each of those five numbers. As it is you would read one number, and if that worked (which it won't thanks to your error checking...so ironic) then you would discard the others and be forced to ask again to get subsequent numbers.

>Since the max input would be 11 digits (including the - and + )then what would the ramifications be?
What about the rest of the input? What if the maximum value of a long is more than 11 digits?

>I don't this either but you don't really have to explain it lol
Sometimes it's useful to attempt to read a value and then attempt to read a value of another type if that fails. For example:

Code:

---

#include <stdio.h>
#include <stdlib.h>

int main ( void )
{
  char name[BUFSIZ];
  int age;

  printf ( "Enter your name or age: " );
  fflush ( stdout );

  if ( scanf ( "%d", &age ) == 1 )
    printf ( "You're %d years old?\n", age );
  else {
    if ( fgets ( name, sizeof name, stdin ) != NULL )
      printf ( "Nice to meet you, %s", name );
    else
      fprintf ( stderr, "Error processing input\n" );
  }
}

---

>it would return gibberish .. that wouldn't be so nice ,would it?
It isn't the job of the input function to validate data. A generic input function gets data of a certain type and hands it over to the client code for processing. If an input error occurs or data of the wrong type is encountered, that's a valid problem and only the client code can know how to react.

>But it would return 1 for '555a5' and treat it as '555'
How do you know that this isn't the correct behavior? Once again, input functions know nothing about the application's expected data formatting, and thus can't assume anything.

>But Prelude is saying its not good within another function.
It depends on the function. But with your function, it causes more problems than it solves.

>How about if i keep repeating that in 'main' for every input? will it be better?
If you find yourself repeating it then write a wrapper for the application. If you put it in getl itself then the chances for reuse are cut drastically because the function is not as flexible.

Quote:

---

Write a program using your getl that allows the user to type five numbers on a line, and then print out each of those five numbers. As it is you would read one number

---

What if it was ment to get one number each time from the user , will it be ok or still have other significant problems?

and thanks so much for the help :)