CODEWITHSUNDEEP
c++arrayspointers
TheBlackKeys
TheBlackKeys

Reputation: 852

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

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.

Upvotes: 0

Views: 372

Answers (4)

Michael Chinen
Michael Chinen

Reputation: 18697

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);
}

Upvotes: 1

Vorlauf
Vorlauf

Reputation: 69

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.

Upvotes: 0

David Brown
David Brown

Reputation: 13526

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.

Upvotes: 1

Joachim Isaksson
Joachim Isaksson

Reputation: 180917

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.

Upvotes: 2

Related Questions