C++ vector::erase

[onewho]

В общем есть класс Student который содержит имя и оценки.
Есть класс Group который содержит в себе изменяемый список студентов (такая мини-бд).
Проблема при удалении студента из списка. Ещё мусор какой-то генерится...
Кароче вот код, красным выделил места проблем.

#include <vector>
#include <iostream>
#include <cstring>
#include <algorithm>



using std::cout;
using std::cin;
using std::endl;



const int n=10;





/* Class Student */

class Student {
	char* name;

protected:
	int marks[n];

public:
	Student() {                             // constr 1
		name = new char[8];
		strcpy(name,"no name");
		for (int i=0; i<n; i++)
			marks[i]=-1; 
	}

	Student(char* _name) {                    // constr 2
		name = new char[strlen(_name)+1];
		strcpy(name,_name);
		for (int i=0; i<n; i++)
			marks[i]=-1;
	}

	Student(const Student& obj) {           // copy constr
		name = new char[strlen(obj.name)+1];
		strcpy(name,obj.name);
		for (int i=0; i<n; i++)
			marks[i]=obj.marks[i];
	}

	~Student() { delete[] name; }

	char* get_name() {return name; }

	void show_data() const { 
		cout << name << endl << "marks: ";
		for (int i=0; i<n; i++)
			cout << marks[i] << " ";
		cout << endl;
	}

	void set_marks(int* data) {
			for (int i=0; i<n; i++)
				marks[i]=data[i];
	}

};							



class Group {
	std::vector<Student> data;

public:
	Group() {}

	void add_student(Student obj) {
		data.push_back(obj);
	}

	void show_list() {
		std::vector<Student>::iterator it;
		for (it=data.begin(); it<data.end(); it++) {
			it->show_data();
			cout << endl;
		}
	}

	void remove(Student obj) {
		for (int i=0; i<data.size(); i++)
			if (strcmp(data[i].get_name(),obj.get_name())==0) {
				data.erase(data.begin()+i);
				break;
			}

	}


};





int main()
{

	int marks[n] = {4,4,4,3,3,3,4,5,4,5};

	Student dima("dima petrov");
	Student yan("yan zelenovsky");
	Student petya("petya smirnov");
	dima.set_marks(marks);
	yan.set_marks(marks);
	petya.set_marks(marks);

	cout << "--------demo---------\n\n";

	Group _5911;
	_5911.add_student(dima);
	_5911.add_student(yan);
	_5911.add_student(petya);

	cout << "--------list before remove()---------\n\n";
	_5911.show_list();

	_5911.remove(yan);
	cout << "--------list after remove()---------\n\n";

	_5911.show_list();

	return 0;

}

аутпут...

[Obey-Kun]

for (int i=0; i<data.size(); i++)
if (strcmp(data[i].get_name(),obj.get_name())==0) {
data.erase(data.begin()+i);
break;
}

Я же тебе говорил, итерируй при помощи итераторов! А проблема тут проще паренной репы -- i у тебя идёт от 0 до первоначального размера, но размер во время выполнения цикла становится меньше. Вот и падает программа. Использовал бы итераторы -- не было бы проблемы.

Алсо, вместо show_data и show_list перегружай operator<< для ostream.

[Obey-Kun]

Вот ещё что, ты нарушил один из принципов ООП, поместив поле marks в protected. Его надо поместить в private. Все поля должны быть private. Для доступа мог сделать в private метод, возвращающий константный указатель на начало массива оценок.

И да, насчёт именования. Почитай вторучю половину сообщения (со слов «Всё. Но осталось рассказать о важной вещи. Неотъемлемой частью документации к API и к коду является следование хорошим правилам именования.»): http://www.programmersforum.ru/showt...083#post747083. И последнее приложение в той же теме.

[onewho]

ну я изначально и хотел с помощью итераторов

		std::vector<Student>::iterator it;
		for (it=data.begin(); it!=data.end(); it++)
			if (strcmp(it->get_name(),obj.get_name())==0) {
				data.erase(data.begin()+i);

ну допустим сделал я так.
а что вместо i ставить?

//upd: ой а кажется надо так: data.erase(it) ? ток ненадо меня бить)

"Вот ещё что, ты нарушил один из принципов ООП, поместив поле marks в protected. Его надо поместить в private. Все поля должны быть private. Для доступа мог сделать в private метод, возвращающий константный указатель на начало массива оценок." это сделаю

[Obey-Kun]

Подумай, какой тип у data.begin() и сам поймёшь, что там ставить вместо data.begin()+i.
И да, вместо smth++ лучше писать ++smth, в некоторых случаях это даёт оптимизацию (а по мне так ещё и лучше читается).

[onewho]

	void remove(Student &obj) {
		std::vector<Student>::iterator it;
		for (it=data.begin(); it!=data.end(); it++)
			if (strcmp(it->get_name(),obj.get_name())==0) {
				data.erase(it);
				break;
			}

сделал так все равно критует, причем если сделать data.erase(++it); то все ок, но удаляет гад не то что надо. (следующего)

не понятно почему data.erase(it); не работает...

[Obey-Kun]

Замени на it = data.erase(it). Делов-то.

Ну а лучше сделать так:
1) Делаешь bool Student::operator== (const &Student other) { return strcmp(get_name(),other.get_name()) ; }
2) В твоём методе remove пишешь просто std::erase(std::remove(data.begin() , data.end(), obj), v.end());

И да, у тебя косяк. Не void remove(Student &obj), void remove(const Student &obj) должно быть.

[Obey-Kun]

И ещё косяк:

char* get_name() {return name; }

Геттеры должны быть константными! А так можно имя студенту менять... Надо так:

const char* get_name() const {return name; }

То есть константный метод, возвращающий константный указатель. Но со следованием правилам именования, ссылку на которые я давал выше, это будет выглядеть так:

const char* name() const {return m_name; }

Правда ведь cout << some_student.name() смотрится классно?

[onewho]

Сообщение от Obey-Kun

Замени на it = data.erase(it). Делов-то.

тьфу блин не работает!

все эти косяки я знаю, их я исправлю.

[Obey-Kun]

Но это плохой путь, делай через перегрузку оператора и remove.

тьфу блин не работает!

что не работает?