Pointers in C, getting unintended results with arrays

Question

I want to calculate factorials for several numbers and return the results in the form of an array. Here is my program:

#include <stdio.h>
#include <stdlib.h>

int *fact(int arr[], int n) {
    int *facts, fac = 1, i, j;

    facts = (int *) malloc(n * sizeof(int));

    for (i = 0; i < n; i++) {
        for (j = 1; j <= *(arr + i); j++)
            fac = fac * j;

        *(facts + i) = fac;
    }

    return facts;
}

int main(void) {
    int *num, *facto, n, i;     //Initializing variables.

    printf("How many numbers to calculate factorial for :\t");
    scanf("%d", &n);

    num = (int*) malloc(n * sizeof(int));   //Dynamic allocation for array num.

    printf("Enter the elements separated by spaces :\t");

    for (i = 0; i < n; i++)
        scanf("%d", num + i);

    facto = fact(num, n);

    printf("\nFactorials are :\t");

    for (i = 0; i < n; i++)
        printf("%d\t", *(facto + i));

    printf("\n");

    return 0;
}

When it prints the elements of the returned array, however, only first value is correct. The others seem random.

What is wrong with this program, and how can I fix it?

Answer 1

the following proposed code:

1. checks for C library function errors
2. checks for user input validity
3. doesn't allow a negative or 0 for number of elements
4. doesn't allow a negative number for element value
5. eliminates the 'implicit conversions' found in the OPs code
6. eliminates unnecessary heap allocation
7. eliminates the memory leak
8. minimizes the 'scope' of the local variables
9. uses appropriate horizontal and vertical spacing for ease of readability and understanding
10. follows the axiom: only one statement per line and (at most) one variable declaration per statement.

and now, the proposed code:

#include <stdio.h>
#include <stdlib.h>


// prototypes
void fact( size_t arr[], int n);


void fact( size_t arr[], int n)
{

    for ( int i = 0; i < n; i++)
    {
        if( !arr[i] )
        {
            puts( "factorial for 0 not calculated" );
        }

        else
        {
            size_t fac = 1;
            for ( size_t j = arr[i]; j > 1; j-- )
            {
                fac = fac * j;
            }
            arr[i] = fac;
        }
    }
}


int main( void )
{
    int n;

    printf("How many numbers to calculate factorial for :\t");
    if( scanf("%d", &n) != 1)
    {
        fprintf( stderr, "scanf for number of factorials failed\n" );
        exit( EXIT_FAILURE );
    }

    // implied else, scanf successful

    if( n < 0 )
    {
        puts( "negative number of elements is invalid" );
        exit( EXIT_FAILURE );
    }

    // implied else, scanf for number of elements successful

    if( !n )
    {
        puts( "nothing to do" );
        exit( EXIT_FAILURE );
    }

    // implied else, valid number of elements

    size_t *num = malloc( (size_t)n * sizeof( size_t ) );
    if( !num )
    {
        perror( "malloc failed" );
        exit( EXIT_FAILURE );
    }

    // implied else, malloc successful

    printf("Enter the elements separated by spaces :\t");

    for ( int i = 0; i < n; i++ )
    {
        if( scanf( "%lu", &num[i] ) != 1 )
        {
            fprintf( stderr, "scanf for element failed\n" );
            free( num );
            exit( EXIT_FAILURE );
        }
    }

    fact(num, n);

    printf("\nFactorials are :\t");

    for ( int i = 0; i < n; i++ )
        printf("%lu\t", num[i] );

    printf("\n");

    // cleanup
    free( num );

    return 0;
}

Answer 2

As say previously, your problem lies in the variable "fac" not being reset to 1.

A nice way to keep your code short and less prone to error like that is by using function that only do one thing.

For example, your facts function allocate and calcul the factorial. You can split this function in two like that :

int factorial(int n)
{
    int result = 1;

    for (int i = 2; i <= n; ++i) {
        result *= i;
    }

    return (result);
}


int *fact(int arr[], int n)
{
    int *facts = NULL;

    if (!(facts = malloc(n * sizeof(*facts))) {
        // TODO Log (strerror(errno));
        return (NULL);
    }

    for (int i = 0; i < n ; ++i) {
        facts[i] = factorial(arr[i]);
    }

    return facts;
}

Answer 3

You need to reset fac to 1 before each for loop on j, otherwise on every calculation except the first the first factor will not start from 1:

for(i=0; i<n; i++) {   
   fac = 1; // HERE
   for(j=1; j<= *(arr+i); j++) {
        fac = fac*j;
   }
   *(facts+i) = fac;
}