Find divisors and store them in array

Question

I have written the following code which contans two functions

1.To find the number of divisors of a number

2.To store the divisors in an array.

#include <stdio.h>

int numOfDivisors(int n)
{
    int i, numOfDivisors=0;

    for(i=1;i<=n;i++)
    {
        if(n%i==0)
        {
            numOfDivisors++;
        }
    }
    return numOfDivisors;
}

void storeDivisors(int count,int divisors[],int n)
{
    int i,j;
    for(i=1,j=0;i<=n,j<count;i++,j++)
    {
        if(n%i==0)
        {
            divisors[j]=i;
        }
    }

}

int main()
{
    int n,m,a,i;

    scanf("%d %d %d",&n,&m,&a);

    int count1=numOfDivisors(n);
    int count2=numOfDivisors(m);

    /*printf("%d %d",count1,count2);*/

    int divisors1[count1],divisors2[count2];

    storeDivisors(count1,divisors1,n);
    storeDivisors(count2,divisors2,m);

    for(i=0;i<count1;i++)
    {
        printf("%d\t%d\n",divisors1[i],divisors2[i]);
    }

    return 0;
}

This is just part of a bigger code I need to make.

I am getting junk values. What am I doing wrong?

R Sahu · Accepted Answer · 2015-04-15T17:45:46.580

I am getting junk values. What am I doing wrong?

If count2 is less than count1, divisors2 does not have as many values and you end up accessing the array out of bounds.

Print the two divisors separately.

for(i=0;i<count1;i++)
{
    printf("%d\n",divisors1[i]);
}

for(i=0;i<count2;i++)
{
    printf("%d\n",divisors2[i]);
}

Also, as was pointed out by @BLUEPIXY, you have an indexing error in storeDivisiors.

Instead of

for(i=1,j=0;i<=n,j<count;i++,j++)
{
    if(n%i==0)
    {
        divisors[j]=i;
    }
}

use

for(i=1,j=0;i<=n,j<count;i++)
{
    if(n%i==0)
    {
        divisors[j++]=i;
        //       ^^^^ increment j only when storing a divisor.
    }
}

score 2 · Answer 2 · answered Apr 15 '15 at 17:45

The loop in your storeDivisors() function is wrong. In it, variable j appears intended to represent the index in the divisors array where the next divisor should be stored, but in that case you should increment it only when you in fact store a divisor. Instead, you increment it at every iteration. You will end up with some divisors[] positions containing junk values, and not all divisors being stored.

The loop should look like this:

int i,j;
for(i=1,j=0;i<=n,j<count;i++)
{
    if(n%i==0)
    {
        divisors[j++]=i;
    }
}

score 1 · Answer 3 · edited May 23 '17 at 11:51

The problem is in :

for(i=1,j=0;i<=n,j<count;i++,j++)
{
    if(n%i==0)
    {
        divisors[j]=i;
    }
}

j is always equal to i-1. Consequently, most of items in array divisor will never be set.

Try :

for(i=1,j=0;i<=n,j<count;i++)
{
    if(n%i==0)
    {
        divisors[j]=i;
        j++;
    }
}

That way, j increases if, and only if, a divisor is found.

The next step is to use dynamic memory allocation, to avoid computing the divisors twice and use a "resizable" array. The function realloc() of stdlib may inspire you. To avoid reallocating the array each time a new divisor is found, a structure similar to a vector will be needed. See How to replicate vector in c? or Converting C++ implementation of vector in C

score 0 · Answer 4 · answered Jun 30 '15 at 06:23

I am answering my own question here, I have gotten better since the past few days and intend to improve my above code.

I have eliminated the need to make a function to find the number of divisors of n since that is immaterial in storing the divisors in an array. I have improved my storeDivisors function which not only stores all the divisors but if needed will also give the count of divisors. Here is the updated code --

#include <stdio.h>

void storeDivisors(int n,int a[],int *x)
{
    int i;
    for(i=1;i<=n;i++)
    {
        if(n%i==0)
            a[(*x)++]=i;
    }
}

int main()
{
    int n = 100;
    int a[100];
    int x = 0;
    int i;

    storeDivisors(n,a,&x);

    for(i=0;i<x;i++)
        printf("%d\n",a[i]);
    printf("number of divisors = %d\n",x);

    return 0;
}

Find divisors and store them in array

4 Answers4