Vectors, Structs, and either not setting or retrieving string variable

  • (3 Pages)
  • +
  • 1
  • 2
  • 3

33 Replies - 1673 Views - Last Post: 25 April 2018 - 07:23 AM Rate Topic: -----

#16 SloanTheSloth   User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 14
  • Joined: 23-April 18

Re: Vectors, Structs, and either not setting or retrieving string variable

Posted 24 April 2018 - 06:35 AM

View Postsnoopy11, on 24 April 2018 - 04:40 AM, said:

Oh for goodness sake we are trying to help you...

Given the lack of information about the assignment I would approach the problem something like this....



Lol I really don't think you're trying to help
I didn't ask for my whole assignment to be written, which is exactly why I didn't give information about the assignment. I wanted help finding the main error, which was the fact I was calling inStream.eof when it wouldn't work yet.

I'm also confused as to why you would change the whole class to become "employee". For my assignment, I specifically made the class "Employee manager" for a reason.

Anyway, everyone else has been helpful but all you've done is patronize.
Was This Post Helpful? 0
  • +
  • -

#17 modi123_1   User is online

  • Suitor #2
  • member icon



Reputation: 14092
  • View blog
  • Posts: 56,460
  • Joined: 12-June 08

Re: Vectors, Structs, and either not setting or retrieving string variable

Posted 24 April 2018 - 06:49 AM

Everyone needs to chillax.
Was This Post Helpful? 0
  • +
  • -

#18 snoopy11   User is offline

  • Engineering ● Software
  • member icon

Reputation: 1467
  • View blog
  • Posts: 4,726
  • Joined: 20-March 10

Re: Vectors, Structs, and either not setting or retrieving string variable

Posted 24 April 2018 - 07:10 AM

View PostSloanTheSloth, on 24 April 2018 - 01:35 PM, said:

I'm also confused as to why you would change the whole class to become "employee". For my assignment, I specifically made the class "Employee manager" for a reason.

Anyway, everyone else has been helpful but all you've done is patronize.


If you had bothered to look,

there is a Employee_Manager class which inherits from the base class Employee.......
Was This Post Helpful? 0
  • +
  • -

#19 baavgai   User is offline

  • Dreaming Coder
  • member icon


Reputation: 7181
  • View blog
  • Posts: 14,969
  • Joined: 16-October 07

Re: Vectors, Structs, and either not setting or retrieving string variable

Posted 24 April 2018 - 07:45 AM

View Postsnoopy11, on 24 April 2018 - 09:10 AM, said:

there is a Employee_Manager class which inherits from the base class Employee.......


Dude, I hate that!

I did play around with this a little. So:
class EmployeeManager {
public:
    EmployeeManager(const std::string &); // Constructor
    EmployeeManager(); //Default constructor
    bool verifyLogin(const std::string &username, const std::string &password) const; //Verifies login information
    bool changePassword(const std::string &username, const std::string &oldPass, const std::string &newPass); //change password
    bool isAdmin(const std::string &username) const; //Verify if user is admin
    bool addEmployee(const std::string &username, const std::string &password, bool isAdmin); //add Employee
    bool deleteEmployee(const std::string &username);
    void displayEmployees(); //Displays employee list. Might overload << instead.
    // why?
    // friend void test_loadEmployees(); //Test function
private:
    //Struct to hold employee information.
    struct Employee {
        std::string username, password;
        bool admin;
        // counstructor would be nice
        Employee(const std::string &username, const std::string &password, bool isAdmin);
        Employee(); // allow for an empty
        // consider the redunancy in you manager
        bool changePassword(const std::string &oldPass, const std::string &newPass);
        // hmm, you seem to have this going on
        bool read(std::istream &);
    };
    // never    typedef Employee* EmployeePtr;
    std::vector<Employee> employeeList;
    std::string employeeFile; //File of employees
    // I'd prefer this in the constructor
    // void loadEmployees(); //Loads employees into employeeList
    void save(); //save employeeList into employee file
    // no
    // static const string DEFAULT_FILE;
};


using namespace std;

//Loads employees from employeeFile. If no employees, create default admin with password 0000.
//If fails to open, display error message.
EmployeeManager::EmployeeManager(const std::string &fn) : employeeFile(fn) {
    ifstream in(fn.c_str());
    //If cannot find or open, display error.
    if (in.fail()) {
        cerr << "File open failed. Please use staff.txt\n";
        // you shouldn't really do anything else after this
        // either return or, like here, make another branch
    } else {
        while (!in.eof()) {
            Employee emp;
            if (emp.read(in)) {
                employeeList.push_back(emp);
            }
        }
        // we're done with the file, don't forget this
        in.close();

        if (employeeList.empty()) {
            employeeList.push_back(Employee("admin", "0000", true));
        }
    }
}



I'm not sure how I feel about having the save internal, though I understand it. I'm concerned by the friend tester: you should reasonably be able to test without it. You might consider a debug mode for the manager, if it seems like you need it. Honestly, I just riddle anything I'm working on with debug code as part of the process and then rip it out as I go.

There is, of course, more than one way to skin a cat. Programmers all too easily can get stuck on the "right" way to do it... for them. :P There are, of course, bad ways to do things, and better ways, but domain of right moves more into subjective. Just look at what's being thrown at you, make sure you understand the reasoning, and then do what you will.

Should I appear rude, it's only because I know I'm right. :rolleyes: ;)
Was This Post Helpful? 1
  • +
  • -

#20 snoopy11   User is offline

  • Engineering ● Software
  • member icon

Reputation: 1467
  • View blog
  • Posts: 4,726
  • Joined: 20-March 10

Re: Vectors, Structs, and either not setting or retrieving string variable

Posted 24 April 2018 - 09:25 AM

Well... I don't hate that...

I hate using structs in C++....however yours is an improvement... on the OP's... so I will let you off :P
Was This Post Helpful? 0
  • +
  • -

#21 jimblumberg   User is offline

  • member icon

Reputation: 5487
  • View blog
  • Posts: 17,063
  • Joined: 25-December 09

Re: Vectors, Structs, and either not setting or retrieving string variable

Posted 24 April 2018 - 09:27 AM

To be honest I also dislike the following:

//Loads employees from employeeFile. If no employees, create default admin with password 0000.
//If fails to open, display error message.
EmployeeManager::EmployeeManager(const std::string &fn) : employeeFile(fn) {
    ifstream in(fn.c_str()); // You should be using a modern C++ compiler using one of the current standards (C++14 or C++17) so no need for the c_str().
...
    if (in.fail()) {
        cerr << "File open failed. Please use staff.txt\n";
        // you shouldn't really do anything else after this
        // either return or, like here, make another branch  // You're in a constructor so if this fails you probably should throw an exception!
    } else {
...
        // we're done with the file, don't forget this
        in.close();  // No need for this, the file will properly close when the function returns!
,,,
    }
}




I also recommend that instead of passing a file name you consider opening the stream in the calling function before you create an instance of the class. Opening a file, especially for reading, is one of the biggest failure modes of this type of a program. So, IMO, it is better to handle file opening problems as soon as possible.

Jim
Was This Post Helpful? 2
  • +
  • -

#22 baavgai   User is offline

  • Dreaming Coder
  • member icon


Reputation: 7181
  • View blog
  • Posts: 14,969
  • Joined: 16-October 07

Re: Vectors, Structs, and either not setting or retrieving string variable

Posted 24 April 2018 - 10:05 AM

@jimblumberg: Well, I was trying to stay fairly close to the original. Though I do appreciate the comments.

I totally agree with the no c_str: personal bad habit.

I'm dubious about fail in the constructor: reasonably zero entries is a sign of failure: I've had bad C++ exception behavior in the past. The OP is also supporting odd fail behavior that I was trying to stick to. I do like the idea of just dealing with a passed stream, though.

A for letting the destructor do file cleanup: I would argue that if you're doing more processing in a block and you don't need the file anymore, manual closing seems appropriate.
Was This Post Helpful? 1
  • +
  • -

#23 jimblumberg   User is offline

  • member icon

Reputation: 5487
  • View blog
  • Posts: 17,063
  • Joined: 25-December 09

Re: Vectors, Structs, and either not setting or retrieving string variable

Posted 24 April 2018 - 10:59 AM

Quote

A for letting the destructor do file cleanup: I would argue that if you're doing more processing in a block and you don't need the file anymore, manual closing seems appropriate.

Then this sounds like it should either be in it's own block or better yet in another function.

I understand that you're trying to stick with the original code as much as possible, however IMO that original code has many things that are a bad to poor practice and IMO a lot of the OP's functions are doing too much, again IMO.

Quote

I'm dubious about fail in the constructor: reasonably zero entries is a sign of failure: I've had bad C++ exception behavior in the past.

Yes, I agree having a constructor that can "fail" is a big problem which is why I suggested moving the file opening, along with the required state tests, elsewhere. Also in this program there seems too many different failures that could be happening in the various constructors.

I try to limit the amount of work I do in constructors to limit the amount of potential failure points because there is no reliable way of signaling a failure in a constructor other than throwing an exception which usually means terminating the program.


Jim
Was This Post Helpful? 2
  • +
  • -

#24 snoopy11   User is offline

  • Engineering ● Software
  • member icon

Reputation: 1467
  • View blog
  • Posts: 4,726
  • Joined: 20-March 10

Re: Vectors, Structs, and either not setting or retrieving string variable

Posted 24 April 2018 - 11:37 AM

View Postjimblumberg, on 24 April 2018 - 05:59 PM, said:

I understand that you're trying to stick with the original code as much as possible, however IMO that original code has many things that are a bad to poor practice and IMO a lot of the OP's functions are doing too much, again IMO.


I totally agree... with the above... it could be done better.
Was This Post Helpful? 0
  • +
  • -

#25 SloanTheSloth   User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 14
  • Joined: 23-April 18

Re: Vectors, Structs, and either not setting or retrieving string variable

Posted 24 April 2018 - 12:00 PM

View Postsnoopy11, on 24 April 2018 - 07:10 AM, said:

there is a Employee_Manager class which inherits from the base class Employee.......



That makes no sense.

The employee manager is a class that holds a list of employees and the different operations that can occur to this list of employees.

The employeManager is not an employee, so it shouldn't be a child of employee I purposely encapsulated the employee struct inside the employee manager because no other class needs direct access to the employee struct, just the ability to modify the employees in a list.
Was This Post Helpful? 0
  • +
  • -

#26 SloanTheSloth   User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 14
  • Joined: 23-April 18

Re: Vectors, Structs, and either not setting or retrieving string variable

Posted 24 April 2018 - 12:19 PM

View Postbaavgai, on 24 April 2018 - 07:45 AM, said:

I'm concerned by the friend tester: you should reasonably be able to test without it. You might consider a debug mode for the manager, if it seems like you need it. Honestly, I just riddle anything I'm working on with debug code as part of the process and then rip it out as I go.

There is, of course, more than one way to skin a cat. Programmers all too easily can get stuck on the "right" way to do it... for them. :P/> There are, of course, bad ways to do things, and better ways, but domain of right moves more into subjective. Just look at what's being thrown at you, make sure you understand the reasoning, and then do what you will.



Yeah, I know that the friend tester isn't good practice. Honestly, I was just being lazy. I tend to overtest for these school projects in general. Most of my test functions could probably be replaced with one or two bigger ones, that aren't a part of the class. But, I'm lazy and use friend classes. I usually comment them out or just get rid of them completely before turning it in.

The second part is something I definitely agree with. I love when people give me helpful tips, or ways to do things better. Like someone else telling me the pointer were unnecessary/actually causing a memory leak. That's informative and helpful. I don't, however, like when someone just comes in and says "Of course its not working!" and doesn't give me feedback to where the exact problem lies or that kind of thing.
Was This Post Helpful? 0
  • +
  • -

#27 SloanTheSloth   User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 14
  • Joined: 23-April 18

Re: Vectors, Structs, and either not setting or retrieving string variable

Posted 24 April 2018 - 12:30 PM

Sorry for the spam. I should read more before posting.

View Postjimblumberg, on 24 April 2018 - 09:27 AM, said:

I also recommend that instead of passing a file name you consider opening the stream in the calling function before you create an instance of the class. Opening a file, especially for reading, is one of the biggest failure modes of this type of a program. So, IMO, it is better to handle file opening problems as soon as possible.

Jim


That's actually what originally happens. I have the option to pass in a file name, but also have a default filename it will use if none is mentioned. I really added it so that a) my professor can easily change the file to use other test files, and B) the system class (which is the "backbone" of this project) can also change the filename if for some reason staff.txt wasn't an acceptable file.

So basically, in the constructors (which I don't think I added in my OP because they were irrelevant to the problem at hand) it immediately calls (loadEmployees) which loads the employees into the employeemanager's list. This is the only time loadEmployees is called; it's only in its own method because I didn't want to have to type it twice into two methods. Also, in case you didn't get the chance to see, this is the updated version someone helped me with, which I like much better and would love if you gave your opinion about it.

void EmployeeManager::loadEmployees() {
	ifstream inStream;
	inStream.open((char*)employeeFile.c_str());
	//If cannot find or open, display error.
	if (inStream.fail()) {
		cout << "\tFile open failed. Please use staff.txt\n";
	}
	else {
		//Retrieve employee info
		//Note: Info is stored as username on the first line, password on the next, and then
		//0 or 1 to signify if the user is an admin or not.
		int a;
		string u, p;
		//This statement puts the username, password, and admin status into the placeholder variables.
		//It also checks that it is true/false, or rather, it has not yet reached the end of file.
		while (inStream >> u >> p >> a) {
			employeeList.push_back(new Employee(u, p, a));
		}
	}
	inStream.close(); //Close file
	//If nothing in file, create default admin
	if (employeeList.empty()) {
		employeeList.push_back(new Employee("admin", "0000", 1));
	}
}


saveEmployees is another method, that is called after an employee is added through addEmployee, or deleted through deleteEmployee. It opens the file, saves the new employee list, and then closes the file back.

In the code you had quoted, it had "No need for this, the file will close when the function returns". Is this really true? I'm doing this assignment for a c++ class, and we were taught to always call .close(). It's one of those things that if I don't have it in there, I'll lose points.
Was This Post Helpful? 0
  • +
  • -

#28 jimblumberg   User is offline

  • member icon

Reputation: 5487
  • View blog
  • Posts: 17,063
  • Joined: 25-December 09

Re: Vectors, Structs, and either not setting or retrieving string variable

Posted 24 April 2018 - 01:22 PM

What's this non-sense?
    ifstream inStream;
    inStream.open((char*)employeeFile.c_str());


All you need is:

    ifstream inStream(employeeFile);


Note the lack of that c_str() call and that horrible unnecessary C style cast. If you "must" cast then use the proper C++ style cast.

Quote

So basically, in the constructors (which I don't think I added in my OP because they were irrelevant to the problem at hand) it immediately calls (loadEmployees) which loads the employees into the employeemanager's list.

Like I said you are using poor to bad practices. In this case this function may as well be integral to the constructor since any failure can not be reported to the user. If the opening of the file fails then what does that mean for the rest of the program? My guess would be you would have bad data because you just ignored the problem.

Quote

That's actually what originally happens. I have the option to pass in a file name, but also have a default filename it will use if none is mentioned.

No, you're passing a string into the function, not an open input stream. It is the opening of the stream that usually the source of problems. I'm recommending that you pass an open stream (that has been checked to insure the file opened correctly) not the file name of the file.

Also look at your error message:
   if (inStream.fail()) {
	        cout << "\tFile open failed. Please use staff.txt\n";


Where are you redirecting the program so the user can tell the program to use staff.txt? What happens if staff.txt fails to open as well.

Again look at this snippet:
        while (inStream >> u >> p >> a) {
	            employeeList.push_back(new Employee(u, p, a));



Why are you using a pointer instead of a "normal" instance. Something like:

        while (inStream >> u >> p >> a) {
	            employeeList.push_back({u, p, a});


By the way another bad practice is the use of those single letter variable names, what do they mean?

Now:
	inStream.close(); //Close file
	//If nothing in file, create default admin
	if (employeeList.empty()) {
		employeeList.push_back(new Employee("admin", "0000", 1));
	}


Again with the new! More like:

	// inStream.close(); // This not necessary! Let the @#$$% destructor do it's job!
	//If nothing in file, create default admin
	if (employeeList.empty()) {
		employeeList.push_back({"admin", "0000", true});
	}


Putting it all together.
void EmployeeManager::loadEmployees() {
	ifstream inStream(employeeFile);
	//If cannot find or open, display error.
	if (!inStream) {
		cout << "\tFile open failed. Please use staff.txt\n"; // You really should report the "name" of the file that failed to open.
		// throw() something to stop the execution of the program to fix the problem.
	}
	else {
		//Retrieve employee info
		//Note: Info is stored as username on the first line, password on the next, and then
		//0 or 1 to signify if the user is an admin or not.
		bool isAdmin;
		string userName, password;
		//This statement puts the username, password, and admin status into the placeholder variables.
		//It also checks that it is true/false, or rather, it has not yet reached the end of file.
		while (inStream >> userName >> password >> isAdmin) {
			employeeList.push_back({userName, password, admin});
		}
	}
	//If nothing in file, create default admin
	if (employeeList.empty()) {
		employeeList.push_back({"admin", "0000", true});
	}
}


Note this is just cleaning up your function, not tested (you may need a constructor?).

And note that I think you have too much buried inside your class. Remember in C++ not everything needs to (nor should should everything) be part of a class.


Jim

This post has been edited by jimblumberg: 24 April 2018 - 01:24 PM

Was This Post Helpful? 1
  • +
  • -

#29 SloanTheSloth   User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 14
  • Joined: 23-April 18

Re: Vectors, Structs, and either not setting or retrieving string variable

Posted 24 April 2018 - 02:22 PM

View Postjimblumberg, on 24 April 2018 - 01:22 PM, said:

What's this non-sense?
    ifstream inStream;
    inStream.open((char*)employeeFile.c_str());


All you need is:

    ifstream inStream(employeeFile);


Note the lack of that c_str() call and that horrible unnecessary C style cast. If you "must" cast then use the proper C++ style cast.


So this is actually something our professor told us to do. Just doing what you said returns an error. I'm not sure if its the compiler I'm on (I just use Bash on windows 10), but if I don't cast to the c_str, it won't work. The syntax that you see in my code is something he provided us to open files. Good to know that if I continue with c++ that it isn't necessary/is crappy code.


Quote

No, you're passing a string into the function, not an open input stream. It is the opening of the stream that usually the source of problems. I'm recommending that you pass an open stream (that has been checked to insure the file opened correctly) not the file name of the file.


So, (bear with me as I probably repeat exactly what you say), you mean that in the constructor, instead of doing the file name, the program that is calling/creating an EmployeeManager should create/open an input stream and then pass that stream over? I guess I never thought of doing that.

As for the error/message outputed if it doesn't open properly... This is another requirement of the assignment. I guess they don't think fitting in exception throwing and that sort of thing is important for this course, or we are learning it the last week of class lol. He specifies that if the file does not open, it should display a message stating so, but continue on by creating the default admin (even though any new employees made won't be able to be saved).


I actually just fixed the pointer issue after replying to you lol. When I was having my original problem in the post, I changed a bunch of shit to pointers because I didn't know if maybe somehow I needed pointers. Someone corrected me. I thought I went back and changed it, but obviously didn't. It's all fixed now to normal instances of Employee. I also plan on going back to fix the variable names. Originally I had them as like tempUsername, or maybe it was userIn, something like that, but when another person helped me, they switched to the u >> a >> p thing, and honestly I just copied and pasted it test it and hadn't yet gone back to fix the names how I like them.


is the inStream.fail() thing just useless? Or rather, is there any point in typing it that way? I see in your code you just have if (!inStream).




Thank you for all your help! Especially with small things like good programming style. I'm still learning (and obviously have alot more to learn) but I want to learn good habits ASAP as opposed to fixing my bad habits later on. I also know I usually overthink shit and put wayy too much into my classes as well.
Was This Post Helpful? 0
  • +
  • -

#30 jimblumberg   User is offline

  • member icon

Reputation: 5487
  • View blog
  • Posts: 17,063
  • Joined: 25-December 09

Re: Vectors, Structs, and either not setting or retrieving string variable

Posted 24 April 2018 - 03:05 PM

Quote

So, (bear with me as I probably repeat exactly what you say), you mean that in the constructor, instead of doing the file name, the program that is calling/creating an EmployeeManager should create/open an input stream and then pass that stream over? I guess I never thought of doing that.

Yes that is exactly what I'm saying. Pass a open stream that has already been checked to insure it is indeed open.


Quote

So this is actually something our professor told us to do. Just doing what you said returns an error. I'm not sure if its the compiler I'm on (I just use Bash on windows 10), but if I don't cast to the c_str, it won't work.

Then you're either doing something wrong or the compiler you're using is a pre-standard piece of crap. Even if you use the c_str() function you should not need to use that horrible cast, even in C++98.

Quote

I'm not sure if its the compiler I'm on (I just use Bash on windows 10)

How exactly are you compiling your code? If you're using bash you're probably using g++ which means that you also need to add switches to control the error messages generated and the version of the C++ standard.

Quote

As for the error/message outputed if it doesn't open properly... This is another requirement of the assignment.

What is part of the requirements, outputting an error message or ignoring the problem?

Quote

is the inStream.fail() thing just useless? Or rather, is there any point in typing it that way? I see in your code you just have if (!inStream).

No, it's not useless, it's just not checking every error condition. It is only checking for the "fail" condition. The other method reports the status of all the error flags. If you don't like the !inStream syntax then you should use if(!inStream.good()).

Quote

I also know I usually overthink shit and put wayy too much into my classes as well.

It seems that way, part of the way too much is that embedded structure, having it outside the class would enable you to use it outside the class if you so desired. But when inside the class you can only ever use the structure inside the class.

Jim
Was This Post Helpful? 0
  • +
  • -

  • (3 Pages)
  • +
  • 1
  • 2
  • 3