inserting struct pointer to vector

Question

I'm trying to make a list of struct pointer using vector. My struct contains some fields like

#include<stdio.h>
#include<stdlib.h>
#include<vector>
#define MAX 100

struct Student {
    char* name,
    *phoneNum,
    *address
};

I have a utility function that helps init for struct pointer

struct Student* newStudent() {
    struct Student* pStudent = NULL;
    pStudent = (struct Student*)malloc(sizeof(struct Student));
    pStudent->name = (char*)malloc(MAX * sizeof(char));
    pStudent->phoneNum = (char*)malloc(MAX * sizeof(char));
    pStudent->address = (char*)malloc(MAX * sizeof(char));
  
    return pStudent;
}

inserting function is like

void insert(vector<Student*> &listStudents, Student* pStudent) {
    printf("name: "); scanf("%s\n" , pStudent->name);
    printf("phone number: "); scanf("%s\n", pStudent->phoneNum);
    printf("address: "); scanf("%s\n", pStudent->address);
    listStudents.push_back(pStudent);
    printf("inserted OK!\n");
    printf("Size: %lu\n", listStudents.size());
}

and display function

void display(vector<Student*>& listStudents) {
    printf("total students: %lu\n", listStudents.size());
    for (int i = 0; i < listStudents.size(); i++) {
        printf("Student %d\n", i+1);
        printf("name: %s\n", listStudents[i]->name);
        printf("phone number: %s\n", listStudents[i]->phoneNum);
        printf("address %s\n", listStudents[i]->address);
    } 
}

here is my main function

int main() {
   
   Student* pStudent = newStudent();
   vector<Student*> listStudents;
   while(true) {
        int op1;
        printf("\n1. input\n2. output\n3. search\n4. erase\n5. end\n");
        printf("option: "); 
        scanf("%d", &op1);
        switch(op1) {
            case 1:
                insert(listStudents, pStudent);
                break;
            case 2:
                display(listStudents);
                break;
            default:
                printf("invalid option!\n");
                break;
        }
    }
    
    free(pStudent);
}

when I tried to insert some information into each field. It's was fine. But when I displayed it out. The results are duplicated. For example:

insert:

Student 1:
name: A
phone number: 010...
address: xyz

Student 2:
name: B 
phone number: 011...
address: zyz

display the result was

Student 1:
name: B 
phone number: 011...
address: zyz 

Student 2:
name: B 
phone number: 011...
address: zyz

What's wrong with that??

insert just pushes the argument-provided student object pointer into the vector. You pass the same object pointer with each iteration, changing its content along the way. As a result, your vector is filled with (a) the same pointer duplicated over and over, and (b) the pointed-to object contains whatever was read last. Why you're mashing C and C++ concepts in this is a bigger mystery. — WhozCraig
– WhozCraig, Commented May 20, 2021 at 6:03
Consider just how many Student objects are ever created in your code since the call to newStudent() happens once. — TheUndeadFish
– TheUndeadFish, Commented May 20, 2021 at 6:03
If you are using C++ for vector, why can't use use proper C++ for the Student struct. Namely, a constructor, new instead of malloc, or better yet, just use std::string instead of char* . Not to mention cout instead of printf. You'll save many lines of code... — selbie
– selbie, Commented May 20, 2021 at 6:06
What's wrong with that?? You are making a big mess of mixing c++ and c. Why.... ? — super
– super, Commented May 20, 2021 at 6:40

Aykhan Hagverdili · Accepted Answer · 2021-05-20 06:44:36Z

2

Your problem is that you have a single instance of the student class and you write over that. In C++, classes usually have value semantics. You usually don't manage your own memory. Here's a more idiomatic implementation:

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

class Student final {
 private:
  std::string m_name;
  std::string m_phoneNum;
  std::string m_address;

 public:
  Student(std::string name, std::string phoneNum, std::string address)
      : m_name(std::move(name)),
        m_phoneNum(std::move(phoneNum)),
        m_address(std::move(address)) {}

  auto& Name() const noexcept { return m_name; }

  auto& PhoneNumber() const noexcept { return m_phoneNum; }

  auto& Address() const noexcept { return m_address; }
};

Student GetStudent() {
  std::string name, num, addr;
  std::cout << "Name: ";
  std::cin >> name;
  std::cout << "Phone nnumber: ";
  std::cin >> num;
  std::cout << "Address: ";
  std::cin >> addr;
  Student st(std::move(name), std::move(num), std::move(addr));
  return st;
}

int main() {
  std::cin.exceptions(std::istream::failbit | std::istream::badbit);

  std::vector<Student> vec;
  vec.push_back(GetStudent());
  vec.push_back(GetStudent());
  vec.push_back(GetStudent());

  for (auto const& elm : vec) {
    std::cout << "Name: " << elm.Name() << "\nPhone: " << elm.PhoneNumber()
              << "\nAddress: " << elm.Address() << '\n';
  }
}

This also solves many other bugs:

Your scanf calls are subject to buffer overflow
lu is not the correct specifier for size_t
You leak the string members (all of them)
You don't check for nullptr after malloc
Your code is not exception safe
printf should either be flushed explicitly, or you should end it with '\n' before calling scanf.

and possibly more.

edited May 20, 2021 at 6:44

answered May 20, 2021 at 6:28

Aykhan Hagverdili

30.5k6 gold badges53 silver badges109 bronze badges

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

4 Comments

frankiie Over a year ago

Thanks for your comment! I have an idea about using class. But I just want to try with struct pointer and see how it works.

Aykhan Hagverdili Over a year ago

@KeinKeinKein it's possible to make it work. It's just harder.

frankiie Over a year ago

yeah, the above guys just pointed out my mistake that is I used duplicated struct pointer for every inserting data. So when printed out the results are the same. But anyway thank you the the kind!

Aykhan Hagverdili Over a year ago

@KeinKeinKein doing it the "C way" is not inherently bad. It is just more difficult. You have to keep track of many little things, all those pointers, and if you get them wrong, it fails silently. If you are in the "++ Land", you might as well do it the C++ way. Otherwise you could use the C language to begin with. I did point out lots of bugs in your implementation.

Shreyan Avigyan · Accepted Answer · 2021-05-20 06:31:55Z

0

As @WhozCraig mentioned in his comment, you are changing the same struct pointer. You must allocate different struct pointer each time. Your main function should look like this,

int main() {
   vector<Student*> listStudents;
   while(true) {
        int op1;
        printf("\n1. input\n2. output\n3. search\n4. erase\n5. end\n");
        printf("option: "); 
        scanf("%d", &op1);
        switch(op1) {
            case 1: {
                Student* pStudent = newStudent();
                insert(listStudents, pStudent);
                break;
            }
            case 2:
                display(listStudents);
                break;
            default:
                printf("invalid option!\n");
                break;
        }
    }
    
    for (const auto& pointers: listStudents)
        free(pointers)
}

You have to allocate a new struct pointer each time user enters 1.

edited May 20, 2021 at 6:31

answered May 20, 2021 at 6:19

Shreyan Avigyan

1831 silver badge11 bronze badges

3 Comments

frankiie Over a year ago

@Thanks for your comment! But put the object inside switch's case like above cause It will cause a "crosses initialization error`.

Shreyan Avigyan Over a year ago

Ok. So there'll be another edit. I'll edit and fix the code.

Shreyan Avigyan Over a year ago

No there'll not be a error because the scope of the variable is local.

Collectives™ on Stack Overflow

inserting struct pointer to vector

2 Answers 2

4 Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

4 Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related