Re: fgets() - supposed to be simple :)

From: Peter (shaggy_at_australis.net.STOP.SPAM)
Date: 11/25/03

Next message: Martijn Lievaart: "Re: about BigEndian and LittleEndian"
Previous message: Peter : "Re: test"
In reply to: Rob Somers: "fgets() - supposed to be simple :)"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Date: Tue, 25 Nov 2003 10:08:07 GMT

Groovy hepcat Rob Somers was jivin' on 23 Nov 2003 14:08:26 -0800 in
comp.lang.c.
fgets() - supposed to be simple :)'s a cool scene! Dig it!

>I am writing a program to keep track of expenses and so on - it is not
>a school project, I am learning C as a hobby - At any rate, I am new
>to structs and reading and writing to files, two aspects which I want
>to incorporate into my program eventually. That aside, my most
>pressing problem right now is how to get rid of the newline in the
>input when I use fgets(). Now I have looked around on the net, not so

You apparently didn't look very hard, then.

>much in this group as on another C board, and I have found several

Why didn't you read this newsgroup first? It is very rude to post to
a newsgroup without first reading at least a month worth of articles
therein. This has been standard practice since the early days of the
Internet, and has been ignored by the scores of newbies who have
posted here since the Net became mainstream.
Why didn't you read this newsgroup's FAQ either? It is extremely
rude to post to a newsgroup without first reading its FAQ list. This
has also been standard practice for a long time, but now, sadly,
ignored by many. Find the FAQ at this URL:

http://www.eskimo.com/~scs/C-faq/top.html

>good solutions for getting rid of the newline character, however I
>cannot seem to make it work for me, and now my program is becoming
>more unmanageable as I go along, trying to fix the problem. I will
>post the code, and I will comment in the part that I am specifically
>not understanding.

  OMG! You've posted your entire program! That is a big no no. You
should have cut it down to the smallest complete program that
demonstrates the problem. By "the smallest complete program" I mean
something that (if corrected) will compile and run, but only does
enough to show what the problem is. We don't want to have to wade
through an entire program just to see what's wrong with one small part
of it. You should do the work (of cutting it down), not us. After all,
you're the one who wants our help.
  However, there are many problems throughout your code that I have
addressed below.
  Another thing: you really need to work on your code formatting. Your
code is rather ugly visually, due to a total lack of consistency in
indenting and use of white space. This makes your code harder to read
and understand. Please adopt a legible white space and indentation
style, and stick to it. (Use spaces, not tabs.) Where several lines
perform a simple operation together, group these lines together and
surround them with white space (newlines). If it is not immediately
obvious what these groups of lines do, then include a brief, concise
comment that describes, in plain language, what the intention is. This
will greatly aid in understanding the logic of your program.

>#include <stdio.h>
>#include <stdlib.h>
>#include <string.h>
>#include <ctype.h>
>
>#define CLEAR printf("\033[2J\033[0;0f");

Poor use of a macro. You're using an object-like macro to call a
function, which is counter intuitive. You're also ending the
replacement text with a semicolon, which means you don't need to (and,
in some cases, shouldn't) use the macro with a semicolon. It's not
even a portable way of clearing the screen (if that's what it's meant
to do). In fact, there is no portable way to do so. There are C
implementations that run on systems that don't even have screens.
But, as you said, you're not overly concerned about portability.
However, we are. You should have cut out everything non-portable
before posting.

>#define MAX 100
>
>/* structure for writing to and reading from file */
>
>struct money{
> char category[MAX];
> char month[MAX];
> int day[MAX];

You need an array of 100 ints to store a day?

> char purchase_description[MAX];
> float purchase[MAX];

  And you need an array of 100 floats for the amount of the purchase?
  BTW, double is usually a better choice, where a floating type is
needed. It is the "natural" floating type in C, and typically has
higher precision than float.
  But actually, the best type for the purchase price is not a floating
type at all. An integer type (representing, for example, the number of
cents instead of dollars) would be better. Due to the way floating
types are represented, they are not accurate enough for something like
this.

>};
>
>void menu( void );
>int get_num( void );
>int validate( char *a );
>void add_record( void );
>int main( void )
>{
> menu();
> return 0;
>}

Poor design. main() becomes nothing more than a wrapper, which makes
it rather useless and just adds to the function call overhead (not
that that matters much in a program of this nature).

>/* menu() - display a menu and return the choice of the user */
>void menu( void )

You should probably do some kind of error testing, and return an
error indicator to main(). main() would then handle the error in some
way. Then main() would be more than just a wrapper. It would have some
purpose after all.

>{
> char choice;

Why char?

> printf( "\t\t\tMoney Log \n\n\n" );
> printf( "\t1. Update your record\n\n" );
> printf( "\tPlease enter your selection: " );

You need to flush the output here to make sure the prompt is
displayed before the program waits for input.

fflush(stdout);

> choice = get_num();
> switch( choice ){
> case 1:
> CLEAR;
^
Ande here you've forgotten that you've ended this macro's
replacement text with a semicolon (see above), so you have an extra
semicolon here. IOW, this line expands to this:

printf("\033[2J\033[0;0f");;
^
Note the extra semicolon. This is not an error (in this case), but is
not really wanted.

> add_record();

add_record() should probably return an error indicator, which could
perhaps be returned to main(), or could be intercepted here.

> break;
> default:
> break;
> }
>}
>
>/* add_record - Read from keyboard and add the record in the list */
>void add_record( void )
>{
> char buffer[MAX];
> char *p = NULL;
>
> struct money money_log;
>
>/* This next bit is supposed to ask the user for each piece of
>information,and store the input in the struct - It is where my problem
>lies I think */
>
> printf ( "Enter category of purchase: " );

Again you need to flush the output.

fflush(stdout);

> fgets( buffer, MAX, stdin );
> if( ( p = strchr( buffer, '\n' ) != NULL )
^
You have a missing closing parenthesis here. That is the likely
casue of your problem.

> *p = '\0';
> strcpy( buffer, money_log.category );

Your arguments are the wrong way around.

> fflush( stdout );

Why on Earth are you flushing stdout here? You haven't written
anithing to stdout here. A few lines above, yes; the line below, yes;
but not here.

> printf( "Enter month (eg, Oct.): " );

fflush(stdout);

> fgets( buffer, MAX, stdin );
> if( ( p = strchr( buffer, '\n' ) != NULL )
^
And once again you have a missing parenthesis.

> *p = '\0';
> strcpy( buffer, money_log.month );

Wrong way around.

> fflush( stdout );

And again, what is the purpose of flushing stdout here?

> printf( "Enter day: " );

fflush(stdout);

> fgets( buffer, MAX, stdin );
> money_log.day = atoi( buffer );

You can't do that. money_log.day is an array, and you can't assign
to an array.
You should also be checking for valid input here, rather than just
taking for granted that the user will enter the kind of data expected.

> if( ( p = strchr( buffer, '\n' ) != NULL )
^
At least you're consistent in your errors, if not your indentation
style.

> *p = '\0';
> fflush( stdout );

You need to flush after writing but before reading, not after
everything.

>/* I have been experimenting here (above) with different things, and I
>get a pile of warnings on my compiler which I will post also. */
>
> printf ("Enter a description of the purchase (eg, phone bill):
>");

You need to be careful of line wrapping when posting code here. In
theory, we should be able to copy & paste your code straight from your
post, and compile it unaltered. In practice, however, there is usually
some editing to be done to rejoin wrapped lines. It is your job, as
the one asking for help, to ensure that we don't have to do this.

fflush(stdout);

> fgets(money_log.purchase_description, MAX, stdin);
>
> printf ("Enter the amount of the purchase : ");

fflush(stdout);

> fgets(buffer, MAX, stdin);
> money_log.purchase = atoi( buffer );

There are two errors here. First, money_log.purchase is an array,
and you cannot assign to an array. Second, it's an array of float, and
you're assigning it an int value. atoi() returns an int, oddly enough.

> CLEAR;

See above.

> printf( "Category\tDate\tPurchase\tAmount\n\n" );
> printf( "%s %s%d %s %.2f", money_log.category,
>money_log.month, money_log.day, money_log.purchase_description,
>money_log.purchase );

And again you're forgetting that money_log.day and
money_log.purchase are arrays.

>}
>
>/* get_num() - get a number from the user, and check it for validity
>*/
>int get_num( void )
>{
> int choice;
> char buffer[BUFSIZ];
>
> if( fgets ( buffer, sizeof buffer, stdin ) != NULL ){
> buffer[strlen ( buffer ) - 1] = '\0';

You need to be careful here, as a whole line may ne be read in
(because the line entered is longer than the buffer), in which case
there is no neline in the buffer at this point. So, if you remove
buffer[strlen(buffer) - 1], you may be removing part of the entered
text. In this case it probably doesn't matter, but in a real-world
program it can matter a great deal. That's why we usually use strchr()
to find the newline (if there is one).

> if( validate( buffer ) == 0 ){
> choice = atoi( buffer );
> }
> else
> printf( "\nPlease enter a numerical character." );
> }
> else
> printf( "Error reading input\n" );
>
> return choice;

Note that choice has not been initialised, and still contains
garbage, if the fgets() call failed.

>}
>
>/* validate() - connected to get_num() */
>int validate( char *a )
>{
> unsigned x;
> for ( x = 0; x < strlen ( a ); x++ )
> if( !isdigit ( a[x] ) ) return 1;
>
> return 0;
>}
>
>Here are warnings gcc gives me when I try to compile:
>
>another.c: In function `add_record':
>another.c:62: warning: assignment makes pointer from integer without a
>cast
>another.c:63: invalid operands to binary *