Map not properly identifing elements

AterVulpes (60)
The purpose of this thing is to allow the user to type in a name of an element and the program will search the database for said element and use the elements constants for certain calculations.
Sometime ago, with your help, the following (below) solution to a problem was reached. For certain reasons, i did not fully check it for possible errors.
As i now again have some time for C++, i went back to it and the program does not work as intended.
When started it prints the first string (element name) in .\\db\\elements.txt". It should not print that. Then, after either choosing the "suggested" element or another it asks to calc again, printing the next name (a do-while loop problem elaborated upon further in the text).
If the selected name isn't the same as the "suggested" name, then the calculation gives 0, which is incorrect. If the suggested name is used, it's still wrong. Al gives the correct answer, but the rest seem to be using only it's input (1 1 1...).

And the do-while loop seems to be broken by the file not closing, even though on line 121, myfile.close(); is used. Its' closing down cannot be stopped by std::cin.clear(); std::cin.get(); combo.

The header file contains many files, because this is just a part of a training program.

Test contents of elements.txt

Fe 3 3 3 3 3 3 3 3
Al 1 1 1 1 1 1 1 1
Cu 2 2 2 2 2 2 2 2


Header code
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
 
#ifndef Y_H_H_INCLUDED
#define Y_H_H_INCLUDED

#include <iostream>
#include <math.h>
#include <cstdlib>
#include <cmath>
#include <iomanip>
#include <fstream>
#include <ostream>
#include <string>
#include <map>

//math
const double pi = 4.0 * atan(1.0); //pi
const double goldrat = (1 + sqrt(5))/2; //golden ratio
const double EulerConstant = std::exp(1.0); //euler constant

//physics
const double elementalcharge = 1.6021766208*pow(10,-19); //charge of electron

struct Element
{
    std::string name;
    double E;
    int M1;
    int M2;
    double Z1;
    double Z2;
    double Us;
    double WZ2;
    double QZ2;
    double Eth;
};

double calcLindhardScreeningLengthForYield(const Element &tmm);
double calcEpsilonForYield(const Element &tmm);
double calcAlphaForYield(const Element &tmm);
double calcgammaForYield(const Element &tmm);
double calcGammaForYield(const Element &tmm);
double calcEthForYield(const Element &tmm);
double calcSnEForYield(const Element &tmm);

double calcYield(const Element &tmm);

#endif // Y_H_H_INCLUDED



Main code
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
 
  #include "Y_h.h"

double calcLindhardScreeningLengthForYield(const Element &tmm)
{
    double aL;

    aL = 0.4685*pow((pow(tmm.Z1,2/3)+pow(tmm.Z2,2/3)),-1/2);

    return aL;
}

double calcEpsilonForYield(const Element &tmm) //reduced energy
{
    double aL = calcLindhardScreeningLengthForYield(tmm);
    double epsilon;

    epsilon = tmm.E*(tmm.M2/(tmm.M1+tmm.M2))*(aL/(tmm.Z1*tmm.Z2*pow(elementalcharge,2)));

    return epsilon;
}

double calcAlphaForYield(const Element &tmm) //alpha with asterisk (a*)
{
    double aa;

    aa = 0.249*pow(tmm.M2/tmm.M1,0.56)+0.0035*pow(tmm.M2/tmm.M1,1.5);

    return aa;
}

double calcgammaForYield(const Element &tmm) //energy transfer factor in the elastic collision
{
    double gamma;

    gamma = (4*tmm.M1*tmm.M2)/pow(tmm.M1+tmm.M2,2);

    return gamma;
}

double calcGammaForYield(const Element &tmm)
{
    double Gamma;

    Gamma = (tmm.WZ2)/pow((1+tmm.M1/7),3);

    return Gamma;
}

double calcEthForYield(const Element &tmm)
{
    double gamma = calcgammaForYield(tmm);
    double Eth;

    Eth = (tmm.Us*6.7)/gamma;

    return Eth;
}

double calcSnEForYield(const Element &tmm) //Thomas – Fermi potential
{
    double epsilon = calcEpsilonForYield(tmm);
    double SnE;

    SnE = (3.441*sqrt(epsilon)*(std::log(epsilon+2.718)))/((1+6.355*sqrt(epsilon))+(epsilon*(6.882*sqrt(epsilon)-1.708)));

    return SnE;
}

double calcYield(const Element &tmm) //sputtering yield
{
    double aL = calcLindhardScreeningLengthForYield(tmm);;
    double Eth = calcEthForYield(tmm);
    double aa = calcAlphaForYield(tmm);
    double epsilon = calcEpsilonForYield(tmm);
    double Gamma = calcGammaForYield(tmm);
    double SnE = calcSnEForYield(tmm);
    double Y;

    Y = 0.042*(((tmm.QZ2*aa*(tmm.M2/tmm.M1))/tmm.Us)*(SnE/(1+Gamma*aL*pow(epsilon,0.3))))*1+pow(sqrt(tmm.E/Eth),2); //last part seems wrong, doesn't it ? ;)

    return Y;
}

int main()
{
    char again;

    Element tmm; //tempmaterialmap
    std::map<std::string, Element> Properties_Map;
    std::map<std::string, Element>::iterator it;
    std::string key = " ";

    std::ifstream myfile (".\\db\\elements.txt");
    do
    {
        if (myfile.is_open())
        {
            while (myfile >>tmm.name >> tmm.E >>tmm.M1 >>tmm.M2 >>tmm.Z1 >>tmm.Z2 >>tmm.Us >>tmm.WZ2 >>tmm.QZ2)
            {
            Properties_Map.insert ( std::pair<std::string, Element>(tmm.name, tmm) );
            std::cout << tmm.name << '\n';
            if (!std::cin.fail())
                {
                std::cout << "Please enter abbreviated element name: ";
                std::cin >> key;
                it = Properties_Map.find( key );
                double Y = 0;

                if (it != Properties_Map.end())
                    Y = calcYield(tmm); //Y = it -> second.calcYield();
                    std::cout << "Y=" << Y << '\n';
                    std::ofstream out("Yield.txt", std::ios::app);
                    out << Y << std::endl;
                }
            else
                {
                std::cerr << "Parsing error!\n";
                break;
                }
            }
            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');
}
}


Example output for "wrong" choice (F is purposefully used, i need to add input validation for incorrect choice):

Fe
Please enter abbreviated element name: Al
Y=0
Al
Please enter abbreviated element name: Cu
Y=0
Cu
Please enter abbreviated element name: F
Y=0
Calculate another ? y/n



Example output for "suggested" choice (not 0, but only Al gives correct answer and the rest seem to use its input values):

Fe
Please enter abbreviated element name: Fe
Y=0.149254
Al
Please enter abbreviated element name: Al
Y=0.149254
Cu
Please enter abbreviated element name: Cu
Y=0.149254
Calculate another ? y/n


As this seems difficult, and as i have some job and nutjob related things to do, i will most likely be able to reply no sooner then tommorow.
Last edited on
Moschops (7244)
When started it prints the first string (element name) in .\\db\\elements.txt". It should not print that.


So what's this line of code for?
std::cout << tmm.name << '\n';
jlb (4973)
And the do-while loop seems to be broken by the file not closing, even though on line 121, myfile.close(); is used.

Why are you closing the file inside the loop?

Also your indentation needs some serious attention. Look at this snippet:
1
2
3
4
5
6
7
8
9
10
 
 if (!std::cin.fail())
                {
 ...
                if (it != Properties_Map.end())
                    Y = calcYield(tmm); //Y = it -> second.calcYield();
                    std::cout << "Y=" << Y << '\n';
                    std::ofstream out("Yield.txt", std::ios::app);
                    out << Y << std::endl;
                }
            else

By looking at your indentation it looks like you intend all the code after the first if statement to be part of the body of that if statement but in reality it looks like the following to the compiler:

1
2
3
4
5
6
7
8
9
10
11
 
                if(!std::cin.fail())
                {
...
                    if(it != Properties_Map.end())
                        Y = calcYield(tmm); //Y = it -> second.calcYield();

                    std::cout << "Y=" << Y << '\n';
                    std::ofstream out("Yield.txt", std::ios::app);
                    out << Y << std::endl;
                }
                else




gunnerfunner (2127)
First some GENERAL BACKGROUND before we move on to the actual code:

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)
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
3. you don't need both cmath and math.h header files, only cmath is retained
4. implementation and main() are separated in keeping with the general principle of separating implementation from interface
5. you don't need a map container to do what you're trying to do and hence removed
6. two separate versions of calcGammaforYield() are defined with the same signatures => not overloaded, so one of them has been commented out and you can decide which version to keep

Now moving on to the APPROACH:

1. we declare a class Elements with the data members as given in your struct Element
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
3. when the program is run it offers a menu 1. Search 2. Quit
4. if Search is chosen, user enters element symbol that is checked against the file, if element is not found non-existence is reported
5. if the input element is found in the file a stringstream object is initialised with the line that contains the element, the benefit of the stringstream is that it parses the string holding the element into the corresponding data members of an Elements object
6. the class constructor is then run to create an elements object and then the member methods of that object can be called

REMAINING ISSUES:

1. as you've mentioned yourself in your post, please double-check the calcYield() formula and then put the actual values of the data members in the file to check the formula, right now the output is garbage
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:
error: passing 'const Elements' as 'this' argument of 'double Elements::get_Z1()' discards qualifiers [-fpermissive]
I am hoping someone more experienced than me will be able to explain this. Having said all that, the code follows:

HEADER FILE 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
 
#ifndef element_h_
#define element_h_

class Elements{

    private:
        std::string m_name;
        double m_E;
        int m_M1;
        int m_M2;
        double m_Z1;
        double m_Z2;
        double m_Us;
        double m_WZ2;
        double m_QZ2;
        double m_Eth;
    public:
        Elements(std::string name, double E, int M1, int M2, double Z1, double Z2, double Us, double WZ2, double QZ2, double Eth);
        double calcLindhardScreeningLengthForYield();
        double calcEpsilonForYield();
        double calcAlphaForYield();
        double calcgammaForYield();
     //   double calcGammaForYield();
        double calcEthForYield();
        double calcSnEForYield();
        double calcYield();

        inline void set_name(std::string name){m_name =name;}
        inline void set_E(double E){m_E = E;}
        inline void set_M1(int M1) {m_M1 = M1;}
        inline void set_M2 (int M2) {m_M2 = M2;}
        inline void set_Z1 (double Z1) {m_Z1 = Z1;}
        inline void set_Z2 (double Z2) {m_Z2 = Z2;}
        inline void set_Us (double Us){m_Us = Us;}
        inline void set_WZ2 (double WZ2) {m_WZ2 = WZ2;}
        inline void set_QZ2 (double QZ2){m_QZ2 = QZ2;}
        inline void set_Eth (double Eth){m_Eth = Eth;}

        inline std::string get_name(){return m_name;}
        inline double get_E(){return m_E;}
        inline int get_M1(){ return m_M1;}
        inline int get_M2(){ return m_M2;}
        inline double get_Z1(){ return m_Z1;}
        inline double get_Z2(){ return m_Z2;}
        inline double get_Us() { return m_Us;}
        inline double get_WZ2() { return m_WZ2;}
        inline double get_QZ2(){ return m_QZ2;}
        inline double get_Eth(){ return m_Eth;}
};
#endif // element_h_

IMPLEMENTATION FILE 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
 
#include<iostream>
#include <cmath>
#include <string>
#include"Elements_header.h"
using namespace std;
//math
const double pi = 4.0 * atan(1.0); //pi
const double goldrat = (1 + sqrt(5))/2; //golden ratio
const double EulerConstant = exp(1.0); //euler constant
//physics
const double elementalcharge = 1.6021766208*pow(10,-19); //charge of electron

Elements::Elements(string name, double E, int M1, int M2, double Z1, double Z2, double Us, double WZ2, double QZ2, double Eth)
    :m_name(name), m_E(E), m_M1(M1), m_M2(M2), m_Z1(Z1), m_Z2(Z2), m_Us(Us), m_WZ2(WZ2), m_QZ2(QZ2), m_Eth(Eth){}

double Elements::calcLindhardScreeningLengthForYield(){

    return 0.4685*pow((pow(get_Z1(),2/3)+pow(get_Z2(),2/3)),-1/2);
}
double Elements::calcEpsilonForYield(){
    double a = (get_M2()/(get_M1() + get_M2()));
    double b = get_Z1() * get_Z2() * pow(elementalcharge,2);
    return get_E()*a*(this->calcLindhardScreeningLengthForYield()/b);
}
double Elements::calcAlphaForYield(){
    return 0.249*pow(get_M2()/get_M1(),0.56)+0.0035*pow(get_M2()/get_M1(),1.5);
}
double Elements::calcgammaForYield(){
    return (4*get_M1()*get_M2())/pow(get_M1()+get_M2(),2);
}
/*double Elements::calcGammaForYield(){
    return get_WZ2()/pow((1+get_M1()/7),3);
}*/
double Elements::calcEthForYield(){
    return (get_Us()*6.7)/(this->calcgammaForYield());
}
double Elements::calcSnEForYield(){
    double a = 3.441 * sqrt(this->calcEpsilonForYield());
    double b = (log(this->calcEpsilonForYield() + 2.718));
    double c = 1 + 6.355 * sqrt(this->calcEpsilonForYield());
    double d = (this -> calcEpsilonForYield()) * (6.882 * sqrt(this->calcEpsilonForYield() - 1.709));
    return (a + b) / (c + d);
}
double Elements::calcYield(){
    double a = (get_QZ2() * this -> calcAlphaForYield() * (get_M2()/get_M1()))/get_Us();
    double b = (this->calcgammaForYield())*(this->calcLindhardScreeningLengthForYield())*pow(this->calcEpsilonForYield(),0.3);
    double c = 1 + pow(sqrt(get_E()/this->calcEthForYield()),2);
    return 0.042 * a * (this->calcSnEForYield()/(1+b))*c;
}

MAIN() 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
 
#include <iostream>
#include <cmath>
#include<fstream>
#include<string>
#include<sstream>
#include"Elements_header.h"

using namespace std;

int main()
{
bool fQuit = false;
	while(!fQuit){
		cout<<"1. Search: \n2. Quit\n";
		int choice;
		cin>>choice;
		switch(choice){
			case 1: {
				string name, line, search;
				cout<<"Enter search string: \n";
				cin>>search;
				double E, Z1, Z2, Us, WZ2, QZ2, Eth;
				int M1, M2;
				int match = 0;
				ifstream File;
				File.open("F:\\elements.txt");
				if(File.is_open()){
					while(getline(File,line)){
						if(line.find(search)!=string::npos){
							match++;
							stringstream stream(line);
							stream>>name>>E>>M1>>M2>>Z1>>Z2>>Us>>WZ2>>QZ2>>Eth;
							Elements e(name, E, M1, M2, Z1, Z2, Us, WZ2, QZ2, Eth);
                                cout<<"Sputtering yield for element "<<e.get_name()<< " is "<<e.calcYield()<<endl;
							}
						}
					}
				if(match==0){
					cout<<"The given element doesn't exist\n";
				}
				File.close();
				break;
			}
			case 2:
				cout<<"Goodbye\n";
				fQuit = true;
				break;
			default:
				cout<<"Incorrect choice, try again\n";
				break;
		}
	}
}





AterVulpes (60)
@Moschops
That is a mistake on my side. I actually had to ctrl+f to find it, because it somehow hid from me.

@jlb
Well, crap. Need to "reindent" then, but i did think it was correct :(
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

@gunnerfunner
Holy screaming sheep shit! I am looking into everything you wrote. Thank you very much for granting me so much time and thought.

There are actually two different gamma functions (for clarity's sake probably should have called them smallGamma and bigGamma).

I have few questions, however. Why did you use inline in the class ? Isn't that done by default when a function is declared and defined within a class ?
And why were the equations changed is such a way ?
1
2
3
4
5
6
 
double Elements::calcEpsilonForYield() //reduced energy
{
    double a = (get_M2()/(get_M1() + get_M2()));
    double b = get_Z1() * get_Z2() * pow(elementalcharge,2);
    return get_E()*a*(this->calcLindhardScreeningLengthForYield()/b);
}


What i see here is correct, but why so ?


And i made slight changes to the thing. Moved includes into the header file (thoes should go there, right ?) and changed input validation so a character and not numerical input in the menu doesn't freak out the program. ANd changed file path, to mine.

Function to clear the buffor.
1
2
3
4
5
6
 
void badUser() //must have #include <limits>
{
    std::cin.clear();
    std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
    std::cout << "\nIncorrect input. \nTry again: \n";
}


Changed main.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
 
#include "Y_h.h"

int main()
{
bool fQuit;
    do
    {
    std::cout<<"1. Search\n2. Quit\n";
    int choice;
    std::cin>>choice;

    if ((std::cin.fail()) || (choice < 1) || (choice > 2))
        {
            badUser();
        }
        else
        {
            switch(choice)
            {
			case 1:
                        {
				std::string name, line, search;
				std::cout<<"Enter search string: \n";
				std::cin>>search;
				double E, Z1, Z2, Us, WZ2, QZ2, Eth;
				int M1, M2;
				int match = 0;
				std::ifstream file;
				file.open (".\\db\\elements.txt");
				if(file.is_open())
				{
					while(getline(file,line))
					{
						if(line.find(search)!=std::string::npos)
						{
							match++;
							std::stringstream stream(line);
							stream>>name>>E>>M1>>M2>>Z1>>Z2>>Us>>WZ2>>QZ2>>Eth;
							Elements e(name, E, M1, M2, Z1, Z2, Us, WZ2, QZ2, Eth);
                                std::cout << "Sputtering yield for element " << e.get_name() << " is " << e.calcYield() << std::endl;
							}
						}
					}
				if(match==0)
				{
					std::cout<<"The given element doesn't exist.\n";
				}
				file.close();
				break;
                        }
			case 2:
			{
				std::cout<<"Goodbye.\n";
                                fQuit = true;
				break;
                        }
             }
        }

    }
    while (fQuit == false);
}


Indentation above is not what i have in Code::Blocks. I try to fix it, but in some places there are numerous tabs out of nowhere and i have a hard time reformatting this by the eye.
Last edited on
gunnerfunner (2127)
1. Inline - you can remove them if you wish

2. Equations - broken up into smaller chunks to save my eyes

3. #includes in header files - you can search this topic yourself, here is a sample:

https://blog.knatten.org/2012/11/09/another-reason-to-avoid-includes-in-headers/
jlb (4973)
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.

Last edited on
gunnerfunner (2127)
@jlb: thank you for your post, I was hoping for such clear and well crafted comments when I posted. In reply:

1. yes, it would indeed be better to read the file into memory first and then process from memory; given the finite size of known elements I suspect such a file would not be too large but, in general, is there a ball-park file size number beyond which would make reading from memory inefficient?

2. I did some further reading on the struct v class detail after your post and it seems to me that the only thing a struct can do that a class cannot is interface with C; OP can decide if this will be a requirement or not

3. yes, we do indeed don't require the setters given the way the ctor has been defined and these can be dispensed with

4. the ctor may be complicated but since it reads directly from file chances of error in input is limited

5. if we were to get rid of the getters would you suggest turning the data members public, assuming we're still using class and not struct? This in turn would determine whether we need class methods or would non-member functions suffice

6. some of the calculations have indeed been simplified using intermediate variables, something the OP notes in his reply to my first post on this topic

7. re variable names, I've used the one's provided by OP with a m_ prefix for data members, s/he can change them as you suggest if s/he wants

8. re getters being const qualified which in turn would enable the calculation methods being const qualified since the latter incorporates the former - I can't thank you enough for pointing this out

9. re inline, I've already replied to OP on your lines

10. agreed, some member functions can be private as they're being used inside other member functions
jlb (4973)
in general, is there a ball-park file size number beyond which would make reading from memory inefficient?

The only real time would be if the file was too large to fit into memory. However even large files can often be processed in stages.
4. the ctor may be complicated but since it reads directly from file chances of error in input is limited

But since you're not doing much if any input validation this is an even bigger source of probable errors.
5. if we were to get rid of the getters would you suggest turning the data members public, assuming we're still using class and not struct? This in turn would determine whether we need class methods or would non-member functions suffice

No, leave the variables private if you use a clas.
6. some of the calculations have indeed been simplified using intermediate variables, something the OP notes in his reply to my first post on this topic

But when you "simplified" the functions you used horrible names for these intermediate values. You should have at least commented on what those horrible single letter names were all about.
9. re inline, I've already replied to OP on your lines

But your reply wasn't really answering the OP's question for clarification. You should have also acknowledged that the OP's assumption was correct.
Topic archived. No new replies allowed.