On 27 Dec 2003 07:05:12 -0800,
()
wrote:
>I wrote that example from a book and there is en error in the display
Assuming you copied it correctly, the book has several errors. Any
chance it was written by Schildt?
>module that it does not showing all the records are entered in the
>input module.
>I traced with some printf statments without getting the solution . I
>think the error in the display module loop condition.
>Thanks
>
>
>
>#include <stdio.h>
>//-exercise 7.5.1 - Goal Statistics
>struct football
>{
>char name[30];
>char team[30];
>int goals;
>};
>main ()
While older compilers still accept this, the new language standard has
done away with implied return types for functions. Get in the habit
of doing it right:
int main(void)
>{
>struct football player[100];
>int option;
>//initialize player array
Interesting that your book uses a comment style allowed only in C99
yet uses other features (such as above) that are disallowed in C99.
>init_player(player);
>do
>{
>
>//Display menu of options ;
>menu();
>
>
>//Determine users requirements
>menu_choice(&option);
>
>//Perform users specified option
>switch(option)
> {
> case 1: //Enter information for all players
> player_input(player);
> break;
> case 2: //display table for all players
> goal_table(player);
> break;
> case 3: //update specific information for a player
> goal_update(player);
> break;
> case 4: //exit from program
> break;
> default://illegal input
> printf ("\n\n This is not an available option\nAvailable options are
>1,2,3,4");
Any time your interactive output does not end with a '\n', you run the
risk of it not being displayed to the user due to buffering
considerations.
> }
>} while (option !=4);
Not an error but most recommend an explicit return from main().
>}
>
>init_player(player)
>//------------------
>struct football player[];
Since this is not main and since you don't return a value, the implied
return type of int here is not acceptable. You must specify void.
While still legal, this construction is very obsolete. (When was the
book written?) The "modern" construction combines the two lines to
void init_player(struct football player[])
This has the additional benefit of allowing you to place function
prototypes in scope before you call the function. I believe this is a
requirement in the new language standard but is a good idea even if
not since it allows the compiler to check that your arguments to the
function have the correct type.
>{
>int i,j;
>for (i=0;i<100;++i)
>{
> for (j=0;j<30;++j)
> {
> player[i].name[j]=' ';
> player[i].team[j]=' ';
> }
You do realize that this serves no purpose?
> player[i].goals=0;
Neither does this.
>}
>}
>
>menu()
>//------------------
>{
>system ("clear");
system is declared in stdlib.h which you did not #include.
>printf("\n\n football system ");
>printf("\n ------------------- ");
>printf("\n\n 1- Enter information ");
>printf("\n\n 2- Display table for all players");
>printf("\n\n 3- Update specific information");
>printf("\n\n 4- Exit from program ");
>}
>
>
>menu_choice(opt)
>//------------------
>int *opt;
>{
>printf("\n\n Select one of the above (1-4) : ");
>scanf("%d",opt);
>
>//clear screen
>system("clear");
Why on earth would you want to clear the screen right after accepting
user input?
>}
>
>
>player_input(player)
>//------------------
>struct football player[];
>{
>int i;
>char more;
>i=-1;
>do
> {
> ++i;
>
> //input players name
> printf("Enter players name : ");
> input_string(player[i].name);
>
>
> //input players team
> printf("Enter players team : ");
> input_string(player[i].team);
>
>
> //input number of goals scored
> fflush(stdin);
This invokes undefined behavior. fflush is defined for output streams
only and has never been defined for input streams.
> printf("Enter number of goals scored: ");
> scanf("%d",&player[i].goals);
>
> printf("\n %-30s %-30s %4d\n",player[i].name,player[i].team,player[i].goals);
> printf("\n %d I Value inside insert\n ",i);
> //more players
> if (i<99)
> {
> printf("More playersd to be entered (y/n)---->");
> scanf("%s",&more);
This is a major problem. more is a single char. %s will accept a
string which is guaranteed to be longer than one char. This invokes
undefined behavior and overwrites whatever is in memory following
more. One possible solution is to use %c.
> }
> } while (more == 'y' && i<99);
What if the user types 'Y'?
>
>//Terminate players list
>if (i<99) player[i+1].goals=-1;
>}
>
>
>input_string(alpha)
>//------------------
>char *alpha;
>{
>int i;
>i=-1;
>
>//Flush the keyboard buffer
>fflush(stdin);
>do
>{
> ++i;
>
> //input a character
> alpha[i]=getchar();
>} while (alpha[i] !='\n' && i<29);
When i is 28, the while is true and you loop one final time to accept
alpha[29] ...
>
>//Terminate string
>alpha[i]='\0';
and you then overlay alpha[29] with a nul. It is bad form to destroy
a user's input without telling him. If you coded 28 in the while, he
would at least know that you didn't let him enter the last character.
>}
>
>goal_table(player) //Display table of goals scored
>//------------------
>struct football player[];
>{
>int i=0;
>char cont;
>
>//Output table headings
>printf("\n\n Name Team Goals");
>printf("\n\n ---- ---- -----");
These titles and underscores are not wide enough for the 30 character
entries that follow.
>
> do
> {
> //Output player information
> printf("\n %d I Value inside display ",i);
> printf("\n %-30s %-30s %4d\n",player[i].name,player[i].team,player[i].goals);
> ++i;
> } while (i<=2);
This is the source of the error you asked about. Did you mean 99
here? 2 makes no sense at all. This looks like a transcription
error.
Even with 99, there is a logic error. In init_player(), you
initialize name and team to blanks with no terminating '\0'. In
player_input(), you allow for the possibility of less than 100
players. This loop does not test for this and will attempt to print
names and teams that are not strings with the %s format. This invokes
undefined behavior.
> printf("\n\n Press C to continue ");
> scanf("%s",&cont);
Another attempt to read a string into a single char. Is this really
what the book says?
>}
>
>goal_update(player) //update table of goals scored
>//------------------
>struct football player[];
>{
>char name[30];
>int i,match,goal;
>
>//input players name to be updated
>printf("Enter name of player");
>input_string(name);
>
>//Find players record
>i=0;
>while ((player[i].goals!=-1)&&(i<100)&&(match=strcmp(name,player[i].name)
>!=0))
Even though it produces the correct result in most cases, this is a
poor construct. To see why, let's reformat it so the news reader does
not break up the line. The only changes I am making are removing the
/n> that indicates quoted material, inserting a space before each &&,
and inserting a \n after each &&.
while ((player[i].goals!=-1) &&
(i<100) &&
(match=strcmp(name,player[i].name)!=0))
The first problem is the second test needs to be first. You cannot
check player[i].goals if i is 100 or more. That variable does not
exist and attempting to do so invokes undefined behavior. The &&
operator has a short-circuit evaluation so if you rearrange the tests
and the first one fails, the remaining are never evaluated.
Additionally, last expression is "broken." Here it is again with some
addition (and suggestive) white space.
(match = strcmp(name,player[i].name) != 0)
Since != has higher precedence than =, this expression is evaluated as
( match = (strcmp(name,player[i].name) != 0) )
which means "assign to match the value 1 or 0 depending on whether the
return value from strcmp is different from or the same as 0,
respectively."
The recommended construction is
( (match = strcmp(name,player[i].name)) != 0 )
which means "assign the return value from strcmp to match and then
determine if it is different from or the same as 0."
The reason yours works in this case is because you are only interested
in equality or inequality and match will be 0 if and only if strcmp
returns 0. However, for example, if your array were sorted, you would
be interested in two of the three possible return values from strcmp.
Your construction would not give you that. Both negative and positive
returns from strcmp would be combined into a single value for match.
>++i;
>
>//Input number of goals to be added
>printf("Enter number of goals to be added to players account");
>scanf("%d",&goal);
Don't you think this would be better inside the following if? Why ask
for input if you know you cannot use it?
>
>//Update players record
>if (match==0)
> player[i].goals=player[i].goals+goal;
>else
> printf("\n\n Player %s is not in the goal table \n",name);
>}
If you give us the name of the book and author and date, we can add it
to the list of books not to use.
<<Remove the del for email>>
Similar Threads
