C++ array of pointers returns the same value for each element

Question

I wrote a simplified version of the code I'm working on to illustrate a problem I'm having. I think something is wrong in main() that causes the showUsers() method to output the same Login/Password combo for every element (always the last one added).

Here is my code:

#include <iostream>

using namespace std;

//=============================================================================
class AccountInfo {
private:
    char* _username;
    char* _password;
public:
    AccountInfo();
    AccountInfo(char* username, char* password);
    ~AccountInfo();
    void setUsername(char* username);
    void setPassword(char* password);
    char* getUsername();
    char* getPassword();
    friend ostream& operator<<(ostream& out, AccountInfo& x) {
        out << "Login: " << x.getUsername() << endl 
            << "Password: " << x.getPassword() << endl;
        return out;
    }
};

AccountInfo::AccountInfo() {
    _username = "";
    _password = "";
}

AccountInfo::AccountInfo(char* username, char* password) {
    _username = username;
    _password = password;
}

void AccountInfo::setUsername(char* username) {
    _username = username;
}

void AccountInfo::setPassword(char* password) {
    _password = password;
}

char* AccountInfo::getUsername() {
    return _username;
}

char* AccountInfo::getPassword() {
    return _password;
}

//=============================================================================
class UsersDB {
private:
    int _size;
    AccountInfo* _accounts[200];
public:
    UsersDB();
    ~UsersDB();
    int getSize();
    void addUser(AccountInfo* newUser);
    void showUsers();
};

UsersDB::UsersDB() {
    _size = 0;
}

UsersDB::~UsersDB() {
    delete[] _accounts;
}

int UsersDB::getSize() {
    return _size;
}

void UsersDB::addUser(AccountInfo* newUser) {
    _accounts[_size] = newUser;
    _size++;
}

void UsersDB::showUsers() {
    for (int i = 0; i < _size; i++) {
        cout << *_accounts[i] << endl;
    }
}

//---------------------------------------------------------emptyString function
void emptyString(char* token, int size) {
    for (int i=0; i < size; i++) token[i] = '\0';
}

//----------------------------------------------------------copyString function
void copyString (char* from, char* to, int size) {
    to = new char[size+1];
    for (int i=0; i < size; i++) to[i] = from[i];
    to[size] = '\0';
}

 //--------------------------------------------------------getNextToken function
int getNextToken(char* buffer, char* token, int startPos, 
                    int bufSize, int tokenSize, char delimeter) {
    int i, j;

    emptyString (token, tokenSize);

    i = startPos;
    j = 0;

    while ((buffer[i] == ' ') && (i < bufSize)) i++; //skipblanks
    if (i < 256) {
        while ((buffer[i] != delimeter) && (i < 256) && (j < tokenSize))
                token[j++] = buffer[i++];
    }
    return i;
}

//=============================================================================
int main() {
    char buffer[256];
    char userLoginName[9];
    char password[17];
    int i, j, k;
    char flag[3];;
    char command[11];
    char blank = ' ';

    UsersDB* users = new UsersDB();
    AccountInfo* tempAccount;

    while (!cin.eof()) { //while end of line is not reached
        cin.getline(buffer, 256);
        k = getNextToken(buffer, command, 0, 256, 10, blank);
        if (command[0] == 'a') {
             tempAccount = new AccountInfo();
             k = getNextToken(buffer, userLoginName, k, 256, 8, blank);
             (*tempAccount).setUsername(userLoginName);
             k = getNextToken(buffer, flag, k, 256, 2, blank);
            if (flag[1] == 'p') {
                 k = getNextToken(buffer, password, k, 256, 16, blank);
                (*tempAccount).setPassword(password);
            }
            cout << *tempAccount << endl;
            (*users).addUser(tempAccount);
        }
        else if (command[0] == 's') {
            (*users).showUsers();
        }
        else cout << "Command not found." << endl;
    }

    return 0;
}

The output looks like this:

===============================================================================
>adduser bob -p password1
Login: bob
Password: password1

>adduser jack -p mypassword
Login: jack
Password: mypassword

>adduser jill -p pass1234
Login: jill
Password: pass1234

>showusers
Login: jill
Password: pass1234

Login: jill
Password: pass1234

Login: jill
Password: pass1234
===============================================================================

The output SHOULD look like this:

===============================================================================
>adduser bob -p password1
Login: bob
Password: password1

>adduser jack -p mypassword
Login: jack
Password: mypassword

>adduser jill -p pass1234
Login: jill
Password: pass1234

>showusers
Login: bob
Password: password1

Login: jack
Password: mypassword

Login: jill
Password: pass1234
===============================================================================

Note: When I alter main() (by passing the info directly instead of getting it from the console using cin) to look like this:

//=============================================================================
int main() {

    UsersDB* users = new UsersDB();
    AccountInfo* tempAccount;

    tempAccount = new AccountInfo("jack", "mypassword");
    (*users).addUser(tempAccount);

    tempAccount = new AccountInfo("jill", "pass1234");
    (*users).addUser(tempAccount);

    (*users).showUsers();

    return 0;
}

...I get the desired output.

Thanks a lot.

Joachim Isaksson · Accepted Answer · 2012-02-02 06:06:47Z

2

You're using char* pointers in your class, they don't work like "ordinary" strings. When you pass a char* pointer to a method and assign that to a member variable, you're not copying the string, you're just making the member variable point to the same string as the parameter points to. Since all the parameters when passed in are a pointer to your input buffer, the next input in the buffer will overwrite the text in all previous objects.

In C++, you should really use std::string instead of passing around char* for string manipulation, it will help you greatly in this kind of situation, and will handle copying and allocating memory for new strings automatically.

The reason the latter version with constants works, is that your objects end up pointing to the text constants that aren't overwritten like your buffer is.

answered Feb 2, 2012 at 6:06

Joachim Isaksson

182k28 gold badges297 silver badges307 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

TheBlackKeys Over a year ago

Hey thanks! I'm a student and we are intentionally dodging std::string like as an exercise or something. Good thing too I guess.

Michael Chinen · Accepted Answer · 2012-02-02 06:43:41Z

Your problem is that your UserAccounts (all of them) end up being pointers to the (single) user and password char arrays in main(). You can fix it by creating a new array each time with this main:

//=============================================================================                                                                                                                                                              
int main() {
   char buffer[256];
   char *userLoginName;
   char *password;
   int i, j, k;
   char flag[3];;
   char command[11];
   char blank = ' ';

   UsersDB* users = new UsersDB();
   AccountInfo* tempAccount;

   while (!cin.eof()) { //while end of line is not reached                                                                                                                                                                                   
      cin.getline(buffer, 256);
      k = getNextToken(buffer, command, 0, 256, 10, blank);
      if (command[0] == 'a') {
         userLoginName = new char[9];
         password = new char[17];
         tempAccount = new AccountInfo();
         k = getNextToken(buffer, userLoginName, k, 256, 8, blank);
         (*tempAccount).setUsername(userLoginName);
         k = getNextToken(buffer, flag, k, 256, 2, blank);
         if (flag[1] == 'p') {
            k = getNextToken(buffer, password, k, 256, 16, blank);
            (*tempAccount).setPassword(password);
         }
         cout << *tempAccount << endl;
         (*users).addUser(tempAccount);
      }
      else if (command[0] == 's') {
         (*users).showUsers();
      }
      else cout << "Command not found." << endl;
   }

   return 0;
}

Just remember to delete them afterwards. This is the least lines of code to fix it, which I'm showing just to demonstrate what the problem is. A better solution would be to instead create new arrays within the UserAccount on construction (since it seems you will always need them) and delete them on dtor like this:

//At top of file:
#include <string.h>

AccountInfo::AccountInfo() {
   _username = new char[9];
   _password = new char[17];
}

AccountInfo::AccountInfo(char* username, char* password) {
   strcpy(_username, username);
   strcpy(_password, password);
}

AccountInfo::~AccountInfo() {
   delete _username;
   delete _password;
}

void AccountInfo::setUsername(char* username) {
   strcpy(_username, username);
}

void AccountInfo::setPassword(char* password) {
   strcpy(_password, password);
}

Thank you very much! I have spent many hours on this problem (sadly...) and would have spent many more hours. A++ would read again.

David Brown · Accepted Answer · 2012-02-02 06:10:33Z

1

You never allocate space to store the usernames and passwords in your AccountInfo class. Your setUserName and setPassword functions simply copy the raw pointers into the class. So in your first main each user simply points to the username and userLoginName buffers you declared in main thus they are all the same. In your second main, they point to the string literals you use to construct them.

I would also add that this is essentially C code with member functions. If you're using C++ you should use C++ features like std::string and std::vector which make errors like this nearly impossible.

answered Feb 2, 2012 at 6:10

David Brown

13.6k4 gold badges41 silver badges56 bronze badges

Comments

Vorlauf · Accepted Answer · 2012-02-02 06:11:04Z

0

In your program setPassword and setUsername store pointers to passed in buffers rather than making a copy of the string.

You should use your copyString function.

answered Feb 2, 2012 at 6:11

Vorlauf

695 bronze badges

Collectives™ on Stack Overflow

C++ array of pointers returns the same value for each element

Here is my code:

The output looks like this:

The output SHOULD look like this:

4 Answers 4

1 Comment

1 Comment

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

Here is my code:

The output looks like this:

The output SHOULD look like this:

4 Answers 4

1 Comment

1 Comment

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related