is mein code so in ordnung?

hi,
bin anfänger, hab code geschrieben, möchte wissen was es alles an dem auszusetzen gibt und was ihr anders gemacht hättet. unter dem code stehen noch ein paar konkrete fragen, hier ist mal der quelltext:

Person.h

#ifndef PERSON_H
#define PERSON_H

#include <string>
using namespace std;

class Person {

private:
	string name;

public:	
	Person(string name): name(name) {};

	string getName() {
		return name;
	}

	void setName(string name) {
		this->name = name;
	}
};

#endif

Iterator.h

#ifndef ITERATOR_H
#define ITERATOR_H

class Iterator {
public:
	virtual void* next() = 0;
	virtual bool hasNext() = 0;
};

#endif

LinkedList.h

#ifndef LINKED_LIST_H
#define LINKED_LIST_H
#include <cstdlib>
#include "Iterator.h"

class Node {
private:
	void* data;
	Node* next;

public:
	Node(void* data): data(data), next(NULL) {};
	void* getData() { return data; }
	Node* getNext() { return next; }
	void setNext(Node* next) { this->next = next; }

};

class LinkedListIterator: public Iterator {
	Node* current;
public:
	LinkedListIterator(Node* node): current(node) {};
	void* next();
	bool hasNext();
};

class LinkedList {

private:
	Node* head;
public:
	LinkedList(): head(NULL) {};
	void add(void* data);
	bool remove(void* data);
	Iterator* getIterator();
};

#endif

LinkedList.cpp

#include "LinkedList.h"

void LinkedList::add(void* data) {
	Node* newNode = new Node(data);
	if(head == NULL) {
		head = newNode;
	} else {
		head->setNext(newNode);
	}
}

bool LinkedList::remove(void* data) {
	Node* previous = NULL;
	Node* current = head;
	while(current != NULL) {
		if(current->getData() == data) {
			if(current == head) {
				head = current->getNext();
			} else {
				previous->setNext(current->getNext());
			}
			return true;
		}
		previous = current;
		current = current->getNext();
	}
	return false;
}

Iterator* LinkedList::getIterator() {
	Iterator* it = new LinkedListIterator(this->head);
	return it;
}

bool LinkedListIterator::hasNext() {
	return current != NULL;
}

void* LinkedListIterator::next() {
	if(!hasNext())
		return NULL;
	void* data = current->getData();
	current = current->getNext();
	return data;
}

main.cpp

#include <iostream>
#include "Person.h"
#include "LinkedList.h"

void list(LinkedList* list) {
	cout << "====" << endl;
	Iterator* it = list->getIterator();
	while(it->hasNext()) {
		void* data = it->next();
		Person* person = (Person*) data;
		cout << person->getName() << endl;
	}
	cout << "====" << endl;
}

int main (int argc, char * const argv[]) {
	LinkedList* persons = new LinkedList();
	cout << "test keine personen: " << endl;
	list(persons);
	persons->add(new Person("hans wurst"));
	Person peter("peter lustig");
	persons->add(&peter);
	cout << "2 personen:" << endl;
	list(persons);
	bool ok = persons->remove(&peter);
	cout << "peter entfernen erfolgreich? " << ok << endl;
	cout << "ohne peter jetze:" << endl;
	list(persons);
    return 0;
}

fragen die mir beim programmieren in den sinn gekommen sind:
- wann benutzt man diese syntax für konstruktoren: Node(void* data): data(data), next(NULL) {}; ? finde die komisch
- wann und warum trennt man die implementierung von klassen von der definition (so wie in LinkedList.cpp/LinkedList.h)? Ich finde das nämlich unangenehm wenn der Code für eine einzige klasse über 2 dateien aufgeteilt ist. In der Person klasse hab ich es ja anders gemacht, fand ich angenehmer.
- muss ich in LinkedList::remove() irgendwie speicher aufräumen oder wird das automatisch gemacht?
- gibt es irgendeine möglichkeit die sichtbarkein von klassen einzurschränken? zum beispiel kann jeder die klasse Node und LinkedListIterator instantiieren, was aber keinen sinn macht. Ich hab zuerst versucht die beiden klassen in die LinkedList klasse einzunisten, aber das hat bei der implementierung nicht richtig geklappt

danke für die antworten

edit:

head->setNext(newNode);

in der remove() methode ist natürlich falsch, hab es jetzt geändert in:

Node* current = head;
		while(current->getNext() != NULL)
			current = current->getNext();
		current->setNext(newNode);

drakon

anfängerpeter schrieb:

- wann benutzt man diese syntax für konstruktoren: Node(void* data): data(data), next(NULL) {}; ? finde die komisch

Das ist die Initialisierungsliste und die sollte dazu benutzt werden Datenmember zu initialisieren. (Probier mal ohne die eine const- Member zu haben und einen Anfangswert zuzuweisen).

anfängerpeter schrieb:

- wann und warum trennt man die implementierung von klassen von der definition (so wie in LinkedList.cpp/LinkedList.h)? Ich finde das nämlich unangenehm wenn der Code für eine einzige klasse über 2 dateien aufgeteilt ist. In der Person klasse hab ich es ja anders gemacht, fand ich angenehmer.

Grundsätzlich sollte die Implementierung von der Schnittstelle getrennt werden, daher hast du eine .h und eine .cpp. Der Vorteil liegt darin, dass die Module, die lediglich den Header include nicht neue kompiliert werden müssen, wenn du z.B die Implementierung einer Funktion änderst. Da musst du nur die jeweilige .cpp neu kompilieren, was unter Umständen ein riesen Vorteil ist.
Inline Funktionen kann man aber getrost in der Klasse definieren. Vor allem getter und setter sind beliebet, da die grundsätzlich nicht verändert werden.

anfängerpeter schrieb:

- muss ich in LinkedList::remove() irgendwie speicher aufräumen oder wird das automatisch gemacht?

Für jeden Speicher, den du mit new/new[] anforderst bist du verantwortlich, somit musst auch du dich darum kümmern den Speicher wieder frei zu geben. (mit delete, respektive delete[]).

- gibt es irgendeine möglichkeit die sichtbarkein von klassen einzurschränken? zum beispiel kann jeder die klasse Node und LinkedListIterator instantiieren, was aber keinen sinn macht. Ich hab zuerst versucht die beiden klassen in die LinkedList klasse einzunisten, aber das hat bei der implementierung nicht richtig geklappt

Da gibt es verschiedene Möglichkeiten. Du kannst z.B den Konstruktor privat machen und nur einer Klasse mittels friend die Möglichkeit geben diese überhaupt zu erstellen, oder du könntest auch mit anonymen Namensräumen arbeiten.

danke
das delete würde aber definitiv nicht in die remove() methode gehören, sondern vielmehr dorthin, wo remove() aufgerufen wird, oder?
was passiert, wenn ich noch mehr pointer hab die auf dieses objekt zeigen und ich das dann mit delete entferne? was passiert beim nächsten aufruf von getName() wenn das objekt weg is?

drakon

Grundsätzlich sollte das Objekt, respektive die Stufe, die Speicher anfordert diesen auch wieder freigeben/verwalten.

Wenn du noch mehr Pointer hast, die auf den selben Speicher zeigen, dann darfst du den nachher nicht mehr löschen, da das Verhalten dann undefiniert ist. (Sprich dein Programm wird mit grosser Wahrscheinlichkeit abstürzen).

getName ist bei dir ungefährlich, da std::string den Speicher selbst verwaltet und du dich somit gar nicht drum kümmern musst. Allerdings ist bei dir eher das Problem bei node und getData .

btw:
using namespace std; gehört fast nie in einen Header, da es den Sinn eines namespaces untergräbt. Im Header solltest du direkt den Bereichsauflöser :: benutzen.