Richard said:
It's much better than average - in fact, it's pretty good, and you've
obviously put some thought into it. But I do have a couple of
observations.
Thank You Sir!
..snip...
If fgets yields NULL, it may be because the end of input as been reached.
(This can happen interactively if the user knows how, and will certainly
happen at some point if the input is redirected from a file.)
Ok ,but i did not think about the file part.Ive never used input which
was redirected from a file.So some work lies ahead!
It's probably a good idea to check whether flushln returned EOF (see above
comment re fgets).
Will take care of that the next time
..snip...
I think you need to distinguish between your exception conditions. You
clearly consider a too-long input to be erroneous. That's one. An error on
the stream is also possible (ferror(stdin) will yield non-zero if that
happens). And of course you could hit the end of the file (in which case
feof(stdin) will yield non-zero). These are three very different
conditions. The first is easily recoverable - you can ask the user to
provide shorter input and not to be such a berk.
The second, however,
is more problematical, and there is no standard way to recover from such
an error. The third probably just means that your program's work is
complete. Clearly, these are all very different, and should be treated
differently, but at present they are all covered by a NULL return.
Also, remember that you're relying on a static buffer within input() -
which is fine, it's not as if that memory is going to vanish... but it
does mean that you can only point to one input at once. If you need to
store information, you'll need to reserve some storage into which to copy
it.
Ok.Can you please point out books/sections of K & R2/internet resources
where i can read about these errors.I've not read K & R 2 from cover to
cover or for that matter the C Standard.
Please do point out specific sections
(I do have the c99 draft though)
With due credits to Mr.Jack Klein
(
http://home.att.net/~jackklein/c/code/strtol.html)
Ive come up with this code to accept valid integers
Modifying it to accept long ,long long and unsigned is pretty simple
It does look like there are a lot of overheads involved for an input,But
this this the best I've come during the one and a half year of my 'C'
experience
!
I have not implemented all the changes suggested as yet.
1.The memset part is certainly a big overhead and as suggested ,i will
change it
2.Did not check the return value of flushln.Will work on it as well
#include <stdio.h >
#include <stdlib.h>
#include <string.h>
#include <errno.h >
#include <limits.h>
#define BUFFSIZE (80 +2)
#define MAXTRY 5
int flushln(FILE *f) {
int ch;
while (('\n' != (ch = getc( f ))) && (EOF != ch))
continue;
return ch;
}
char *input(char *message)
{
static char buffer[ BUFFSIZE ];
This function "input" is not labelled static, yet you use and return a
static declared from within it. This is usually a sign that the code
you are writing is not reusable.
char *p = &buffer[ BUFFSIZE-1 ];
int flag = 0;
puts(message);
memset(buffer,-1,BUFFSIZE);
So you are using -1 as a sentinel character (you should comment these
things.)
if( fgets(buffer,BUFFSIZE,stdin) == NULL){
puts("Input Error");
return NULL;
}
else{
if((*p == -1) || ( (*p == '\0') && (*--p == '\n') ))
flag = 1;
Well, ok, so if the very end of your buffer has a sentinel, or if your
buffer exactly ends with "\n\0" then set this flag. What happens if
the use inputs character -1 (under Windows, while holding the ALT key,
the user presses 2 5 5 on the numeric keypad) as the last character in
the buffer then proceeds to keep inputing text? While you won't
overflow, you will not have a '\0' anywhere in your buffer either;
which means your going to overflow in typical C string functions in
all the code above this point. You need to scan for the '\0' in the
entire buffer to be sure.
As a rule of thumb, remember that security of the C language string
functions is always more costly that you think at first, (and more
costly that it needed to be). In this case you have to do an extra
scan of the buffer to make sure its terminated properly no matter
what. This isn't the cost of security -- this is the cost of the C
language.
You are also learning the cost of re-inventing the wheel. For a more
straight forward approach to user input see:
http://www.pobox.com/~qed/userInput.html
Using getInputFrag() as your primitive you can set policy with respect
to overflowing input as you appear to be trying to do in your code,
without ever having the need for doing a second pass scan.
You can remove the "flag = 1; if (!flag)" and just put "else" its
place (or negate the condition in the if statement.)
flushln(stdin);
puts("Dont try it honey !\n\n");
return NULL;
}
}
return buffer;
}
int getInt(void)
{
char *buff;
char *end_ptr;
long int lVal;
int trial = MAXTRY;
errno = 0;
while(trial != 0)
{
while( (buff = input("Enter Integer :")) == NULL )
continue;
lVal= strtol(buff, &end_ptr, 0);
if (ERANGE == errno){
puts("Number Is Out of Range\n\n");
trial--;
errno = 0;
memset(buff,-1,BUFFSIZE);
}
else if (lVal > INT_MAX){
Some compilers will issue a warning telling you that this is not
possible. (Because long int and int can be the same.)
printf("%ld Too Large!\n\n", lVal);
trial --;
errno = 0;
memset(buff,-1,BUFFSIZE);
The above is redundant.
Its also poor programming practice. The buffer that is returned
happens to be declared with a size BUFFSIZE, but that's not made
explicit by the input() function itself. Each part of your program
should be responsible for the things it uses/touches, and where it
needs to push that responsibility outward there has to be a way for
those functions themselves to inform the other functions about what
responsibilities are being passed along.
There are two ways to solve this. One is for the input function to
also return with the length of the input buffer (remember you can
supply pointer parameters on input to receive more than one output in
C.) The other is to have the buffer declared by the *calling*
function (like fgets requires you to do) and have the length passed
into it. In either case, then the length of the buffer will become
known and the responsibility of the calling function -- in this case
getInt(), not main(). Then getInt() can decide what other extra
things it wants to do with the buffer it knows the size of.
The fundamental problem is that this code you've written can't be
copied out and placed into other code, unless the consumer is aware of
the BUFFSIZE macro, the management of your buffer and the use of -1 as
a non-valid sentinel. All of these details are irrelevant to the
processing or parsing of integers especially in your highest level
code (main.) main() should just be viewing the task as "I need to get
an integer, and if I can't get one, then I should do something about
it", and not care about the details of *HOW* the integer was
obtained. That's what the functions getInt() and input() are supposed
to handle.
This is the idea of encapsulation. If you are able to separate these
details, then you can replace any of main() or getInt() or input()
with different code without having to maintain non-obvious
restrictions imposed by some statically defined buffer or arbitrarily
chosen BUFFSIZE. For example, in the future you may wish to
dynamically allocate memory for the input string. With the current
code, you would have to touch all three functions to make that work
properly.
}
else if (lVal < INT_MIN){
printf("%ld Too Small!\n\n", lVal);
trial --;
errno = 0;
memset(buff,-1,BUFFSIZE);
}
else if (end_ptr == buff){
printf("Not a Valid Integer\n\n");
trial --;
errno = 0;
memset(buff,-1,BUFFSIZE);
}
else
return (int)lVal;
}
return '\0';
Why not just "return 0;" ?
}
int main(void)
{
int temp;
if((temp = getInt( )) != '\0')
printf("%d\n",temp);
else
{
puts("This is deliberate dear! ");
EXIT_FAILURE;
}
EXIT_SUCCESS;
}
Are there any more errors/areas where i need to work upon.Of course i
have mentioned I've still left a couple of them behind as of now!
Your design appears to be to try to force a good integer input from
the user. But if things don't work out, just use '\0' (== 0) instead,
and lose the fact that an error has occurred. Personally I would
redesign this to make a function like:
int getInt (int * outInteger, FILE * fp);
where outInteger is filled in with the value of the integer, the file
stream used is passed in, and any sort of error code you care to
declare is returned from the function. In this way that calling
function can set policy on when it gives up on trying to receive
input, by escalating the error message, using a default value, or
exiting the program. The point is that you can change this policy
without affecting the task of how you input, or how you parse numbers.
