8
\$\begingroup\$

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);
}
\$\endgroup\$

3 Answers 3

10
\$\begingroup\$

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);
}
\$\endgroup\$
17
  • 2
    \$\begingroup\$ SHUBHAM KULKARNI, "why should the main function always return int" --> main() returning int is allowed by all compilers. Returning void relies on implementation-defined compiler attributes. \$\endgroup\$ Commented Dec 28, 2022 at 16:01
  • 2
    \$\begingroup\$ @SHUBHAMKULKARNI "if the user does not pass any arguments then the if statements in the main function are there for backup" --> Too late. Those tests needed before attempting to read argv[1] in atoi(argv[1]). \$\endgroup\$ Commented Dec 28, 2022 at 16:03
  • 1
    \$\begingroup\$ Pedantic: num = labs(num); risks UB when num == LONG_MIN. \$\endgroup\$ Commented Dec 28, 2022 at 16:35
  • 1
    \$\begingroup\$ @chux-ReinstateMonica thanks; see edits. re. labs it's edgy enough that I think I'll leave it as is and preserve your comment. \$\endgroup\$
    – Reinderien
    Commented Dec 28, 2022 at 16:48
  • 1
    \$\begingroup\$ The code in the question doesn't contain the do keyword or any do{}while() loops. It has some int var=value; while(){} loops that should be for() or do{}while() loops, though! (Especially if the do{}while() let the controlling expression see variables declared inside the loop, it would be great for loops that discover an exit condition instead of having a counter. But since it doesn't, it's often not that wonderful.) \$\endgroup\$ Commented Dec 30, 2022 at 13:39
5
\$\begingroup\$

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().

\$\endgroup\$
1
\$\begingroup\$

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
\$\endgroup\$
4
  • 1
    \$\begingroup\$ if d * d > n overflows when n is a large prime. Consider if d > n/d. \$\endgroup\$ Commented Dec 30, 2022 at 13:45
  • 2
    \$\begingroup\$ On many machines, you can get the quotient for basically free while getting the remainder. That being the case, n / d <= d is a cheap way to detect that you've reached d >= sqrt(n) without doing a sqrt(n) once or an extra multiply every time. (Checking if a number is prime in NASM Win64 Assembly) Oh right, and as chux mentions, overflow is a possible problem in C, unlike in Python which you've used for this answer. (n can be greater than the largest non-overflowing perfect square) \$\endgroup\$ Commented Dec 30, 2022 at 13:45
  • \$\begingroup\$ @chux-ReinstateMonica true - one of the benefits of working in pypy :) \$\endgroup\$
    – qwr
    Commented Dec 30, 2022 at 14:12
  • \$\begingroup\$ qwr, Such benefits come with a cost. IAC, post is tagged C and deserves C considerations. \$\endgroup\$ Commented Dec 30, 2022 at 14:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.