I’m writing a factorial function for practice. On main I declare the variables and call it like this:
int main(){
unsigned long n = 0;
scanf("%lu", &n);
unsigned long result = factorial(n);
printf("%lu \n", result);
return 0;
}
Which works and gives the result I want. But I thought it looked kinda bad so I refactored it like this:
int main(){
unsigned long n = 0;
printf("%lu \n", factorial(scanf("%lu", &n)));
}
Which somehow always gives 1 as a result no matter what. Why? Here’s the factorial function just in case:
unsigned long factorial(unsigned long n){
if(n == 1){
return n;
}
else if(n <= 0){
printf("No factorial for negative numbers");
}
else{
return (n * factorial(n - 1));
}
return 0;
}
I did this same exercise on Python and formatted the function call in the same way and it worked, so I’m confused as to why C doesn’t accept it. Does it not call functions withing functions from the innermost first (scanf, then factorial, then printf in my case)?
>Solution :
-
Missing include.
-
As @RetiredNinja said the issue is that
scanf()returns the number of arguments parsed not the value ofn. I added error checking here to clarify this further. -
factorial():nis unsigned son <= 0is better written asn == 0or just!n. -
factorial(): The placement of thereturn 0;statement is odd as it only executes for the casen <= 0. -
factorial():return nis not wrong but as it’s a base case it’s clearer to usereturn 1. -
(Not fixed)
factorial(): Prefer an iterative implementation as C doesn’t guarantee tail call optimization (hence with sufficiently large input value your stack may overflow). As @Fe2O3 points out in this case you will overflow long before stack overflow, though. -
(Not fixed)
factorial()on my system returns 0 forn > 65due to overflow. Maybe add an upper limit check? -
Prefer the signature
main(void)tomain(). In practice they will do the same but only the former is defined in the standard (along withmain(int, char *[])). -
(Not fixed) You may only want to call
factorial()ifn > 0, or store the value in a variable like your initial implementation so you can omit the output forn == 0instead of:0 No factorial for zero 0
#include <stdio.h>
unsigned long factorial(unsigned long n) {
if(!n) {
printf("No factorial for zero\n");
return 0;
}
if(n == 1)
return 1;
return n * factorial(n - 1);
}
int main(void) {
unsigned long n;
if(scanf("%lu", &n) != 1) {
printf("scanf failed\n");
return 1;
}
printf("%lu\n", factorial(n));
}
and example run:
6
720