4
\$\begingroup\$

This is a basic 6 trigonometric functions calculator that I will incorporate into my main calculator project. It works as intended, but the BUILD MESSAGES issue a warning, the warning being: Warning: 'op' is used uninitialized in this program.

The program works as intended, the only problem being that it gives one warning. Understanding how functions work - am I missing something?

Does op = getOperation(op), not assign the value to op? And if it doesn't, why does the program work properly?

Any other tips on how to make this program better would be much appreciated. Thank you.

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

#ifdef M_PI
#define PI M_PI
#else
#define PI 3.14159265358979323846264338327950288420
#endif // M_PI

float degreesToRadians(float angle);
unsigned int getOperation(unsigned int op);
float sine(float degrees);
float cosine(float degrees);
float tangent(float degrees);
float sineInverse(float degrees);
float cosineInverse(float degrees);
float tangentInverse(float degrees);

int main()
{
    float degrees;
    unsigned int op;

    op = getOperation(op);

    printf("\nEnter the angle in degrees: ");
    scanf("%f", &degrees);

    degrees = degreesToRadians(degrees);

    switch(op)
    {
    case '1' :
        sine(degrees);
        break;

    case '2':
        cosine(degrees);
        break;

    case '3':
        tangent(degrees);
        break;

    case '4':
        sineInverse(degrees);
        break;

    case '5':
        cosineInverse(degrees);
        break;

    case '6':
        tangentInverse(degrees);
        break;

    default:
        printf("You entered an invalid option!");
        break;
    }
    return 0;
}
//------------------------------------------------------------------------------------------------------------------------------------------------------------
//Degrees to Radians Function

float degreesToRadians( float degrees)
{
    return degrees * (PI / 180);
}

//----------------------------------------------------------------------------------------------------------------------------------------------------------

unsigned int getOperation(unsigned int op)
{
    printf("Trigonometric Functions: \n");
    printf("------------------------ \n");
    printf("1. Sine Function\n");
    printf("2. Cosine Function\n");
    printf("3. Tangent Function\n");
    printf("4. Sine Inverse Function\n");
    printf("5. Cosine Inverse Function\n");
    printf("6. Tangent Inverse Function\n");
    op = getch();

    return op;
}
//------------------------------------------------------------------------------------------------------------------------------------------------------------
//Sine function
float sine(float degrees)
{
    float result = sin(degrees);
    printf("Answer: %.2f",result);
    return result;
}
//------------------------------------------------------------------------------------------------------------------------------------------------------------
//Cosine function

float cosine(float degrees)
{
    float result = cos(degrees);
    printf("Result: %.2f", result);
    return result;
}
//------------------------------------------------------------------------------------------------------------------------------------------------------------
//Tangent function

float tangent(float degrees)
{
    float result = tan(degrees);
    printf("Result: %.2f", result);
    return result;
}
//----------------------------------------------------------------------------------------------------------------------------------------------------------
//Sine Inverse function

float sineInverse(float degrees)
{
    float result = asin(degrees);
    printf("Result: %.2f", result);
    return result;
}
//----------------------------------------------------------------------------------------------------------------------------------------------------------
//Cosine Inverse function

float cosineInverse(float degrees)
{
    float result = acos(degrees);
    printf("Result: %.2f", result);
    return result;
}
//----------------------------------------------------------------------------------------------------------------------------------------------------------
//Tangent Inverse function

float tangentInverse (float degrees)
{
    float result = atan(degrees);
    printf("Result: %.2f", result);
    return result;
}
//----------------------------------------------------------------------------------------------------------------------------------------------------------
\$\endgroup\$
4
  • 3
    \$\begingroup\$ Normally, asking for help to fix a specific problem makes a question out of scope for Code Review. However, in this case, the program really does work despite the undefined behavior, and I think the rest of the code could benefit from a general critique, so I would choose to let it slide. \$\endgroup\$ Sep 27, 2022 at 15:45
  • \$\begingroup\$ @ 200_success Thank you for that. Would you be against critiquing it, though? \$\endgroup\$
    – Harith
    Sep 27, 2022 at 16:49
  • \$\begingroup\$ Can you describe your compilation environment and your compiler flags? \$\endgroup\$
    – Reinderien
    Sep 27, 2022 at 20:06
  • \$\begingroup\$ @200_success Removed my comment, changed -1 to +1, retracted VTC. \$\endgroup\$
    – pacmaninbw
    Sep 28, 2022 at 14:54

5 Answers 5

2
\$\begingroup\$

Don't use arguments just to declare the variable

You have

unsigned int getOperation(unsigned int op)

then inside the function, you print some strings that don't have anything to do with op. The next lines that use op are

    op = getch();

    return op;

So you never use the argument op at all except as a declaration. You could instead say

unsigned int getOperation()

and

    return getch();

And you would get the exact same result.

Note that getch is non-standard. getchar is more common (but operates slightly differently). I don't immediately find an example where getch returns an unsigned value, but as it's non-standard, its return type is implementation dependent.

Using a variable before initialization

Now, in your calling function, you have

    unsigned int op;

    op = getOperation(op);

and ask

Does op = getOperation(op), not assign the value to op?

Yes, it does. And that's why the program works. But that's not what the warning is telling you. Let's rewrite the code slightly:

    unsigned int op;

    getOperation(op);

This will give the same warning. Because, at this point, you have not initialized op.

Note that assignment is right associative and lower priority than a function call. This means that getOperation(op) will run before the assignment. Since the assignment is also the initialization of op, the warning is correct. You are attempting to pass op as an argument before initializing it. Then, after that, you initialize it. The compiler is not smart enough to realize that you never use the uninitialized value of op other than to pass it to the function.

Now, if you remove the argument, as suggested in the first section, you'll have

    unsigned int op;

    op = getOperation();

which will work without giving the warning. In most versions of C, you could write

    unsigned int op = getOperation();

which is shorter.

Arc functions don't take angles

You call arcsine, arccosine, and arctangent functions with values in radians (that you call degrees). But these functions don't take angular measures as arguments. They take ratios between two sides as argument and return the angular measure (or arc) that corresponds with that ratio.

This is more than just a naming issue. Converting from degrees to radians on the values entered here can never return a meaningful result. It would make more sense to take two sides for those functions. Then you could calculate the ratio before passing that to the appropriate arc function.

\$\endgroup\$
3
\$\begingroup\$

It's clear that you care about precision, since your M_PI is specified to 38 (!) decimals. It would take a 128-bit floating point number, aka. "quad precision", to make proper use of that. Why, then, do you use float everywhere else (implemented on most common platforms as 32-bit IEEE 754 single precision floating point)? Surely these should be at least 64-bit double. Or, since you only print to two decimal places, maybe float is fine after all, but then it isn't strictly necessary to go so crazy with M_PI.

In such a small program with a very simple call tree, it's not necessary to pre-declare your functions. Just define them with no pre-declaration in order of dependency, main last.

You aren't exporting anything from this single translation unit so make all functions static other than main.

This:

    scanf("%f", &degrees);
    degrees = degreesToRadians(degrees);

is downright nasty. The variable name degrees is true until it isn't. Once it becomes a quantity in radians, you very much should not re-use that variable, and should declare a second one called radians. Almost all of your function arguments should be called radians and not degrees.

Also, scanf returns information to let you know whether the input was successfully parsed; don't ignore that.

Extract printf("Result: %.2f", result); away from your logic functions; do that in an upper function only once.

None of your comments explain more than the code itself, so delete them all.

Delete the argument to getOperation and replace it with (void). Change its return type from an unsigned int to a char.

\$\endgroup\$
7
  • \$\begingroup\$ While I agree with most of what you say, it is wrong to change the result of getOperation to char. getch does not return a char. The more typical return type is a signed int. \$\endgroup\$
    – mdfst13
    Sep 28, 2022 at 3:32
  • \$\begingroup\$ @Reinderien I'll make the changes now. Thank you. \$\endgroup\$
    – Harith
    Sep 28, 2022 at 17:28
  • \$\begingroup\$ @mdfst13 If your concern is that getch may return EOF (-1), you may instead be thinking of getchar; getch doesn't do that. \$\endgroup\$
    – Reinderien
    Sep 28, 2022 at 23:30
  • \$\begingroup\$ " It would take a 128-bit floating point number" is amiss. Lengthy decimal coded FP constants can affect float. Consider a number in code that is exactly half-way between two float values and it rounds toward 0 to form the float due to tie-breaker rules. Now append 00000000000000000000000000001 to that same code and the it rounds away from 0 to form the float. Using lots of precision to code irrational values like pi rarely has a downside. Further, float may be as precise as long double and they both may use 128-bit. Avoid assuming too much about FP characteristics. \$\endgroup\$ Sep 29, 2022 at 16:28
  • \$\begingroup\$ @chux-ReinstateMonica glibc 2.36 has # define M_PI 3.14159265358979323846 /* pi */. By your logic, there's no harm in extending that to 1,000 digits. So why, pray tell, have they not done that? \$\endgroup\$
    – Reinderien
    Sep 29, 2022 at 16:37
2
\$\begingroup\$

How about some math related feedback?

Much non-math issues already covered by others.

float vs. double

double is far often the better choice than float in C. Save float for select cases where space/speed is a important factor.

Consider changing all objects and math to double. For a stand-alone "calculator", such as this code, I'd even consider long double.

Use float functions

Since OP is using float objects, double trig functions incur a lot of up and down converting. Use float functions for float objects.

// float result = tan(degrees);
float result = tanf(degrees);

Better printing

"%.2f" is uninformative when values are small. About 1/2 of all the 232 float values will print a value of zero. Large values (from tangentInverse()) can print dozens of uninformative digits as output past 9 significant digits lack usefulness for common float.

Rather than a fixed-place format for floating-point values, use "%g" to transition to exponential notation as needed. Use appropriate precision driven from <float.h> and not a naked magic number 2.

// printf("Answer: %.2f",result);
printf("Answer: %.*g", FLT_DECIMAL_DIG, result);

Take advantage that 360.0° is exactly the right value unlike PI

A sub-problem of trig functions is argument reduction of large angle values to the primary range. High quality argument reduction of radian arguments is a hard_problem as π is irrational. Note that PI is rational and not the same value as π - it is just the closest choice. Fortunately, high quality argument reduction of degree arguments is easy. Use fmodf() or remquof() which are expected to perform exactly. Such pre-range reduction avoids problems like Sin and Cos give unexpected results for well-known angles.

float degreesToPrimaryRangeRadians( float degrees) {
    return fmodf(degrees, 360.0f) * (PI / 180);
}

Double math for a float task?

degrees * (PI / 180) is a double operation as PI is a double constant. The product is then down converted to float.

More sensible to use float math here with float constants.

#ifdef M_PI
  #define PI ((float)M_PI)
#else
  //                                                 v appended
  #define PI 3.14159265358979323846264338327950288420f
#endif // M_PI

Sometimes (float)some_double_constant != same_constant_as_a_float.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Your answer was unique and much appreciated. \$\endgroup\$
    – Harith
    Sep 29, 2022 at 17:51
1
\$\begingroup\$

Minimise use of non-standard libraries

#include <conio.h> prevents this program compiling here. But we shouldn't need any libraries other than the Standard Library for a simple program like this.

When I remove the include, the only thing that's undefined is getch() - I'm guessing we can use getchar() from <stdio.h> instead. That certainly seems to work, if we cast to unsigned. Don't forget to ensure that EOF is handled appropriately.

Use function prototypes

Instead of defining main() to accept any number of arguments, be specific that it is only to be called with no arguments - int main(void).

Avoid forward declarations

There's no need to write the declarations of all the functions twice - just define them before they are used (with main() last).

Declare internal functions static

Most of the functions should be defined with internal linkage (i.e. static). That makes no practical difference in this program that's all in a single translation unit, but becomes important with larger programs linked from multiple object files.

Don't ignore runtime errors

float degrees;
scanf("%f", &degrees);

degrees = degreesToRadians(degrees);

What happens if the user enters something that's not a valid floating-point number (e.g. π)? scanf() will return 0, but we treat that exactly the same as if it returns 1. Good code always assumes users are idiots, or even actively hostile:

if (scanf("%f", &degrees) != 1) {
    fputs("That's not a valid number!\n", stderr);
    return EXIT_FAILURE;
}

Enable more warnings

You say your compiler output mentioned only the passing of uninitialised op to getOperation(), which suggests you haven't enabled most other warnings. Here's the ones I get after removing the <conio.h> include (which halts compilation there):

gcc-12 -std=c17 -fPIC -gdwarf-4 -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wconversion  -Wstrict-prototypes -fanalyzer  -Wconversion      280016.c  -lm  -o 280016
280016.c:21:5: warning: function declaration isn’t a prototype [-Wstrict-prototypes]
   21 | int main()
      |     ^~~~
280016.c: In function ‘degreesToRadians’:
280016.c:73:20: warning: conversion from ‘double’ to ‘float’ may change value [-Wfloat-conversion]
   73 |     return degrees * (PI / 180);
      |            ~~~~~~~~^~~~~~~~~~~~
280016.c: In function ‘getOperation’:
280016.c:88:10: warning: conversion to ‘unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]
   88 |     op = getchar();
      |          ^~~~~~~
280016.c: In function ‘sine’:
280016.c:96:20: warning: conversion from ‘double’ to ‘float’ may change value [-Wfloat-conversion]
   96 |     float result = sin(degrees);
      |                    ^~~
280016.c: In function ‘cosine’:
280016.c:105:20: warning: conversion from ‘double’ to ‘float’ may change value [-Wfloat-conversion]
  105 |     float result = cos(degrees);
      |                    ^~~
280016.c: In function ‘tangent’:
280016.c:114:20: warning: conversion from ‘double’ to ‘float’ may change value [-Wfloat-conversion]
  114 |     float result = tan(degrees);
      |                    ^~~
280016.c: In function ‘sineInverse’:
280016.c:123:20: warning: conversion from ‘double’ to ‘float’ may change value [-Wfloat-conversion]
  123 |     float result = asin(degrees);
      |                    ^~~~
280016.c: In function ‘cosineInverse’:
280016.c:132:20: warning: conversion from ‘double’ to ‘float’ may change value [-Wfloat-conversion]
  132 |     float result = acos(degrees);
      |                    ^~~~
280016.c: In function ‘tangentInverse’:
280016.c:141:20: warning: conversion from ‘double’ to ‘float’ may change value [-Wfloat-conversion]
  141 |     float result = atan(degrees);
      |                    ^~~~
280016.c: In function ‘main’:
280016.c:26:10: warning: ‘op’ is used uninitialized [-Wuninitialized]
   26 |     op = getOperation(op);
      |          ^~~~~~~~~~~~~~~~
280016.c:24:18: note: ‘op’ was declared here
   24 |     unsigned int op;
      |                  ^~
280016.c:26:10: warning: use of uninitialized value ‘op’ [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
   26 |     op = getOperation(op);
      |          ^~~~~~~~~~~~~~~~
  ‘main’: events 1-2
    |
    |   24 |     unsigned int op;
    |      |                  ^~
    |      |                  |
    |      |                  (1) region created on stack here
    |   25 | 
    |   26 |     op = getOperation(op);
    |      |          ~~~~~~~~~~~~~~~~
    |      |          |
    |      |          (2) use of uninitialized value ‘op’ here
    |

Most of these can be fixed by ditching the use of float, and sticking with double throughout.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Disagree with "Avoid forward declarations" as OP's style nice fits with a progression to a .h file where such declarations belong. \$\endgroup\$ Sep 29, 2022 at 16:20
  • \$\begingroup\$ @Toby Speight I took away much from this review, thank you. \$\endgroup\$
    – Harith
    Sep 29, 2022 at 17:39
1
\$\begingroup\$

User input

Others have mentioned that conio.h is platform specific and outside of the C standard. That's not necessarily a problem per se if the program is not intended to be portable, but even in such a situation, you should be very careful when mixing stdio.h functions with other functions that operate on the same file handle.

In particular, you have this.

op = getch();
//...
scanf("%f", &degrees);

These function will only work as you expect if they are called in this exact order. If you swap them, it will break. This is because you're mixing "raw mode" operations with "canonical mode" (or "line mode" or "buffering mode") operations.

scanf() first attempts to fill the buffer associated with the FILE * (stdin here) until a newline character is typed. This newline is an actual character in the buffer. So if you were to reverse the input order and ask the user for the number first and then the operation, you'd have to call getch() twice to get past the newline character.

User interaction

You also mention that this program is intended to be part of a larger program. You may discover that combining programs becomes easier if you avoid interactive IO as much as possible. For a program like this, it's common to organize it along the lines of a Unix shell utility where it accepts arguments from the command line parameters including options (importantly -h to print help information).

If the program were to accept arguments this way, you could achieve a similar interactive usage by running the program twice.

$ trig -h
$ trig 2 5.7293483

The C library includes functions atoi() and atof() that make it easy to parse numbers from the argv[] strings.

There is also the POSIX getopt() library, implementations of which are also available for use in non-Unixy platforms. This functions makes it very easy to process the options (like -h) from the command line and leave the other arguments for you to process with your own functions.

\$\endgroup\$

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.