3

I have a small program in which I have two structs: Person - Consists of id, and basic methods Ppl - Consists of an array of people with some methods to operate on the array.

struct Person {
 const int id;
 Person();
 Person(int a);
};

Person::Person(int a) : id(a) {}

This is the Person struct with its methods.

const int MAX = 5;

Sets limit on array length

struct Ppl {
 static int current;   //Keeps track of current position in array
 Person *ppls[MAX];    //Creates an array of Person structure pointers
 void add(int a);      //Adds a person with id to the next available position
 //void remove(int b);
 int searchid(int c);  //Searches ppls for an id c.
 Ppl();                //Constructor
};

int Ppl::current = 0;  //Initializing static variable

void Ppl::add(int a) {
 int ret = searchid(a);          //Determine if id a already exists in ppls
 //cout << "Ret: " << ret << endl;
 if (ret > MAX) {                //If value returned is > MAX, value exists
  cout << "User " << a << " already exists" << endl;
 } else {                        //else, add it to the next available spot
  Person p(a);
  ppls[current] = &p;
  cout << "Added user: " << a << " at index: " << current << endl;
  current++;
 }
}

Ppl::Ppl() {
 current = 0;
 int i = 0;
 while (i < MAX) {    //Sets all Person pointers to NULL on initialization
  ppls[i] = NULL;
  cout << "ppls[" << i << "] is NULL" << endl;
  i++;
 }
}

int Ppl::searchid(int c) {
 int i = 0;
 bool found = false;
 while(i < MAX) {
  if (ppls[i] == NULL) {  //If NULL, then c wasn't found in array because
   break;                 //there is no NULL between available spots.
  } else {
    if (ppls[i]->id == c) {
     found = true;
   }
  }
  i++;
 }
 if (found == true) {
  return 10;     //A number higher than MAX
 } else {
  return 1;      //A number lower than MAX
 }
}

The main function is:

int main() {
 Ppl people;
 people.add(21);
 cout << people.ppls[0]->id << endl;
 people.add(7);
 cout << people.ppls[0]->id << " ";
 cout << people.ppls[1]->id << endl;
 people.add(42);
 cout << people.ppls[0]->id << " ";
 cout << people.ppls[1]->id << " ";
 cout << people.ppls[2]->id << endl;
 people.add(21);
 cout << people.ppls[0]->id << " ";
 cout << people.ppls[1]->id << " ";
 cout << people.ppls[2]->id << " ";
 cout << people.ppls[3]->id << endl;
}

The output that I get is:

ppls[0] is NULL
ppls[1] is NULL
ppls[2] is NULL
ppls[3] is NULL
ppls[4] is NULL
Added user: 21 at index: 0
21
Added user: 7 at index: 1
7 0
Added user: 42 at index: 2
42 0 0
Added user: 21 at index: 3
21 0 0 0

Why is it adding all new entries to the beginning of the array while keeping the rest NULL? Why isn't it detecting that 21 was already added.

I have been going crazy trying to figure this out. Any help would be much appreciated. Thanks a lot community.

EDIT I have fixed it so that it adds the elements to the array and recognizes when an id exists.

I made changes to the Ppl struct by adding a destructor:

Ppl::~Ppl() {
 int i = 0;
 while (i < MAX) {
  delete ppls[i];
  i++;
 }
}

and by changing the add method.

void Ppl::add(int a) {
 int ret = searchid(a);
 //cout << "Ret: " << ret << endl;
 if (ret > MAX) {
  cout << "User " << a << " already exists" << endl;
 } else {
  **Person *p = new Person(a);
  ppls[current] = p;**
  cout << "Added user: " << a << " at index: " << current << endl;
  current++;
 }
}

So the output now is

ppls[0] is NULL
ppls[1] is NULL
ppls[2] is NULL
ppls[3] is NULL
ppls[4] is NULL
Added user: 21 at index: 0
21
Added user: 7 at index: 1
21 7
Added user: 42 at index: 2
21 7 42
User 21 already exists
Segmentation fault (core dumped)

What is a segmentation fault and how can I fix it? Thanks again

4
  • You're assigning an address of a stack variable to a pointer array... Always a bad idea. Person p(a); then ppls[current] = &p;? I don't have time to write a proper answer but you have a long way to go in your programming career... Commented Nov 4, 2015 at 2:19
  • Thanks. And I know. I'm still only an undergrad Commented Nov 4, 2015 at 2:53
  • Sorry if I came off dismissive or insulting. It's good to see new people taking up programming, sorry I didn't have time to help properly. Welcome to the club! Commented Nov 6, 2015 at 2:37
  • Please read this blog post, and learn how to use a debugger. They will be invaluable to you in the future. Commented Nov 6, 2015 at 2:44

1 Answer 1

4
Person p(a);
ppls[current] = &p;

is a problem. You are storing a pointer to a local variable. Your program is subject to undefined behavior.

Use

Person* p = new Person(a);
ppls[current] = p;

Make sure to delete the objects in the destructor of Ppl.

Suggestion for improvement

It's not clear what's your objective with this program but you can make your life a lot simpler by using

std::vector<Person> ppls;

instead of

Person *ppls[MAX];
Sign up to request clarification or add additional context in comments.

3 Comments

Thanks for the help. The reason that I can't use vectors and the reason that this program has no clear objective is because I wrote this program to understand some concepts I was having trouble with. We were taught to use structs at my school, and I was trying out an exercise.
I ran the program after fixing the mistakes. I'm getting a segmentation fault Please refer to my edit above.
@thisisalongdisplayname, the segmentation fault is caused by the last line in main since people.ppls[3] is NULL.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.