Searching and comparing a record

Question

I am still a beginner in C++ and I am always curious about good/best coding methods.

Let's say I have a program which allows a users to edit the salary of a employee.

1. The system will prompt the user which to key in a employee's name first.

2. The system will then check whether or not the username exist.

3. If the username existed, the system will then allow the user to change the employee's salary.

The salary and the name of the person is stored in a text file.

employeeInfo.txt (formatted by name and salary)

john 1000
mary 2000
bob 3000

user.h

#ifndef user_user_h
#define user_user_h
#include <iostream>

class  user  {

public:

    user(std::string userName,std::string salary);

    std::string getUserName();
    std::string getSalary();

    void setUserName(std::string userName);
    void setSalary(std::string salary);


private:
    std::string userName,salary;

};
#endif

user.cpp

#include "user.h"
#include <iostream>
#include <string>
using namespace std;

user::user(string userName,string salary)  {

    setUserName(userName);
    setSalary(salary);


};

string user::getUserName()    {
    return userName;
}
string user::getSalary()  {
    return salary;
}

void user::setUserName(std::string userName) {
    this->userName = userName;
}

void user::setSalary(std::string salary) {
    this->salary = salary;
}

main.cpp

#include "user.h"
#include <iostream>
#include <string>
#include <fstream>
#include <sstream>
#include <vector>
#include <stdio.h>
#include <string.h>
using namespace std;


int main(){
    vector<user> userDetails;
    string line;
    string userName;
    string salary;

    ifstream readFile("employeeInfo.txt");

    while(getline(readFile,line))   {
        stringstream iss(line);
        iss >> userName >> salary;

        //consturctor
        user employeeDetails(userName,
                             salary
                             );
        userDetails.push_back(employeeDetails);

    }
    readFile.close();

    string name;
    cout << "Please enter a user name\n";
    cin >> name;

    for (int i =0; i<userDetails.size(); i++) {
        //search if there's a match
        if (userDetails[i].getUserName() == name) {
            string newSalary;

            cout << "Please enter a new salary" << endl;
            cin >> newSalary;
            userDetails[i].setSalary(newSalary);
        }
    }
    //display to see if the salary gets updated
    for (int i=0; i<userDetails.size(); i++) {
        cout << userDetails[i].getSalary << "\n";
    }
}

I am not really sure if my code (shown below) is the worst method to search for a record in a vector and match it against the input of the user. I would like to know is there any way to improve my code. I'd also like to gather some tips and ideas from you all to determine if this is the worst method to search for a record in a vector.

//search for existing username
for (int i =0; i<userDetails.size(); i++) {
            if (userDetails[i].getUserName() == name) {
                //do stuff
            }
        }

Answer 1

Your question is surprisingly hard to answer, largely because you ask one question in your words, but a different question in your code. Your written question is whether looking up a value by sequentially scanning a vector is the best or worst way to do it. And the answer to that question is nuanced. I'll talk more about this in a bit.

However your code shows a full scenario in which you read a file, look up and edit your single user, and display the full vector's contents. In this fuller context, the performance questions of the sequential scan almost definitely do not matter, as reading and displaying the whole vector will cost more time than you could save by having a smarter look-up. A more typical program might let you perform multiple edits, and then the time it took to look up each person might be more important. So that's another improvement to consider.

Scanning a Vector

Scanning a vector with a hand-rolled for loop is not generally considered the best way to do things. It's often much better to find an algorithm that does what you want, and use it. In this case we don't know anything particularly useful about the contents of your vector, so the best bet is going to be find or find_if. To use find, you'll have to overload operator== in something, and pass it around; to use find_if you can instead create a helper function or overload operator(), or in C++11 you can pass a lambda function. Let's examine the middle option:

struct user_by_name {
    user_by_name(string name) : name(name) {}
    bool operator()(user& user) { return user.getUserName() == name; }
    string name;
};

: : :

vector<user>::iterator iterFound = std::find_if(userDetails.begin(), userDetails.end(), user_by_name(name));
if (iterFound != userDetails.end())
{
    : : :
}

The nice thing about this approach is you can create ways to find users by other criteria, such as a user_by_salary, and substitute it in with almost no code change. The downside is that without making other changes, you'll never get better than "linear" performance - on average you'll always have to look through half of the items in your vector to find the one you need.

Scanning faster

If there's a natural ordering for the items in your vector, you have two main options. Both of them require implementing an operator<, and thus do change your user struct's usage paterns. And both of them scan faster by being able to skip past some items while finding the one you want.

• You can keep the items in the vector, but sort them by this ordering. This allows you to use an algorithm such as lower_bound to find them in a fraction of the time.
• You can store the items in a different data structure such as a std::map which makes similar use of the ordering to give you the same performance assistance that lower_bound does on the sorted vector.

Choosing between these cases depends on how the data will be used. Again, with just a single edit in your main program, this is all overkill. But if you are going to have a long-running multi-edit scenario, especially one with thousands of employees being looked up by name and updated, it might be worth examining these options.

Other options

If you were storing a realistic amount of data about each employee, had a realistic number of employees, and had a lot of tasks you wanted to perform on them, chances are you'd find storing their information in a database would make a lot more sense. Databases solve a lot of problems for you; not only do they support fast look-ups, they handle persistence of the data (loading and saving it as necessary) and, for the right scenarios, save you a lot of time and effort. Obviously for learning how to do some of the C++ programming that you show here, using a database may help you less (although learning how to use a database isn't a bad skill either).

Other comments

Finally I wanted to end this review with a list of more generic comments about your actual code. A lot of these things are details you don't need to care about yet, or that may have occurred just in trying to post your code here, but they may help you form better habits as you advance your C++ programming skills.

• using namespace std is frowned upon. It's utter anathema in a header file (which you didn't do), but it's also a risk in your cpp file. You're better off either adding the std:: prefixes, or adding using std::string; using std::vector; etc. However it's unlikely to matter in code like you show here.
• Your use of whitespace is inconsistent. You don't always put spaces after commas, and sometimes use newlines instead. In some places you use more blank lines that I would consider helpful. Also not going to really matter in code this short.
• Your property accessors (getUserName, getSalary) should be const as they should not visibly modify the object, and probably should return a const string& instead of a mutable copy.
• Your header should include <string> instead of <iostream>, and user.cpp can similarly drop its incldue of <iostream>; you don't appear to use anything from iostream in there. Your main.cpp similarly has a lot of unused includes, but I would expect it's more of a testing file so won't harp on it.
• Your search loop will find multiple matches. If you have multiple employees named bruce, your code will stop at each of them and ask for a new salary. If this is not intentional, you can use break; to avoid this. Note that most of my commentary above assumes you will want to only update a single employee at a time.
• If there's no match, the program directly displays the output without any explanatory messages. That seems a little surprising to me, but perhaps it's intentional.

There's not a lot of self-documentation in your code. You'll note the last two bullets I talk about what's intentional, and in particular that I'm uncertain what you meant for your code to do. Finding the right balance between variable names, function names, and comments will help resolve that in the future. This can start as simply as adding the description you gave in your post as a comment in your main.cpp.

But all that said, I think you're starting off on the right foot. You show some solid understanding, and you also show interest in refining your skills. Congratulations on a good first post here!

Answer 2

I just have a little to add to @MichaelUrman's review.

Keep as much code out of main() as practical. It will force you to write better structured code.

int main() {
    std::vector<user> userDatabase = loadEmployees("employeeInfo.txt");

    std::string name;
    std::cout << "Please enter a user name" << std::endl;
    std::cin >> name;

    changeSalary(name);
    display(userDatabase);
}

Storing the salary as a string is probably a bad idea. A long would be more appropriate. (If you need more precision, don't use a float or double, but store the number of cents as a long instead.)