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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.