3
\$\begingroup\$

I'm looking for advice on how I could improve this. I'm using VS2015 to compile so whatever language features are supported in there is fair game!

Problem: Create a program that reads a username and a password from the user and grants access if that username and password exists in code. Bonus points for storing the possible usernames in a file like so:

user1,pass

user2,pass

...

Encryption is not really a concern; I'm just getting the hang of things. Points I'd like to improve:

  • I'm using a placeholder vector for the user input, initialized with empty strings. It feels like there would be a more elegant way to get user input without using a holding variable?

  • I imagine there's a more elegant way to go through each line of the file too, again, lots of placeholder variables for data I could process in place I think.

#include <iostream>
#include <fstream>
#include <vector>
#include <string>

using namespace std;

// Extract the contents of a user file for processing.
vector<string> get_user_list(ifstream& userfile)
{
    string line;
    vector<string> userlist;

    if (userfile.is_open())
    {
        while (getline(userfile, line))
            userlist.push_back(line);

        userfile.close();
    }
    else
        cout << "Couldn't read from file.";

    return userlist;
}

// Go through each line of the user list and extract a comma
// seperated username and pass; match against the supplied
// username and pass.
bool check_pass(vector<string> tomatch, vector<string> userlist)
{
    string fuser, fpass;
    bool match = false;

    for (string uline : userlist)
    {
        for (size_t i = 0; i < uline.size(); i++)
        {
            if (uline[i] == ',')
            {
                fuser = uline.substr(0, i);
                fpass = uline.substr(i + 1, string::npos);
                break;
            }
        }

        if (tomatch[0] == fuser && tomatch[1] == fpass) 
        {
            match = true;
            break;
        }
    }

    return match;
}

int main()
{
    ifstream userfile("data/users.txt");
    vector<string> userlist = get_user_list(userfile);
    vector<string> input = { "", "" };

    cout << "Enter your username followed by your pass: ";
    cin >> input[0] >> input[1];

    if (check_pass(input, userlist))
        cout << "ACCESS GRANTED" << endl;
    else
        cout << "ACCESS DENIED" << endl;

    return 0;
}
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Whenever you are passing arguments in C++, you must be a bit more careful. Since C++ is a default by-value language, passing in a std::vector the way you are doing it will create a full copy of the vector each time you call the function. This is expensive, and not needed. Instead, you should be passing by const &:

bool check_pass(const vector<string>& tomatch, const vector<string>& userlist)

The same thing applies when using a range-based for loop:

for (const string& uline : userlist)

Or you could simply let C++ deduce the type for you, using auto:

for (const auto& uline : userlist)

string has some methods that can help make your code a bit shorter and clearer; such as find:

auto place = uline.find(',');

As @janos has already mentioned, using a map (or unordered_map) would be a better fit here. This could look something like:

std::unordered_map<std::string, std::string> user_to_pwd;
while (getline(userfile, line)) {
    const auto index = line.find(',');
    user_to_pwd.insert(
        std::make_pair(line.substr(0, index), line.substr(index + 1));    
}

Your check_pass function could then simply take three parameters: the map, and the username and password. This then becomes a (fast) lookup:

bool check_pass(
    const std::unordered_map<std::string, std::string>& user_pwds, 
    const std::string& user, 
    const std::string& password)
{
    auto it = user_pwds.find(user);
    if(it == user_pwds.end()) {
        return false;
    }
    return it->second == password;
}

Note also that you should not use using namespace std; up the top. This pulls everything from the std namespace into the global namespace, which can cause all sorts of issues. For small programs it probably won't, but it's best not to get into the habit of using it anyway.

\$\endgroup\$
2
\$\begingroup\$

The get_user_list function doesn't really return a user list: it returns a list of username and password pairs. Then, to check if a user is in the list, you have to iterate over the pairs, and extract the username part and the password part to perform the check.

It would be more natural and efficient to parse the file into a std::map<std::string, std::string>, mapping usernames to passwords. You could do this in a load_users function. I prefer "load" for functions that perform I/O, and "get" for functions that are "getters", and simply return objects that are ready to use. This function would replace your original get_user_list.

Once you have a map, it becomes trivial and very efficient to check if a username is contained in the map, and if the password stored for that username matches the input.

I'm using a placeholder vector for the user input

You shouldn't have. You should use separate username and password, simple string variables for each. There's no reason to use a vector for this.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.