4
\$\begingroup\$

I wrote a simple program to perform g.c.d and l.c.m., trying to follow the Linux kernel coding style. I know C for about one year, but I'm still a beginner.

Example:

user@compuer:~/projects/tmp$ ./tmp 21 45
G.C.D: 3
l.c.m: 315

Code:

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

/*
 * It returns the greatest common divisor between two numbers. The numbers
 * must not be negative (undefined behaviour).
 */
int gcd(int a, int b);

/*
 * It returns the least common multiple. The numbers must not be negative
 * (undefined behaviour).
 */
int lcm(int a, int b);

int main(int argc, char *argv[])
{
        int retval = -1, a, b;

        if (argc != 3) {
                fprintf(stderr, "Error: missing or too many arguments\n");
                goto err_exit;
        }

        a = strtol(argv[1], NULL, 0);
        b = strtol(argv[2], NULL, 0);

        if (a < 0 || b < 0) {
                fprintf(stderr, "Error: numbers must be postive\n");
                goto err_exit;
        }

        if (!a || !b)
                printf("Warning: one or both numbers are 0.\n");

        printf("G.C.D: %d\nl.c.m: %d\n", gcd(a, b), lcm(a, b));

        retval = 0;

        err_exit:
                return retval;
}

int gcd(int a, int b)
{
        int retval = b, r = a;

        if (!a)
                goto exit;

        // Euclidean alrgorithm.
        do {
                a = b;
                b = r;

                r = a % b;
        } while (r);

        retval = b;

        exit:
                return retval;
}

int lcm(int a, int b)
{
        int retval = 0;

        if (a && b)
                retval = a / gcd(a, b) * b;

        return retval;
}

Makefile:

.PHONY = clean all

PROGNAME = tmp

SHELL = /bin/sh

CC ?= gcc

CFLAGS = -Wall -Wextra

all : main.o
    $(CC) -o $(PROGNAME) $^

clean :
    $(RM) *.o $(PROGNAME)
\$\endgroup\$
  • \$\begingroup\$ Minor: Spelling "alrgorithm" --> "algorithm" \$\endgroup\$ – chux Aug 9 '17 at 4:01
  • \$\begingroup\$ More: "postive" --> "positive". Run spell check \$\endgroup\$ – chux Aug 9 '17 at 18:34
6
\$\begingroup\$
  1. I don't see the point in using goto here. There's no resource clean up or any other strong reason to resort to it. It just makes the control flow more complicated.

  2. How to fix it? In the main function, you can just return the value immediately in case of an error. In the gcd function, you can run the while loop with a condition a != 0. It'll make the code easier to folloow and eliminate a corner case a = 0.

  3. The error message fprintf(stderr, "Error: missing or too many arguments\n"); seems too generic to me. It would be more helpful if it were more detailed (too many and missing arguments are clearly different cases).

  4. An LCM of two int's may not fit into int. I'd recommend to document that your program has undefined behavior if the LCM is too large or use a wider integer type.

\$\endgroup\$
  • \$\begingroup\$ Thanks, how can I show the updated code? do I need to edit the original post? \$\endgroup\$ – It_bump Aug 8 '17 at 14:57
  • 1
    \$\begingroup\$ @It_bump I don't think that updating the post is good idea (because it changes the contes significantly and makes my answer irrelevant). If you're happy with code you've got, you can just keep the updated version for yourself. If you want it to be reviewed, too, I suggest creating a new post. \$\endgroup\$ – kraskevich Aug 8 '17 at 15:00
  • \$\begingroup\$ ok, really thanks for the review, I fixed the code! \$\endgroup\$ – It_bump Aug 8 '17 at 15:06
  • \$\begingroup\$ @It_bump - the advice is summarized in What should I do when someone answers my question? - specifically, in the section headed "I improved my code based on the reviews. What next?". \$\endgroup\$ – Toby Speight Aug 9 '17 at 11:38
1
\$\begingroup\$

strtol() returns a long, as its name suggests. You should not assign the result to an int.

\$\endgroup\$
  • 2
    \$\begingroup\$ he might also use strtoul and rewrite it all for unsigned longs, since the specification in the comment declares the program is for non-negative values only. it will set errno to ERANGE which he should be checking for, eliminating the undefined behavior. linux.die.net/man/3/strtoul \$\endgroup\$ – j. andrew shusta Aug 9 '17 at 17:41
0
\$\begingroup\$

Since zero is not a positive value, it's not necessary to test separately for negative and for zero. Instead, a single <= will do the job:

        if (a <= 0 || b <= 0) {
                fprintf(stderr, "Error: numbers must be positive\n");
                return 1;
        }

I'm not a big fan of the single-return philosophy when there are no resources to release; by returning immediately, we avoid the need for the retval variable.


Calculating the Lowest Common Multiple is (naturally) done by calling gcd(), but this is wasteful in this program, where we already have the Greatest Common Divisor. You might want to consider an alternative interface (perhaps a separate version that accepts GCD as an argument, or always accept it as argument, and calculate it within the function if 0 is supplied; alternatively, consider computing both values and returning through reference).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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