Also thought that it should be ordered like this:
1. Open file
2. Ask for input (choose element)
3. Calc and save
4. Close file
5. Ask if calc another
6. Close or open again |
Okay, but look at this snippet:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20
|
std::ifstream myfile(".\\db\\elements.txt");
do
{
if(myfile.is_open())
{
...
myfile.close();
}
else
{
std::cerr << "Unable to open file\n"; //this happens regardles of line121 command
break;
}
std::cout << "Calculate another ? y/n \n";
std::cin >> again;
again = toupper(again);
} while(again == 'Y');
| |
You only open the file once before the loop. So after the first loop you won't be able to access the file.
Also if the file is not too large it would probably be better to read your file into memory and then do your processing from memory rather than trying to read the file over and over. Remember that reading a file is at least 1000 times slower than reading from memory, and that opening and closing the file is also slow and very error prone.
@gunnerfunner
1. suggest you use class instead of struct, putting all the functions into the class as class methods and hiding the member data as private (something that struct would not allow) |
A structure can do everything a class can do, the only difference between the two is the default access specifiers. A class defaults to private access where a struct defaults to public access.
2. you should put the minimum necessary #includes into the class header file and here none is necessary, all standard header files are moved to the implementation file and the main() program |
This is a very good point. Never rely on some "compiler magic" to include required header files. Always include the required header files for every file, and only include required files.
3. you don't need both cmath and math.h header files, only cmath is retained |
Another good point. These two header files could be different, in C++ use the C++ standard headers like <cmath> instead of the C standard headers like <math.h>.
4. implementation and main() are separated in keeping with the general principle of separating implementation from interface |
Just separating the class implementation from main() doesn't necessarily follow the principle of separating the implementation from the interface (as in User Interface) although it is a start. You can do this separation by using small functions that do as little as possible.
1. we declare a class Elements with the data members as given in your struct Element |
Again whether using a class or a struct makes no difference. If you always explicitly use the access specifiers a struct and a class can be used identically.
From here on when I refer to to a class I mean either a class or a struct that is explicitly using the access specifiers.
2. as mentioned in #2 of general background, all non-member functions are now class methods and the class also has a constructor, getters and setters |
This is were I have to strongly disagree! A class doesn't have to, nor should it, contain the entire class interface. A function should not be a member function unless it requires access to private class member functions.
Since you have setter/getter functions for all of your variables, a possible bad practice by the way (
1.more on this later), your functions should be non-class functions since they are using these getters/setters to access those private variables (
2.will talk about variable naming later as well).
1. As you setup this class you really don't need any of those setter functions because the only constructor you have sets the values of all the variables. I have mixed emotions about having that complicated constructor, on the one hand it removes the need for all those setters, a possible good thing, but on the other having all of those parameters are error prone. It will be very easy to transpose the numbers. It also makes using a container to hold this class more difficult because you can only insert items using this multiple parameter constructor. It may be a better idea to have a simple constructor and then have a "setter" function that can set multiple values at one time, perhaps doing some error checking/input validation at the same time. But really you should strive to limit the number of "setter" functions to the absolute minimum to help preserve encapsulation. Now to the "getter" functions, IMO you should strive to eliminate as many of these "getters" as possible, especially "getters" that do nothing more than return an internal value. In this class there is probably no reason to return the "raw" values held in the internal variables since you're probably only interested in the values calculated from the functions. To do this get rid of all the "getters" and then your calculation functions are a good candidate for being class member functions.
double Elements::calcAlphaForYield()
{ // By the way you may want to consider more parentheses because of the interspersed addition, or simplify the calculation by using intermediate variables.
return 0.249 * pow(m_M2 / m_M1, 0.56) + 0.0035 * pow(m_M2 / m_M1, 1.5);
}
2. Now to the variable naming. While I'm not a believer in having either a prefix or suffix on member variables this is really a personal preference that I can live with or without. However you (and the OP) should really be using meaningful variable names that help document what the above calculation is working with. Also those "magic numbers" (0.249, 0.56, etc.) should really be named constants, IMO, this will make understanding the calculation easier.
2. I wanted to make all the non-constructor class methods const but the compiler was refusing to do this with error messages on the lines of:
[code]error: passing 'const Elements' as 'this' argument of 'double Elements::get_Z1()' discards qualifiers [-fpermissive][code] |
This is really a const correctness issue, all of your getters should be const qualified.
std::string get_name() const {return m_name;}
And note that there is no need for that inline qualifier. When you define the function within the class definition you aromatically have inlined the function.
It also looks like you may have some member functions that should be private instead of public.
1 2 3
|
double Elements::calcEthForYield(){
return (m_Us() * 6.7) / calcgammaForYield(); // No need for the this-> it's implicit.
}
| |
If that calcgammaForYield() function is only used inside other member functions it should probably be private. And again that "magic number" should probably be a named constant.