4
\$\begingroup\$

While reviewing a C question I started writing my own implementation and in no time I had a completely refactored completely different implementation. As the asker is learning I did not want to post it, instead I ask you for further review and possibilities of improvement.

#include <stdio.h>
#include <stdbool.h>

int get_side(name) {
    printf("Side-%d has value: ", name);
    int value; scanf("%d", &value);
    return value;
}

bool is_valid_triangle(side_1,side_2,side_3) {
    bool all_positive = side_1 > 0 && side_2 > 0 && side_3 > 0;
    bool correct_sized_sides = side_1 + side_2 > side_3 && \
                               side_2 + side_3 > side_1 && \
                               side_3 + side_1 > side_2;
    return all_positive && correct_sized_sides;
} 

bool is_equilater(side_1,side_2,side_3) {
    return (side_1 == side_2 && side_2 == side_3);
}

bool is_isosceles(side_1,side_2,side_3) {
    return (side_1 == side_2) || (side_2 == side_3) || (side_3 == side_1);
}

bool is_scalen(side_1,side_2,side_3) {
    return ! is_isosceles(side_1,side_2,side_3);
}

const char * triangle_categorizer(side_1,side_2,side_3) {
    if (is_equilater(side_1,side_2,side_3)) {
        return "equilater";
    }

    if (is_isosceles(side_1,side_2,side_3)) {
        return "isosceles";
    }

    if (is_scalen(side_1,side_2,side_3)) {
        return "scalen";
    }
}

int main() {
    int side_1 = get_side(1);
    int side_2 = get_side(2);
    int side_3 = get_side(3);

    if (! is_valid_triangle(side_1,side_2,side_3)) {
        puts("The triangle you intered is invalid!");
        return 1;
    }
    printf("The triangle you entered is %s.\n", \
           triangle_categorizer(side_1,side_2,side_3));
    return 0;
}
\$\endgroup\$
5
\$\begingroup\$

A few simple comments:

Spelling:

Triangles are either Isosceles, Equilateral, or Scalene

Also instead of using string literals, replace them with macros like so:

#define EQUILATERAL "equilateral"
#define ISOSCELES "isosceles"
#define SCALENE "scalene"

I commend you on using long names in c, as many c-programmers use 1-2 letter names for things. I might suggest adding comments to complex code, such as mentioning the fact that you are using triangle inequality in your is_valid_triangle function

I would also suggest extending this program to work with non-integers, such as long longs, doubles and the like. In addition, it may be more helpful to a user to pass in coordinates instead of side lengths. That would also allow you to not check if a triangle is valid or not, as technically all 3 point pairs form a triangle (even if it is degenerate).

\$\endgroup\$
1
  • \$\begingroup\$ Very nice point about using points instead of sides. (No pun intended. I swear.) \$\endgroup\$
    – RubberDuck
    Mar 23 '15 at 23:12
4
\$\begingroup\$

There are multiple issues I can see with your implementation.

Always validate

All the is_* methods require the calling of the is_valid_triangle(...) as a separate operation. I would recommend adding the is_valid_triangle() as part of all the triangle-type checks (even the is_equilater call). This will prevent the methods returning true for failing conditions... for example, the following are all nonsensical, but return true nonetheless:

is_equilater(0,0,0);
is_equilater(-1,-1,-1);
is_isosceles(0,0,0);
is_isosceles(10,10,10000);

No-return value problems

This code has a redundant if-condition in the last check, and in has a 'logic path' which does not have a return statement:

const char * triangle_categorizer(side_1,side_2,side_3) {
    if (is_equilater(side_1,side_2,side_3)) {
        return "equilater";
    }

    if (is_isosceles(side_1,side_2,side_3)) {
        return "isosceles";
    }

    if (is_scalen(side_1,side_2,side_3)) {
        return "scalen";
    }
}

There should be some form of terminating "return" expression, or, alternatively the last if-statement should be removed...:

const char * triangle_categorizer(int side_1, int side_2, int side_3) {
    if (is_equilater(side_1,side_2,side_3)) {
        return "equilater";
    }

    if (is_isosceles(side_1,side_2,side_3)) {
        return "isosceles";
    }

    return "scalen";
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ To answer your question, "how does this compile?" — in C, function parameters without an explicit type have type int. \$\endgroup\$ Mar 23 '15 at 22:35
  • \$\begingroup\$ Mutter mutter... thanks @GarethRees TIL that C supports default paameter types. \$\endgroup\$
    – rolfl
    Mar 23 '15 at 22:36
  • 1
    \$\begingroup\$ Not just function parameters — function return types and global variables too! (This was standardized in C89 — see §3.5.2 — but in C99 the "implicit int" rule was dropped.) \$\endgroup\$ Mar 23 '15 at 22:48
3
\$\begingroup\$

The juxtaposition of C99 bool and K&R-style function parameters is… "interesting". In the original C language, you could write functions like

double power(x, y)
           double x; unsigned int y; {
    double result = 1;
    while (y --> 0) {
        result *= x;
    }
    return result;
}

… with the type specifications being optional (defaulting to int). This style of parameter lists has been out of fashion since ANSI C came out in 1989, and is allowed today only for compatibility with ancient code. GCC and Clang would complain if you compiled using the -pedantic flag.

But why limit yourself to triangles of integer-length sides? Very few geometry problems fit that mold. You should be using doubles for all lengths.

The way you truncated the names is_equilateris_equilateral and is_scalenis_scalene reminds me of Ken Thompson's greatest regret: dropping the "e" at the end of the name of the creat function.

In summary, you should write

bool is_scalene(double side1, double side2, double side3) {
    …
}
\$\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.