So i started learning C++ a little while ago and have written a program for the basic functions of trigonometry and wanted some feedback on my code. If anyone is interested in having a look you can find it here: https://github.com/shadowclocker/trigcalc If there is something i'm doing wrong please let me know.
My first question is, why do you have all these global variables? They don't appear to be needed in the global scope. Best practice is to keep variables out of the global scope unless absolutely necessary. I would move those definitions to inside your main function at least.
Next problem, your menus make use of multiple if statements, when I think you're intending to use if {...} else if {...} else if {...} else {...}. As it stands now every time through when c2 isn't "cancel" that else statement is going to trigger every time.
Just a couple things after a quick glance and I'm a bit tired and rusty. It's been maybe 5 years since I've actually done anything in the language and that was back when I was still in college.
Thanks for the feedback i will be sure to correct my mistakes
For starters, you should consider using a namespace to cut down on your keystrokes and clean up your code a bit. All you would have to do is insert the following line of code before line 20:
using namespace std;This will remove the need to continually type "std::" before those prefixed methods.
I have broken down some specific corrections/tips by their line numbers below:
line 5: The variable cCom is never used and should therefore be removed
line 22: M-PI should be M_PI
@xipher I second that the use of global variables should be avoided, unless the situation absolutely demands it. Move those variables inside of main().
@xipher is also right with structuring you input comparisons. Since you ask a question to the user that has a binary responce (yes ot no), then you should have your checks structured more like the following:
using namespace std;
string QuestionPrompt = "Sample question (y/n): ";
string UserInput = "";
cout << QuestionPrompt << endl;
cin >> UserInput;
if(UserInput=="y"){
// Do the 'yes' stuff...
} else if(UserInput=="n"){
// Do the 'no' stuff...
} else {
// Do the invalid input stuff...
}When you structure your input checks like this, you also remove the need for the variables c2-c4, as c1 can be used to store all of the user responses.
Lastly, you may wish to change some of the grammar in your output. While it is not crucial to how well your code may run, correct grammar certainly helps eliminate user confusion.
I threw together a quick edit for you, mainly to see how using namespaces can clear some of the clutter from function calls like std::cout and std::cin. http://hastebin.com/ruzozemibi.cpp
Thanks for the corrections. As you mentioned adding the line using namespace std; would cut down on keystrokes however in the past it has caused problems as i added more code.
Instead of namespace you can also accept just the ones your using, i.e.
using std::cin;
using std::cout;
using std::string;
//etcThat way you only include what you need.
You probably should watch these two lectures:
Having single 120 line function with that many IFs is a really gad style.
lol i made one of these in c#!
Polluting namespaces are baaaad.
1 Like
There are several things you can do to improve this code. First of all, all your variables are global. You need to make them local. Secondly, a lot of these variables can be reused when you are only using them once.
For instance, you have:
std::cin >> c1;
if (c1 == "trig")
{
std::cout << "PLease chose from sin, cos, tan" << std::endl;
std::cin >> c2;
if (c2 == "sin")You can simply reuse c1 instead of c2. This brings me to my next point: you need to choose better variable names. Prefer "input' over "c1" or "c2".
Here is another example:
std::cin >> angle1; //input angle
angle = anglecalc(angle1); //convert to radiansIt is easier to read if "angle1" were called "degrees" and "angle" were called "radians".
My next point is that you ought to look for code duplication and try to minimize it. I messed around and got your trig function down to:
if (input == "sin")
{
handleTrig(input, "opposite", "hypotenuse", angle, trigResult);
return;
}
else if (input == "cos")
{
handleTrig(input, "adjacent", "hypotenuse", angle, trigResult);
return;
}
else
{
handleTrig(input, "opposite", "adjacent", angle, trigResult);
return;
}Where handleTrig is:
void handleTrig(std::string funcName, std::string topName, std::string bottomName, double angle, double trigResult){
char response{};
std::cout << "Is the " << topName << " known?\nPlease enter y/n: ";
std::cin >> response;
switch (response)
{
case 'y':
handleTop(funcName, angle, trigResult, topName);
break;
case 'n':
handleBottom(funcName, angle, trigResult, bottomName);
break;
default:
std::cout << "Did not recognize input.\nReturning to main...\n";
return;
}I also agree with velhojoel, don't use "using namespace std;" flippantly. Namespaces are there for a reason and it's a good habit to get into to avoid importing whole namespaces into the Global namespace. It is okay to use the "using" statements inside of functions though.
Anyways, I hoped this helped out. Happy coding!
I would try to split these into more functions or classes with methods.
Having all the "main loop" and input logic in one big block is usually pretty bad as you have big scopes. I tend to favour smaller scopes as there is less chances of modifying a variable which is uses elsewhere.
On the same note, having each block of code in its own function/method helps identifying what it actually does. In your case, it's not really an issue because it's a small program but trust me I've seen huge blocks of bad written code and it gets really tricky somethimes.
As others stated, the global variables are just not suited for this situation and they should be declared in smaller scopes. For example, the variable "sin" is only used when you want to calculate the sin so it whould be declared locally there.
The only cases where you really want to have variable declared in larger scopes than needed is in the context of high performance. If you are in a tight loop, then it is a good idea to allocate the memory (create the variable) outside of the loop.
For the "usings", I think that Eden gave a fair compromise (specifying each types that you will actually use).
I know that you are starting but I think it would be wise to know a bit how the preprocessor works (the "#define" statement in your code).
A define statement will make the compiler "copy the code" where the name of the macro is used. So in your example (assuming that your macro has the right name of "M_PI" and it's not defined yet), this line: angle = (angle * M_PI) / 180; //angle * pi
would actually be compiled as angle = (angle * (4.0 * std::atan2(1.0, 1.0))) / 180; //angle * pi
which works but is far from optimal. To do something like this, it's always better to calculate the result in advance for constants (simply give 3.1415... directly). The reason is that with the current code, the computer will have to calculate a trigonometric function each time just to get the value of something that never changes.
So if you have a loop like this for (int i=0; i</* A very large number*/ ; ++i)
{
double result = /* some other calculations*/ + M_PI;
// Do something with result
}
the performance will be way worse.
Your variable names could really be better. Try to give them a name which represents more their context. The variable "loopm" for example could be called "exited" and you have the loop like this "while (!exited)". This way, if you end up with multiple loops, it's easier to know which one does what.
As other already stated, it would be nice to separate the UI (cin and cout) and the logic in the code. Also, these "looping programs which ask for user input" is not really standard and it would be better to use the arguments received in the console directly and have to call the program with the command like:myProgram.exe sin -h 0.7 2.3
or myProgram.exe sin -o 0.7 2.3
I hope this helps!