A Simple C Program to Find Factors of any Integer

Question

I have written a simple CLI app that finds factors in 3 different forms. I would like the code to be reviewed.

Here's the code:

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

void eq(int num)
{
    int factor_found = 0, factor = 2;
    printf("%d = " , num);

    while(factor_found==0)
    {
        if(num%factor==0 && num!=0)
        {
            printf("%d*" , factor);
            num = num/factor;
        }
        else if(num==-1 || num==0 || num==1)
        {
            printf("%d." , num);
            factor_found=1;
        }
        else
        {
            ++factor;
        }
    }
}

void n(int num)
{
    int factor_found = 0, factor = 2;
    printf("Factors of %d are : ", num);

    if(num==1 || num==-1)
    {
        printf("%d,%d.", num, num*-1);
    }
    else if(num==0)
    {
        printf("Math Error.");
    }
    else
    {
        printf("1, -1, ");
        while(factor_found==0)
        {
            if(num%factor==0 && num!=factor && num!=factor*(-1))
            {
                printf("%d, %d, ", factor, (factor*(-1)));
                ++factor;
            }
            else if(num==factor || num==factor*(-1))
            {
                printf("%d, %d.", num, (num*(-1)));
                ++factor_found;
            }
            else
            {
                ++factor;
            }
        }
    }
}

void poc(int num)
{
    int factor_found = 0, factor = 2, no_of_factors = 0, temp;
    if(num==1 || num==0 || num==-1)
    {
        printf("%d is nor prime nor composite.", num);
    }
    else
    {
        temp = num;
        num = abs(num);
        while(factor_found==0)
        {
            if(num%factor==0 && num!=factor)
            {
                ++no_of_factors;
                num=num/factor;
            }
            else if(num%factor==0 && (num==factor || factor==1))
            {
                num=num/factor;
            }
            else if(num==1 || num==-1)
            {
                ++factor_found;
            }
            else
            {
                ++factor;
            }
        }

        if(no_of_factors==0)
        {
            printf("%d is a prime number.", temp);
        }
        else if(no_of_factors!=0)
        {
            printf("%d is a composite number.", temp);
        }
    }
}

void check(char mode[5], int num)
{
    if(strcmp(mode, "-eq")==0 || strcmp(mode, "eq")==0)
        eq(num);
    else if(strcmp(mode, "-n")==0 || strcmp(mode, "n")==0)
        n(num);
    else if(strcmp(mode, "-poc")==0 || strcmp(mode, "poc")==0)
        poc(num);
    else if(strcmp(mode, "-all")==0 || strcmp(mode, "all")==0)
    {
        eq(num); printf("\n");
        n(num); printf("\n");
        poc(num);
    }
    else
        printf("Invalid Argument!");
}

void main(int argc, char *argv[])
{
    int num = atoi(argv[1]); char mode[5];

    if(argc==1)
    {
        printf("Enter the number : ");
        scanf("%d" , &num);
        printf("Enter the mode : ");
        scanf("%s" , mode);
        check(mode, num);
    }
    else if(argc==2)
    {
        printf("Enter the mode : ");
        scanf("%s" , mode);
        check(mode, num);
    }
    else
        check(argv[2], num);
}

Answer 1

conio is not a portable library so you should not rely on it.

You should include stdbool.h and represent factor_found as a bool. However, in all cases, use of this flag is uncalled-for and you should simply break.

In all cases, your use of while is better-represented by for.

All of your functions other than main should be static.

num==-1 || num==0 || num==1 should be reduced to two inequality comparisons.

n as a function needs a better name. I don't know what that does unless I read the whole source.

factor*(-1) should just be -factor.

main should always return int.

If the user does not pass any arguments, we expect atoi(argv[1]) to produce a memory fault. If it doesn't, that's entirely by coincidence: welcome to C.

atoi is not a safe function because it cannot tell you whether user input is valid or not. Prefer instead something like strtol, and either bounds-check and cast back to an int, or (more easily) use long throughout.

You should be compiling against the C17 standard, and using its features where appropriate, especially moving variable declaration away from the function start and closer to use.

Whenever you print an error, you should usually print to stderr and not stdout.

temp is not a good variable name. In this case, prefer something like orig_num.

The responsibility for printing terminating newlines should be moved into your individual functions.

Factors "%d, %d, ", factor, (factor*(-1))); have a slightly wobbly definition. See divisor; some definitions expect that only positive divisors are shown.

check should not accept a char array, since it should work with any string length; instead accept const char *.

Reduce repetition in main by only accepting the mode string once.

Suggested

I believe this to be equivalent but you should verify. This has been compiled with the following makefile:

282213: main.o
    gcc -o $@ $^

main.o: main.c
    gcc -o $@ $^ -c -Wall -std=c17

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


static void eq(long num)
{
    printf("%ld = ", num);

    for (int factor = 2;;)
    {
        if (num >= -1 && num <= 1)
        {
            printf("%ld." , num);
            break;
        }
        if (num%factor==0)
        {
            printf("%d*", factor);
            num /= factor;
        }
        else ++factor;
    }

    putchar('\n');
}

static void n(long num)
{
    printf("Factors of %ld are: ", num);

    if (num==1 || num==-1)
        printf("%ld,%ld.", num, -num);
    else if (num==0)
        fprintf(stderr, "Math Error.\n");
    else
    {
        printf("1, -1, ");
        for (long factor = 2;; ++factor)
        {
            if (num==factor || num==-factor)
            {
                printf("%ld, %ld.", num, -num);
                break;
            }
            if (num%factor==0)
                printf("%ld, %ld, ", factor, -factor);
        }
    }

    putchar('\n');
}

static void poc(long num)
{
    if (num >= -1 && num <= 1)
    {
        printf("%ld is nor prime nor composite.\n", num);
        return;
    }

    long orig_num = num;
    int no_of_factors = 0;
    num = labs(num);
    for (long factor = 2;;)
    {
        if (num%factor==0 && num!=factor)
        {
            ++no_of_factors;
            num /= factor;
        }
        else if (num%factor==0 && (num==factor || factor==1))
            num /= factor;
        else if (num==1 || num==-1)
            break;
        else
            ++factor;
    }

    if (no_of_factors==0)
        printf("%ld is a prime number.\n", orig_num);
    else printf("%ld is a composite number.\n", orig_num);
}

static void check(const char *mode, long num)
{
    if (!strcmp(mode, "-eq") || !strcmp(mode, "eq"))
        eq(num);
    else if(!strcmp(mode, "-n") || !strcmp(mode, "n"))
        n(num);
    else if(!strcmp(mode, "-poc") || !strcmp(mode, "poc"))
        poc(num);
    else if(!strcmp(mode, "-all") || !strcmp(mode, "all"))
    {
        eq(num);
        n(num);
        poc(num);
    }
    else
        fprintf(stderr, "Invalid mode\n");
}


int main(int argc, const char **argv)
{
    char mode_entry[5];
    const char *mode;
    long num;
    
    if (argc < 2)
    {
        printf("Enter the number: ");
        if (scanf("%ld", &num) != 1)
        {
            fprintf(stderr, "Invalid number\n");
            return 1;
        }            
    }
    else
    {
        char *endptr = NULL;
        num = strtol(argv[1], &endptr, 10);
        if (endptr == argv[1])
        {
            perror("Invalid number");
            return 1;
        }
    }

    if (argc < 3)
    {
        printf("Enter the mode (eq|n|poc|all): ");
        if (scanf("%4s", mode_entry) != 1)
        {
            fprintf(stderr, "Invalid mode string\n");
            return 2;
        }
        mode = mode_entry;
    }
    else
        mode = argv[2];
    
    check(mode, num);
}

Answer 2

Do not use scanf("%s".. without a width

Prevent buffer overruns.
Use a width, which limits the characters read, to 1 less than the array count.

char mode[5];
// scanf("%s" , mode);
scanf("%4s" , mode);

Or to drive the width from one place, use the pre-processor # operator.
Makes for easier maintenance.

#define STRINGIFY(x) #x
#define TOSTRING(x) STRINGIFY(x)

#define MODE_LEN 4
char mode[MODE_LEN + 1];
scanf("%" TOSTRING(MODE_LEN) "s" , mode);

Better code would also check the return value of scanf().

Answer 3

Function names

n and eq are very unclear names, unless they clearly correspond to a referenced formula. eq usually stands for "equals" or "equation", and it's not clear what "equals" would do for one input.

Input handling

I assume you have a consistent way to handle negative numbers. You can simply output -1 as a factor, test for 0, and then only consider positive integers. This would reduce the negative number and zero checks.

Trial Division Algorithm

I think a for loop testing divisors from 2 to n instead of unbounded incrementing is easier to understand. A simple optimization is to divide out 2s and then odds. In fact, you need to only try dividing out primes. (Why is this true?) In this way, using a sieve to get a list of primes and only trying dividing those will be much faster for large inputs (prime number theorem). By the way, the largest factor to try division is sqrt(n); if n leftover after dividing is not 1 then that is the largest prime factor. (Why is this true?) This cuts down the worst-case number of checks from O(n) to O(sqrt n).

Here is python / pseudocode that factors a number into a dictionary of its prime factorization (similar to sympy's factorint):

def factor(n):
    assert n > 0  # only handle positive integer input

    factors = Counter()
    for d in range(2, n):
        if d * d > n: break  # d up to sqrt(n)
        while n % d == 0:
            n //= d
            factors[d] += 1

    if n > 1: 
        factors[n] += 1
    return factors