I am building a small game with a soldier system where the player in certain events, gets to say a number of soldiers to be sent in battle. I have made 2 functions so that numbers above the number of available soldiers (set to 450 at start, may change within the game). Yet, on my tests when entering a bigger number nothing happens. I believe that the recursion on the second function is problematic, and I have implemented something wrong. Here is the code:
int wrong_soldiers_check(int soldiers)
{
int de_facto_soldiers = give_soldiers();
int ret_value = 0;
if(de_facto_soldiers < soldiers){
print_slowly("Sir! We don't have so many soldiers! Let me ask you again...");
ret_value = 1;
}
return ret_value;
}
int generic_attack()
{
int soldiers;
print_slowly("\nAlright! How many men should we send?\n");
print_soldiers();
print_slowly("You: Let's send ");
scanf("%d", &soldiers);
int check;
check = wrong_soldiers_check(soldiers);
if(check == 1){
generic_attack();
}
return soldiers;
}
What have I done wrong?
Edit:
This is the code for give_soldiers
int give_soldiers()
{
soldiersp = fopen("soldiers.txt", "r");
char soldiers[3];
fgets(soldiers, 4, soldiersp);
fclose(soldiersp);
int soldiers_num = atoi(soldiers);
return soldiers_num;
}
CodePudding user response：
Ah-ha!
int give_soldiers()
{
soldiersp = fopen("soldiers.txt", "r"); // may not succeed in opening
char soldiers[3]; // dimensioned for 3 characters
fgets(soldiers, 4, soldiersp); // reading in up to 4 bytes (overflow!)
fclose(soldiersp);
int soldiers_num = atoi(soldiers); // should be null-terminated string, at least
return soldiers_num;
}
Slightly improved...
int give_soldiers() {
char *fname = "soldiers.txt"; // make name prominent
FILE *fp = fopen( fname, "r" ); // local, so use local variable
if( fp == NULL ) {
fprintf( stderr, "Cannot open '%s'... Exiting\n", fname );
exit( EXIT_FAILURE );
}
char str[ 32 ]; // Be generous
str[0] = '\0'; // Is a 'string' even if source file is zero length
fgets( str, sizeof str, fp ); // Should test for NULL return, but being optimistic and concise.
fclose( fp );
return atoi( str ); // fgets() insures null terminated string
}
CodePudding user response：
The natural way to write your generic_attack() function would be to use a loop. For example:
int generic_attack() {
int soldiers;
do {
print_slowly("\nAlright! How many men should we send?\n");
print_soldiers();
print_slowly("You: Let's send ");
scanf("%d", &soldiers);
} while (wrong_soldiers_check(soldiers) == 1);
return soldiers;
}
It would be hard to make the function much clearer than that, and "clear" is among the top measures of code quality.
As for the actual question of what is wrong with the original code, it is as described in comments: that version always returns the value read by the scanf() in the initial execution, regardless of whether it recurses or what any of the recursive executions of the function do. This is in part because every execution of a function gets its own local variables (except static ones), even recursive executions.
In particular, the soldiers variable into which a recursive execution of your original function reads input is a different variable (with a different storage location if you tested that) than the same named-variable of the calling execution of the same function. The caller ignores the return value of the recursive call. Another way to fix the function, then, would be to stop ignoring the return value of the recursive call:
int generic_attack() {
{
int soldiers;
print_slowly("\nAlright! How many men should we send?\n");
print_soldiers();
print_slowly("You: Let's send ");
scanf("%d", &soldiers);
if (wrong_soldiers_check(soldiers) == 1) {
// NOTE: the value returned by the recursive call is returned by this function
return generic_attack();
} else {
return soldiers;
}
}
But don't actually do that. Recursion is the wrong tool for most jobs, including this one.
Now that you have posted the code for give_soldiers(), it is clear that you have another, separate problem as well. As @Fe2O3 observed, that function has a potential buffer overflow. It's not clear to me why you're reading the number of available soldiers from a file every time -- that seems awfully wasteful -- but you're not gaining anything by manually buffering it. This would be simpler:
#define SOLDIERS_FILE "soldiers.txt"
// ...
int give_soldiers() {
// Note: no need for a global variable here:
FILE *soldiersp = fopen(SOLDIERS_FILE, "r");
// check for NULL ...
int soldiers_num;
int num_fields = fscanf(soldiersp, "%d", &soldiers_num);
fclose(soldiersp);
// handle error if num_fields != 1 ...
return soldiers_num;
}
CodePudding user response：
"What have I done wrong?" You've tried to use recursion to solve a problem without understanding the consequences.
If you want to 'restart' the same function, you can use goto and a label
int generic_attack() {
int soldiers;
top:
print_slowly("\nAlright! How many men should we send?\n");
print_soldiers();
print_slowly("You: Let's send ");
scanf("%d", &soldiers);
int check;
check = wrong_soldiers_check(soldiers);
if(check == 1){
goto top;
}
return soldiers;
}
Or, slightly more brief with a small rearrangement:
int generic_attack() {
top:
print_slowly("\nAlright! How many men should we send?\n");
print_soldiers();
print_slowly("You: Let's send ");
int soldiers;
scanf("%d", &soldiers);
if( wrong_soldiers_check( soldiers ) )
goto top;
return soldiers;
}
Since it's a game, you want the program's responses to be less like a program's logic. I'm just having fun here... Another possibility using 'goto'
int generic_attack() {
int soldiers;
print_slowly("\nAlright! How many men should we send?\n");
print_soldiers();
top:
print_slowly("You: Let's send ");
scanf("%d", &soldiers);
// int wrong_soldiers_check(int soldiers) code spliced in here.
int available = give_soldiers();
if( soldiers > available ) {
print_slowly("Sir! We don't have so many soldiers! Enter again...");
goto top;
}
if( soldiers == available ) {
print_slowly("Sir! There'll be no-one left to drive you! Enter again...");
goto top;
}
return soldiers;
}
Other people have suggested using one of C's looping operations. If you did, the final example's goto could be replaced with continue;
Many ways to skin a cat!
You can see where this snippet could also be used in the function. (Since no-one would suspect the user might enter "-42" when asked...)
top:
print_slowly("You: Let's send ");
scanf("%d", &soldiers);
if( soldiers < 1) {
print_slowly("Ah, you mean let the enemy become complacent!\nInteresting...\n");
return 0;
}
// int wrong_soldiers_check(int soldiers) code spliced in here.
int available = give_soldiers();