Bewertung der 1. Adressverwaltung-Abgabe

Häufige Kritikpunkte:

• Document-Klasse darf nicht POSITION nach außen geben. Es muss transparent sein wie die Klasse intern die Daten speichert. Stellt euch vor ihr tauscht den Container gegen irgend etwas anderes (z.B. Array-Klasse). Dann müßtet ihr diese Änderung an vielen Stellen nachziehen.
• Anlegen einer Adresse ohne Vorname und Nachname (macht keinen Sinn in einer ADRESSEN-Verwaltung)
• Kommentare !!!

137 236

• Fehlt noch...

143 376

• Abgabe: Rational-Projekt nicht nur auf Festplatte löschen, sondern auch aus Solution !
• Speicherleck in CAdressenDoc::getAdresse: "CAdresse *temp = new CAdresse()" ist unnötig. (immerhin in CAdressenView::OnLbnSelchangeNamensliste erkannt)
• Viel zu wenig Kommentare (Methoden selten kommentiert, keine Kommentare innerhalb vor Schleifen, if-Abfragen etc)
• CTypedPtrMap wird vom Document nach außen gegeben (CAdressenView::OnUpdate). Dazu hast du doch "iterMap" und "iterStart" implementiert !
• CTypedPtrMap ist public in CAdressenDoc

146 006

• Memory Leak in CAdressenDoc::getNextElement ("CAdresse* Person = new CAdresse();") und CAdressenDoc::selectionChanged ("CAdresse* dummy = new CAdresse();"), CAdressenDoc::createNewPerson.
• CAdressenDoc.createNewPerson: wieso beim Ersetzen eine vorhandene Person löschen statt sie zu recyclen ?
• Die Exception wird nirgends geworfen. Unverwendeter Code macht keinen guten Eindruck. Dito für ein paar Eventhandler in der view (OnEnChangeEditName, OnEnChangeEditPlz).
• Gute Kommentare in z.B. CAdressenDoc.createNewPerson, an anderer Stelle wird es dünner !
• Sehr schön: die View kommt völlig ohne Kenntnis von CAdresse aus.

243 408

• Validate: wenn bei Postleitzahl ungültige Eingabe (Buchstaben), dann gibt es zwei MessageBoxen (MFC-Standard und eigene MessageBox "Sie müssen alle Felder füllen".)
• Speicherleck in CAdressenDoc::SaveAdress: hier wird auch beim Überschreiben eine neue Adresse angelegt.
• Ein bißchen zu wenig Kommentare (Methoden-Parameter, bei Schleifen und if-Abfragen wird eigentlich nicht kommentiert WAS passiert)
• CAdressenDoc::GetPos: POSITION nicht nach außen geben !

246 016

• POSITION darf von CAdressenDoc nicht nach außen gegeben werden.
• Ein bißchen mehr Kommentierung wäre schön

340 037

• Scrolleiste beim Anwendungsstart ist unnötig. Sooo viele GUI-Elemente enthält dein Fenster garnicht.
• DeleteContents von CDocument ist nicht überladen (angelegte Adressen werden nie gelöscht). Dadurch funktioniert auch der Klick auf "Neu" nicht.
• Bei Eingabe von Buchstaben bei Postleitzahl gibt es zwar eine MessageBox, aber die Adresse wird übernommen. Im DoDataExchange anschließend folgende Felder werden dabei allerdings nicht gesetzt.
• Es ist möglich eine Adresse ohne Name und Vorname anzulegen.
• Bei nicht gewählter Adresse und Klick auf Löschen passiert garnix, nichtmal eine MessageBox
• Serialize von CAdressenDoc: Beim Laden sind die Aufrufe von "DeleteContents" und "UpdateAllViews" nicht nötig, dies macht CAdressenDoc selbst.
• Kommentare sind einigermaßen OK, die Eventhandler in der View hätten allerdings auch Kommentare verdient.

546 873

• Eigentliche Abgabe:
  • Beim Update einer vorhandenen Adresse Speicherloch, weil die alte nicht wiederverwendet wird.
  • POSITION darf nicht von Document nach außen gegeben werden
  • Referenzkommentierung !
  • Serialize (Laden) von CAdressenDoc: "DeleteContents();" und "UpdateAllViews(NULL);" nicht nötig, macht CDocument.
• Update 21.04.2005:
  • Neue Version stürzt beim Update einer Adresse ab, weil in "UpdateEntry" für den geänderten Eintrag "mapRemoveKey" aufgerufen wird und dadurch "elementCount" um 1 reduziert wird, ohne anschließend wieder hochgezählt zu werden. Crash beim Laden, weil "elementCount" nicht aktualisiert wird. Lass diese Variable raus, den Wert kannst du jederzeit als "m_pAdressenMap.GetCount()" abrufen,.
  • Serialize: "DeleteContents();" und "UpdateAllViews(NULL);" nicht nötig.
  • Kapselung der CTypedPtrMap ist jetzt OK: nur noch Namen nach außen geben, keine POSITION mehr. Achtung: Der Aufruf

          if (count > 0) delete [] keys; 

    im OnUpdate der View läßt ein Speicherloch zurück ("normal block, 4 bytes long"). Immer löschen, auch bei leerem Array !

546 881

• Anlegen von Adresse mit leerem Namen und Vornamen möglich.
• Kommentare stehen in Header-Datei, wäre schön wenn die primär in CPP-Datei stünden, weil man da am ehesten nachschaut.
• Teilweise mehr Kommentare in Methoden, z.B. in "CAdressenView.OnLbnSelchangeList1" gibt es garnichts (aber immerhin den Kommentar in Headerdatei)
• Übrigens: die ListBox hätte sich über einen anderen Namen als IDC_LIST1 bestimmt gefreut.

835 794

• Eigentliche Abgabe:
  • Uncompilierbar !
• Update 04.05.2005:
  • Wenn im About-Dialog der Name von 543 516 aus Gruppe A steht und die Anwendung keinerlei Ähnlichkeit mit der nicht compilierbaren ersten Abgabe hat (Buttons und Methoden heißen allesamt anders etc), dann kann das nur eine Kopie sein. Für soviel Dämlichkeit gibt es 0 Punkte !

843 365

• Eigentliche Abgabe:
  • Wieso passiert beim Klick auf "Übernehmen" und leeren Feldern garnix ? Eine MessageBox wäre schon schön.
  • Speicherloch in CAdressenDoc:getAdresse ("CAdresse *temp = new CAdresse();")
  • TypedPtrMap wird per "CAdressenDoc::GetAdressenMap" nach außen gegeben. Verboten ! Entsprechend darf die View nicht direkt über diese Map laufen. Die Membervariable im Doc ist außerdem public.
  • Kommentare: im Doc sind die Methodenkommentare einigermaßen OK, aber innerhalb der Methoden hätte mehr kommentiert werden dürfen. Die View ist leider eine Wüste.
  • CAdressenView::OnBnClickedButtonUebernehmen: Bei Änderung wäre es besser nicht die Original-Adresse zu löschen und neu anzulegen sondern die alte zu aktualisieren.
• Update 26.04.2005:
  • Bei Postleitzahl z.B. Buchstaben eingeben: Beim Klick auf "Übernehmen" kommt eine MessageBox mit einer Fehlermeldung, aber die Adresse wird gespeichert (===> Rückgabewert von UpdateData auswerten !)
  • Kapselung der CTypedPtrMap im Document ist jetzt OK. Aber das in CAdressenDoc::getKeys erzeugte CString-Array muss in CAdressenView::OnUpdate auch wieder gelöscht werden (Speicherloch !).
  • Wieso werden im OnUpdate die Eingabefelder geleert ("OnBnClickedButtonNeu") ?
  • CAdressenDoc::addAdresse: erzeugt beim Überschreiben einer vorhandenen Adresse ein Speicherloch. Besser: zuerst prüfen ob der Key schon in TypedPtrMap enthalten ist ("Lookup"). Wenn ja dann diese Adresse updaten: alle Feldwerte setzen. Wenn nein dann eine neue Adresse zufügen.
  • Kommentierung ist jetzt sehr gut.
• Update 01.05.2005:
  • OnBnClickedButtonUebernehmen: Doppelter Aufruf von UpdateDAta. Dadurch: wenn bei Postleitzahl nur Buchstaben eingeben wurden kommt die Meldung "Positive Zahl eingeben" zweimal. Das zweite UpdateDAta ist unnötig.
  • CAdressenDoc::addAdresse: Crash beim Überschreiben einer Adresse, weil zwar die vorherige gelöscht wird, aber der Zeiger in der CTypedPtrMap nicht entfernt wird und auch nicht durch die neue Adresse ersetzt wird. Statt:

    	if(Lookup(key, ptr))
    	{
    		delete ptr;
    	}
    	else
    	{
    		/// Falls der Key noch nicht existiert in die Map eintragen 
    		m_pAdressenMap[key] = adresse;
    	}
          

    sollte dies verwendet werden:

    	if(Lookup(key, ptr))
    	{
    		delete ptr;
    	}
    	/// Falls der Key noch nicht existiert in die Map eintragen 
    	m_pAdressenMap[key] = adresse;
          

  • Andere Punkte sind behoben.
• Update 04.05.2005:
  • PLZ-Prüfung ist jetzt OK. Übrigens: die Prüfung auf eingegebenen Vornamen und Nachnamen hättest du auch ins DoDataExchange packen können, und im Fehlerfall mittels "pDX->Fail()" die Rückgabe von UpdateData auf FALSE setzen können. Hier der vorgeschlagene Codeausschnitt aus DoDataExchange:

    	//Nur beim Save+Validate die Prüfung auf Nachname/Vorname:
    	if (pDX->m_bSaveAndValidate == TRUE)
    	{
    		if(m_strName == "" && m_strVorname == "")
    		{
    			AfxMessageBox(_T ("Bitte füllen Sie Vor- und Nachname aus!"), MB_OK);
    			pDX->Fail ();
    		}
    	} 

  • CAdressenDoc::Serialize: Der Aufruf von UpdateAllViews beim Laden ist unnötig, passiert automatisch.
  • Alle Kritikpunkte aus der ersten Abgabe behoben.

845 927

• Speicherleck in CAdressenDoc::AddAdresse: hier wird auch beim Überschreiben eine neue Adresse angelegt.
• CAdresseDoc darf die POSITION nicht nach außen geben.
• Kommentare sind weitgehend OK.

944 689

• Es ist möglich eine Adresse ohne Name und Vorname anzulegen.
• Speicherloch: beim Bearbeiten einer Adresse wird die alte nicht recycled sondern eine neue erzeugt, die alte wird nicht gelöscht.
• Speicherloch: Beim Löschen wird die Adresse nur aus der Liste entfernt aber nicht gelöscht.
• Wenn bei Postleitzahl z.B. Buchstaben eingegeben werden, dann gibt es eine MessageBox aber die Adresse wird trotzdem angelegt.
• CAdressenDoc::GetAdresse: Zeigt MessageBox an. Verboten, MessageBoxen sind Sache der Oberfläche. Entweder Rückgabewert = NULL oder Exception.
• Serialize von CAdressenDoc: Beim Laden sind die Aufrufe von "DeleteContents" und "UpdateAllViews" nicht nötig, dies macht CAdressenDoc selbst.
• CAdressenView::OnBnClickedButtonUebernehmen: Item wird in Liste eingefügt, anschließend wird UpdateAllViews aufgerufen -> Doppelt gemoppelt.
• Sehr gute Kommentierung

946 665

• Absturz wenn ohne gewählte Adresse auf "Löschen" geklickt wird.
• Anlegen von Adressen ohne Name und Vorname ist möglich.
• Zweimaliges Klicken auf "Neu" legt eine leere Adresse doppelt an, einschließlich Speicherloch
• Fast keine Kommentare.
• Bonus: Splitter. Könnte ich das Projekt eventuell aus dem letzten Semester kennen (und ist es eventuell mit Visual Studio 6 erstellt, das würde die Deprecation-Warning beim compilieren erklären) ?