patterncppMinor
Naive password checker
Viewed 0 times
naivecheckerpassword
Problem
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.
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
#include
#include
#include
using namespace std;
// Extract the contents of a user file for processing.
vector get_user_list(ifstream& userfile)
{
string line;
vector userlist;
if (userfile.is_open())
{
while (getline(userfile, line))
userlist.push_back(line);
userfile.close();
}
else
cout tomatch, vector userlist)
{
string fuser, fpass;
bool match = false;
for (string uline : userlist)
{
for (size_t i = 0; i userlist = get_user_list(userfile);
vector input = { "", "" };
cout > input[0] >> input[1];
if (check_pass(input, userlist))
cout << "ACCESS GRANTED" << endl;
else
cout << "ACCESS DENIED" << endl;
return 0;
}Solution
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
The same thing applies when using a range-based for loop:
Or you could simply let C++ deduce the type for you, using
As @janos has already mentioned, using a
Your
Note also that you should not use
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& tomatch, const vector& 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 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& 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.Code Snippets
bool check_pass(const vector<string>& tomatch, const vector<string>& userlist)for (const string& uline : userlist)for (const auto& uline : userlist)auto place = uline.find(',');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));
}Context
StackExchange Code Review Q#93497, answer score: 4
Revisions (0)
No revisions yet.