Review of my code?

[bakerlandon]

hey I started learning programming about a month ago and i've been teaching myself in hopes that when i start college i will be above average. I wanted to make a simple calculator program to test myself so will you all give a review and maybe give me advice in how to make it better?
all replies are appreciated...thank you very much




#include <iostream>
#include <cmath>
//gives the number PI a value
#define PI 3.14159265

using namespace std;

//defines the 3 variables needed for the operations
long double x;
long double y;
int choice;

//void functions are called in if user needs them

void add()
{
cout << "\nPlease enter a number to add\n";
cin >> y;
//makes the result equal the previous result plus what you want to add to it
x = x + y;
//tells user the new base of operations (result)
cout << "\n\n" << x << "\n\n";
}

void sub()
{
cout << "\nPlease enter a number for the subtrahend\n";
cin >> y;
x = x - y;
cout << "\n\n" << x << "\n\n";
}

void mult()
{
cout << "\nPlease enter a number to multiply\n";
cin >> y;
x = x * y;
cout << "\n\n" << x << "\n\n";
}

void div()
{
cout << "\nPlease enter a number for the divisor\n";
cin >> y;
//doesn't allow zero to be used in denominator
if (y==0)
{
do
{
cout << "\nYou are not allowed to divide by zero, input a new number\n";
cin >> y;
}
while (y == 0);
}
x = x / y;
cout << "\n\n" << x << "\n\n";
}

void exp()
{
cout << "\nPlease enter a number for the power\n";
cin >> y;
//makes the number for y the exponent of x
x = pow(x,y);
cout << "\n\n" << x << "\n\n";
}

void trig()
{
//gives choices for which triogonometric function needs to be used
cout << "\nPlease choose the trigonometric function you wish to use\n";
cout << "1. sin\n2. cos\n3. tan\n4. inverse sin\n5. inverse cos\n6. inverse tan\n7. inverse tan with two numbers\n8. back\n";
cin >> choice;
switch (choice)
{
case 1:
x = sin (x * PI / 180);
cout << "\n\n" << x << "\n\n";
break;
case 2:
x = cos (x * PI / 180);
cout << "\n\n" << x << "\n\n";
break;
case 3:
x = tan (x * PI / 180);
cout << "\n\n" << x << "\n\n";
break;
case 4:
x = asin (x) * 180 / PI;
cout << "\n\n" << x << "\n\n";
break;
case 5:
x = acos (x) * 180 / PI;
cout << "\n\n" << x << "\n\n";
break;
case 6:
x = atan (x) * 180 / PI;
cout << "\n\n" << x << "\n\n";
break;
case 7:
cout << "\nPlease enter a second number\n";
cin >> y;
x = atan2 (x,y) * 180 / PI;
cout << "\n\n" << x << "\n\n";
case 8:
break;
}
}

void log()
{
//gives two types of logarithmic functions to be used
cout << "\nWhich do you want to find?\n1. natural logarithm\n2.log base 10\n3. back\n";
cin >> choice;
switch (choice)
{
case 1:
x = log (x);
cout << "\n\n" << x << "\n\n";
case 2:
x = log10 (x);
cout << "\n\n" << x << "\n\n";
case 3:
break;
}
}

void num()
{
//allows the user to assign a new number for the base of operations
cout << "\nPlease enter a new number to start the operations with\n";
cin >> x;
cout << "\n\n";
}

void help()
{
//gives minor help (i didn't put much effort in the help function, sorry
cout << "Welcome to the Calculator help function. This calculator allows you to do\n";
cout << "simple math through the use of multiple functions. The first number that\n";
cout << "you enter is the base of operations, and after you choose the operation\n";
cout << "desired, the base of operations is the result of the most recent operation.\n";
cout << "When choosing which operation you desire, always use just the number that\n";
cout << "identifies the operator you want. In the trigonometric and logarithmic\n";
cout << "functions, the same rule applies. Thank you for using this program, it is\n";
cout << "the first program that I have created.\n\n";
}

//the main function that is always used by the program
int main()
{
cout << "Welcome to the calculator! Please enter a number to begin.\n";
//x is the base of operations
cin >> x;
//causes a loop to allow multiple operations without reopening the program
do
{
cout << "\nSelect the corresponding operator!\n";
//tells user the choices he has for operations
cout << "1. addition\n2. subtraction\n3. multiplication\n4. division\n5. exponents\n6. trigonometric functions\n7. logarithmic functions\n8. choose a new number for operation\n9. help\n0. exit\n\n";
//accepts the users input to call the function needed
cin >> choice;
switch (choice)
{
case 1:
//calls upon the addition function to be used (see void add)
add();
break;
case 2:
//calls upon the subtraction function to be used (see void sub)
sub();
break;
case 3:
//calls upon the multiplication function to be used (see void mult)
mult();
break;
case 4:
//calls upon the division function to be used (see void div)
div();
break;
case 5:
//calls upon the exponent function to be used (see void exp)
exp();
break;
case 6:
//calls upon the trigonometric function list to be used (see void trig)
trig();
break;
case 7:
//calls upon the logarithmic function list to be used (see void log)
log();
break;
case 8:
//see void num for code in choosing a new base number
num();
break;
case 9:
//see void help
help();
break;
case 0:
//exits the program
cout << "thank you for using the calculator";
break;
}
}
while (choice != 0);

return 0;
}

[TheIdeasMan]

bakerlandon ,

First, can you please use code tags - it's the <> button at the bottom or right of the screen under format. It makes the code much easier to read.

//gives the number PI a value
#define PI 3.14159265 

You can just use M_PI which is defined in the maths header.

//defines the 3 variables needed for the operations
long double x;
long double y;

Is there any reason to use long doubles? ordinary doubles should be fine.

Normally, you declare your functions before main, and put the definitions after main.

Consider having a ShowMenu and ProcessMenu functions.

Be careful comparing doubles, as they often cannot be represented exactly. This code fails:

double a = 0.1;    //cant be represented exactly
if (10 * a == 1.0)   //always fails 

Check out the use of numeric_limits<double>::epsilon()

Also, consider this basic design for a calculator:

// Get a number 
//Get an Operator
//Get a number
//calc answer 

I would have these things as functions. The GetNumber function should have error checking in it, so that you can fix bad input. The GetOperator function should call a IsOperator function.

If you do it that way you won't need a big menu to select each operation, as a switch can do the right operation based on what the operator is.

consider writing a DegToRadians function, so you don't have to do this everywhere:

(x * PI / 180)

Also RadiansToDeg function.

You also need range checking on some of the trig functions, and log functions.

If you are going to use a do loop, put the while part on the same line as the closing brace, so it doesn't cause all kinds of confusion because it looks like a while loop with a null statement.

Think about using a bool variable to help decide whether to continue the loop.

Well, there's some stuff to think about - Good Luck !! I look forward to helping more.

[zb0S216C]

bakerlandon wrote:

#define PI 3.14159265

I recommend swapping this with a float/double constant. Doing so will provide the reader with more information regarding the literal than just a floating-point literal. Also, declaring literal-initialised constants is efficient since the compiler will not allocate memory for the constant. For instance:

float PI(3.14F); // The compiler will allocate memory for 'PI'
float const PI(3.14F); // The compiler will not allocate memory for 'PI' 

bakerlandon wrote:

long double x;
long double y;
int choice;

I strongly recommend avoiding global variables/objects. Either place them inside a namespace, or, declare them local to main().

bakerlandon wrote:

void add()

I recommend passing whatever you want adding as arguments. By implementing add() using global variables, add() becomes dependant on something that may or may not be removed in the future. "Remove the support beams, and the building falls."

_Wazzak

[TheIdeasMan]

Framework wrote:

I recommend swapping this with a float/double constant

I see what you are saying about memory allocation - Just wondering why the OP can't use M_PI from the math header? Is that not one of the purposes of the math header, to have all kinds of constants defined already, so one doesn't have define them (one way or another) again?

Is the memory allocation going to matter for the OP for the program?

[zb0S216C]

TheIdeasMan wrote:

"Just wondering why the OP can't use M_PI from the math header?"

To be perfectly fair, I didn't know that M_PI existed until you said it did. I'm not really interested in the mathematical libraries, so I wouldn't know :)

TheIdeasMan wrote:

"Is the memory allocation going to matter for the OP for the program?"

If I review a piece of code, I'll consider efficiency, readability, portability, et cetera.

_Wazzak

[TheIdeasMan]

@Framework

Alright no worries.

[pbd2012]

I hope this is the right place to post c++ newbie problems. my code is follow this statement.


#include <iostream>
using namespace std;

using std ::cin;

using std ::cout;

using std ::endl;

void problem1()
{
char letters[] = { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M',
'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z' };

cout << "PRINTING CONTENTS OF ARRAY." <<endl;

cout << "==================================" <<endl;


for (int i = 0; i < sizeof(letters); ++i)
{
cout << letters << " ";
}

cout << endl;
}

void problem2()
{
char letters[] = { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M',
'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z' };

cout << "This is the title to your Program related to the alphabet." << endl;

cout << endl;

cout << "Select the number that coincides with the alphabet." << endl;

cout << "For example, the number 7 should display the letter G." << endl;

cout << endl;

cout << "Enter a number between 1 and 26: ";

int number;

cin >> number;

cout << endl;

cout << "The number you selected: " << number << endl;

if ((number > 0) && (number <= sizeof(letters)))

{

cout << "The letter related to this number: " << letters[number-1] << endl;

}

else

{

cout << "Sorry, the number must be between 1 and 26." << endl;

}

}

void problem3()
{

char letters[] = { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M',

'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z' };

bool odd = false;

cout << "PRINTING CONTENTS OF ARRAY and adding x to every other element" << endl;

for (int i = 0; i < sizeof(letters); ++i)

{

if ( odd )

{

char letters = 'x';

}

odd = !odd;

cout << letters << " ";

}

cout << endl;
}

void problem4(bool even)

{

char letters[] = { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M',

'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z' };

char* prefixes[] = { "Even", "Odd" };

cout << "PRINTING CONTENTS OF ARRAY USING THE MOD Option" << endl;

cout << "=====================================================" << endl;

int start;

if (even)

{

start = 0;

}

else

{

start = 1;

}

for (int i = start; i < sizeof(letters); i += 2)

{
cout << prefixes[start] << " Numbered Element = " << i;

cout << " Contents of Element within Array is = " << letters << endl;

}

}

int main()

{

problem1();

cout << endl;

problem2();

cout << endl;

problem3();

cout << endl;

problem4(true);

cout << endl;

return 0;

}
// end program...



The program runs but with strange characters after the letters. Problem3 does not PRINT CONTENTS OF ARRAY and adding x to every other element .
I have changed the for loop to (int i = 1; i < sizeof(letters); ++i)
still no errors but no x is being added to every other element.

[TheIdeasMan]

pbd2012 (1)

You have hijacked this thread with your own and unrelated question - the proper thing to do is start your own thread.

And use code tags - the <> button on the right, or at the bottom for a new thread.