Passing vectors into function by reference vs by value in C++

Question

I have been writing a small project just to expand on my knowledge of c++ and I ran into an issue. When I was accepting a username I wanted to check if it was already taken. If that specific username was taken I re-printed the question to the user. It works fine the first loop, but after that it will accept anything even if it does exist inside the users Vector.

     bool verify(char * a, vector<User> b){
 14         for(int i = 0; i < b.size(); i++){
 15                 if(strcmp(a, b[i].getUsername()) == 0){
 16                         return false;
 17                 }
 18         }
 19         return true;
 20 }
 21 
 22 int main(){
 23 
 24         vector<User> users;
 25 
 26         User us1((char *)"foo", (char *)"bar");
 27         users.push_back(us1);
 28 
 29         
 30         do{
 31                 cout << "Enter Username: ";
 32                 scanf(" %s", username);
 33                 
 34                 
 35         } while(!verify(username, users));
 36         
 37         return 0;
 38 }   

However, if my function verify instead takes in a vector & b it works fine. Can someone explain why this is happening?

User.cpp

    User:: User(char * userName, char * passWord){
  9 
 10         this->userName = strdup(userName);
 11         this->passWord = strdup(passWord);
 12 
 13 }
 14 
 15 User:: ~User(){
 16 
 17         delete userName;
 18         delete passWord;
 19 
 20 }
 21 
 22 void User::getMessage(){
 23 
 24         cout << message << endl;
 25 }
 26 
 27 char * User:: getUsername(){
 28 
 29         return userName;
 30 }
 31 
 32 char * User :: getPassword(){
 33 
 34         return passWord;
 35 }
 36 
 37 void User:: printUser(){
 38 
 39         cout << "User Information" << endl;
 40         cout << "Username: "<< userName << endl;
 41         cout << "Password: "<< passWord << endl;
 42         cout << "Messages: "<< ((message == NULL) ? "User has no messages\n" : "User has 1 message\n");
 43 
 44 }

Answer 1

It appears to work fine, after I make the minimal changes so it actually compiles. I just used std::string to store a string instead of guessing at your User class.

#include <vector>
#include <string>
#include <iostream>
#include <cstdio>
#include <cstring>

struct User {
    std::string name, game;
    User(const char* nam, const char* gam) : name(nam), game(gam) {}
    const char* getUsername() const {
        return name.c_str();
    }
};

bool verify(char * a, std::vector<User> b) {
    for (int i = 0; i < b.size(); i++) {
        if (std::strcmp(a, b[i].getUsername()) == 0) {
            return false;
        }
    }
    return true;
}

int main(){    
    std::vector<User> users;

    User us1("foo", "bar");
    users.push_back(us1);

    char username[100];

    do {
        std::cout << "Enter Username: ";
        std::scanf(" %50s", username);
    } while(!verify(username, users));

    return 0;
}

When I run this on ideone, inputting "foo" three times and then "Greg", it keeps prompting after each "foo" and then accepts "Greg".

Since substituting another class for your unseen "User" class makes it work, the problem must be in that class, probably in your handling of the pointers that are passed to its constructor.

Some notes:

• There should be no need to cast string literals "foo" and "bar" to (char*); they are already char*. If you are doing this to get rid of const-ness, don't: they probably reside in read-only memory.

• If you have C++11, the loop in verify can be replaced by a range-for:

  for (auto u : b) {
      if (strcmp(a, u.getUsername()) == 0)
          return false;
  }
  

• In fact, you can eliminate the whole function verify and use std::any_of(users.begin(), users.end(), [username](const User& u){ return strcmp(u.getUsername(), username) == 0; }), which returns true if username is already in the vector users.

So a shorter, equivalent complete program is

#include <vector>
#include <string>
#include <iostream>
#include <algorithm>

struct User {
    std::string name, game;
    User(const char* nam, const char* gam) : name(nam), game(gam) {}
    const std::string& getUsername() const {
        return name;
    }
};

int main(){
    std::vector<User> users;
    users.push_back({"foo","bar"});
    std::string username;

    do {
        std::cout << "Enter Username: ";
        std::cin >> username;
    } while(std::any_of(users.begin(), users.end(),
            [username](const auto& u){ return u.getUsername() == username; }));

    return 0;
}

(on ideone)

Answer 2

All discussion of using std::string aside, the base problem in OP's code is the lack of a user defined copy constructor for the User class. The default one will just copy the values in the userName and passWord fields, causing both vectors (the one in main and the one created for the verify function) to point to the same allocated memory address. When verify returns, that memory is delete, leaving the Users in the vector in main with dangling pointers (pointing to freed memory).

Using the reference instead avoids this deletion and keeps the original vector intact.

Which is one reason why you shouldn't use raw pointers in code these days.

Answer 3

I'd highly recommend using C++ functionality over C stuff wherever possible, it's usually more readable and less error prone.

Now there are multiple problems in your code and @AlanStrokes' comment was right, you're main problem is not correctly dealing with the rule of three.

Your User class does dynamical allocation but doesn't define a copyconstructor and assignment operator. For a simple snippet showing the problem, have a look at the snippet here. It only copies the addresses, not the actual data that's pointed to, so once the first copy gets deleted all others are invalid.

You also have other problems, strdup is non-portable, it's not part of the C standard. It's part of the POSIX standard so most likely only available on systems implementing that standard. Also it's a C function that allocates memory with malloc, you should delete the pointer it returns the free from C, not the delete from C++.

Also there is a reason that string literals are const char[] in C++, in fact in C they are char[] but you aren't allowed to edit them so it's practically const as well. That's because the compiler is allowed to put string literals into the read only location of the executable. So instead of casting string literals to char * make your function correctly take a const char *.

---

All that said dealing with those pointer things is annoying since C++ makes it way easier with std::string and I'd recommend using that instead:

class User {
  private:
    std::string userName;
    std::string password;

    User(const std::string &userName, const std::string & passWord);
    std::string getUsername();
    std::string getPassword();
    void printUser();
};

User::User(const std::string & userName, const std::string & passWord) {
  this->userName = userName;
  this->passWord = passWord;
}

std::string User::getUsername() {
  return userName;
}

std::string User::getPassword() {
  return passWord;
}

// etc...

Here C++ automatically handles all the copying and deleting logic and you don't have to deal with annoying pointer stuff.