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?

Just for your information, for any pointer *or* array `facts` and index `i`, the expression `*(facts + i)` is *exactly* equal to `facts[i]`. The latter is usually easier to read and understand, and also saves you writing a couple of characters. Also please read [Do I cast the result of malloc?](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) — Some programmer dude, Jun 28 '18 at 13:12
As for your question, please edit it you show your input, the actual output from that input, and the expected output. And of course please [learn how to debug your programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/). — Some programmer dude, Jun 28 '18 at 13:14
OT: when calling any of the `scanf()` family of functions, always check the returned value (not the parameter values) to assure the operation was successful. — user3629249, Jun 29 '18 at 15:40
OT: when calling any of the heap allocation functions: `malloc` `calloc` realloc` 1) the returned type is `void*` which can be assigned to any other pointer. Casting just clutters the code, making it more difficult to understand, debug, etc. 2) always check (!=NULL) the returned value to assure the operation was successful — user3629249, Jun 29 '18 at 15:42

score 2 · Answer 1 · answered Jun 28 '18 at 13:18

2

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;
}

answered Jun 28 '18 at 13:18

FBergo

1,010
8
11

Yes, and a good practice that reduces the occurrence of problems such as this is to keep variables' scope as narrow as possible. In this case, `fac` is used only inside the outer loop, and its values do not need to persist from one iteration to the next, so `fac` ought to be *declared* inside the loop. – John Bollinger Jun 28 '18 at 13:53
@JohnBollinger fac variable is useless, it's better to use facts[i] directly. – Tom's Jun 28 '18 at 13:59
That's a question of style, @Tom's. I'd agree that `fac` is *unnecessary* from a program logic perspective, but "useless" is not at all clear. Under different circumstances, using a separate scalar variable could even serve a functional purpose. – John Bollinger Jun 28 '18 at 14:05
@JohnBollinger I agree that I should use unnecessary rather than useless, and yes, in some specifique circumstance, it's better to use another variable. But in this case, "fac" variable add "complexity" to code for nothing. By complexity, I mean that there is more code to read, more variable to keep in mind, etc etc. For this example, it seem ridiculous, but this habit of adding unnecessary variable often lead to add a fair amout of them, and this is where the real trouble begin, because when you read this kind of code, you wonder whether the variable have a real purpose or not. – Tom's Jun 28 '18 at 14:14
There are several style issues in his code, which is not unusual for beginners. I tried to keep my answer to the point, which was why his code was not computing the expected values, and provided a minimal fix without overhauling the entire code. – FBergo Jun 28 '18 at 14:23
@Tom's, clearly the OP did not think that his use of `fac` was for nothing. It's possible, of course, that the purpose he saw for it was illusory, but there are *bona fide* style reasons that are perfectly applicable, notwithstranding that *your* preferred style does not call for it in this circumstance. For instance, the OP might simply find it easier to read or to reason about. Even after accepting "unnecessary" as a more applicable term, you still seem to be arguing that introducing `fac` here is somehow objectively wrong. I do not accept that. – John Bollinger Jun 28 '18 at 14:32
@JohnBollinger Well, if you going like that, everything is subjective, even declaring fac in the for loop. Not adding "unecessary/useless" variable is part of "good practice" that everyone is not bound to follow. Yes, this is _my_ preferred style code, but when you begin review a code which has function over 400 lines, and each function declare ~20 variables, and you found out that 7 of them is "unnecessary" plus the fact that each variables is only needed at the beginning, middle or last part of the function (emphazing the fact that the function could be cut in smaller [...] – Tom's Jun 28 '18 at 15:13
[...] function), then yes, you begin to be more rigid. For the story, I see that coding style the most on the code of beginner. These beginner only code because the use an arduino (I used to help them). Yes, it's only my story, yes it's only a small part, yes it's only me, but the more common trap for beginner is exactly having some coding habit which lead them to make mistake like that. I do not say that my way is the only good, I just say there is way tha can lead to "stupid" bug, and there is way to reduce the occurence. That's why I use OTB style code. Yet I didn't say the OP must use it. – Tom's Jun 28 '18 at 15:22

score 1 · Answer 2 · answered Jun 28 '18 at 14:02

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;
}

score 0 · Answer 3 · answered Jun 29 '18 at 16:44

the following proposed code:

checks for C library function errors
checks for user input validity
doesn't allow a negative or 0 for number of elements
doesn't allow a negative number for element value
eliminates the 'implicit conversions' found in the OPs code
eliminates unnecessary heap allocation
eliminates the memory leak
minimizes the 'scope' of the local variables
uses appropriate horizontal and vertical spacing for ease of readability and understanding
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;
}

Pointers in C, getting unintended results with arrays

3 Answers3