Review für das Team Dichte, Kießler, Rudevica

# Review für das Team Dichte, Kießler, Rudevica ###### tags: `Praktikum` | ID | Story | | ----- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------- | | IO-01 | Als User möchte ich aus Dateien Wahrheitswerttabellen in verschiedenen Formaten einlesen können. (Markdown, CSV optional) | | IO-02 | Die Anwendung muss mir als User die Auswahl der Testfälle als Wahrheitswerttabelle im gleichen Format wie das Input-Format liefern können. (File Reader / Writer) | | IO-03 | Als User möchte ich mehrere beliebige Dateien automatisch hintereinander verarbeiten lassen können. (Bulkprocessing) | | IO-04 | Als User möchte ich darauf hingewiesen werden, dass eine Input Datei fehlerhaft ist. | | PR-01 | Als User möchte ich zwischen verschiedenen Coverage-Maßen wählen können. (mindestens MMBÜ und MCDC, EBÜ optional) | | PR-02 | Als User möchte ich mehrere Coverage-Maße in einem Durchlauf ausführen. (Mehrfachauswahl) | | PR-03 | Als User soll die interne Struktur der Anwendung nicht sichtbar sein. (sauberes Interface) | | NF-01 | Die Implementierung soll Wahrheitswerttabellen mit beliebig vielen atomaren Bedingungen verarbeiten können (Scalability) | | NF-02 | Die Implementierung soll einfach erweiterbar sein für andere Eingabeformate (Modifiability) | | NF-03 | Die Implementierung soll einfach erweiterbar sein für weitere Bedingungüberdeckungskriterien (Modifiability) | | NF-04 | Der Code soll gut wartbar sein (Maintainability) | IO-01 Einlese wurde für md umgesetzt IO-02 Schreib-Formate wurde umgesetzt (in diesem Fall nur für MD) IO-03 Bulkprocessing wurde nur teilweise umgesetzt, da jede Datei einzeln angegeben werden muss und nicht automatisiert (via Ordner z.B.) des weiteren muss man das im Code angeben, so dass es eher Für den Entwickler, nicht für den Otto-Normal-User zu handhaben ist IO-04 Fehlerhafte Datei beim einlesen eventuell -> muss noch geprüft werden, ob Fehler bei nicht konformer Datei abgefangen wird Ergebnis: Nö -> Entweder direkt abbruch weil der Fehler zum Abbruch des Programms führt oder normales ausführen PR-01 Wahlmöglichkeit der Coverage-Maßnahme gegeben durch 1 oder 2 als Werteübergabe PR-02 Mehfachauswahl Coverage-Maßnahme gegeben durch Ziffer 3 PR-03 sauberes Interface ein Bedinungsüberdeckungsobjekt wird erstellt, man bekommt ein arrayzurück, welches in der darauffolgenden Zeilde direkt wieder zur weiteren Bearbeitung reingeführt wird -> intern im Objekt vorhalten NF-01 Erweiterbarkeit Atomare Bedinungen erfüllt NF-02 Modifiability Eingabeformat nicht gegeben -> da keine Hierarchie (so nur nidifizierbar, weniger erweiterbar (außer neue Funktionen usw, -> open-close prinzip)) NF-03 Modifiability Bedinungsüberdeckung siehe NF-02 NF-04 Maintainability (Wartbarkeit) Ein Codereview soll nun nicht den anderen Code niedermachen, sondern das soll hier nun nur dafür dienen, wo man sich verbessern kann! Also bitte nicht persönlich nehmen, sondern eher als eine Chance von Erfahrungen zu profitieren! - Was war der Gedanke hinter der src.zip? - Vermutlich habt ihr eure Daten so ausgetauscht? Wenn ja, geben wir als großen Tip sich mit einem Versionierungstool wie GIT sich auseinander zu setzen! - Was war der Gedanke zur Nutzung des Interfaces zur Bedinungsüberdeckung? - Es wird dieses Interface nirgendwo weiter genutzt und daher unklar, warum vorhanden. War das Ziel der Erweiterbarkeit? Wenn ja, in welchem Bereich? Oder Erleichterung zum Aufbau einer Testumgebung? (1 Verantwortlicher schreibt die Klasse, ein anderer die Tests und man einigt sich auf diese Schnittstellen?) - meistens werden interfaces nach adjektiven benannt wie flyable oder so, um zu zeigen das ein Objekt auch fliegen kann (implements flyable) - Was ist die Idee von boolean Gueltigkeit(wahrheitstabelle)? - Es wird die Methode ausgeführt, und sollte sie true zurückgeben, dann findet ein System.out.println statt?!? - Ziel der Methode ist vermutlich die Prüfung, ob es keine Duplikate an Testcases gibt. Dazu wurde ein StringArray erstellt, welches alle atomaren Teile als 1 Element hinzufügt und anschließend überprüft, indem durch alle Elemente durchitteriert wird und überprüft, ob kein nachfolgendes Element dem gerade zu prüfenden Element entspricht. - Wenn auf Duplikate geprüft werden soll, empfiehlt sich immer ein Array in ein Set zu werfen und zu vergleichen, ob deren Size/Length gleich groß sind, wenn keine weitere Reperatur def gefundenen Duplikate gewünscht ist. - Was ist die Idee hinter Methode prüfeWahheit()? - Vermutung: Die aus Datei ausgelesene Tabelle wird um 1 Spalte erweitert. In dieser letzten Spalte wird eine 1 gesetzt, wenn die Bedinungsüberdeckung diesen Testcase als benötigt makieren will. Nachdem für MMBUE geprüft wurde, sollte diese Methode wohl die letzte Spalte wieder "reseten", damit bei der Prüfung zu MCDC die Spalte nicht noch einträge von MMBUE hat. 1. Wahrheitstabelle ist kein Attribut des Bedinungsüberdeckungs-Objektes, auf dem gearbeitet wird, daher muss dieser nicht angepasst werden. Des weiteren wäre sonst sehr empfehlenswert, wenn es die Ressourcen des Programmausführenden Systems zulassen, ein Attribut extra pro Bedinungsüberdeckung zu erstellen statt ein Attribut für alle, damit man dann auch Seiteneffekte verhindert. 2. Als Tip vielleicht nochmal anschauen, wie der Lebenslauf von Variablen sind. Die Wahheitstabelle übergebt ihr einer Methode. Diese Methode nutzt nun diesen übergebenen Wert und dekonstruiert ihn im Anschluß nach Abschluß der Methode. 3. prüfeWahrheit() hat als Rückgabewert eine Tabelle, und mit diesem zurückgegebene Wert wird nichts weiter gemacht. Also lasst ihr die Methode was berechnen und zurückgeben, ohne damit dann weiter zu arbeiten 4. Wenn möglich, niemals übergebene Parameter modifizieren. Erstellt einfach ein neues Objekt und modifiziert diese. Erleichtert auch die Wartbarkeit, falls die Methode nocheinmal überarbeitet werden muss, und man dafür ggf diese übergebenen Werte brauch. In einer kleinen Methode ist dies vielleicht noch nachvollziebar, sollte diese Methode aber komplexer werden, erkennt man ggf Fehler erst später, da man nicht gesehen hat, das die Parameter modifiziert wurden. Apropos Wartbarkeit: Als nächsten Tipp für die Struktur von Projekten: Wenn man versucht zu erklären, was eine Klasse macht, und dort die Wörter "und" oder "oder" nutzen kann, sollte man überlegen, diese Klasse weiter zu splitten. In diesem Fall habt ihr hier eine Klasse Bedinungsüberdeckung, 1. welche aus einem MD File eine Wahrheitstabelle ausliest 2. UND entscheidet, was zu tun ist, wenn eine CoverageMethode ausgewählt wurde 3. UND welche die MMBUE Bedinungsüberdeckung auf eine Wahrheitstabelle anwenden kann 4. UND welche die MCDC Bedingungsüberdeckung auf eine Wertetabelle angewendet wird 5. UND das Ergebnis dann wieder in eine Datei gespeichert Das bedeutet das diese Datei ziemlich viele Verantwortungen hat. Wenn man nun in einem Team arbeitet und jeder implementiert davon nun 1 Verantwortung, muss bei Fehlern erst geschaut werden, wer dafür Verantwortlich ist (Bedinungsüberdeckung in Zeile 921 ist nicht klar). Dann müss geprüft werden warum, 1 Grund könnte dann ein Seiteneffekt sein oder weil 2 verantwortliche zur Bearbeitung eine Klassenattribut von gleichen Namen gesetzt haben und beide dann darauf gearbeitet haben Daher ist es ratsam auf Verantwortungen in mehrere Klassen auszulagern, damit sowas vermieden werden kann. Namensgebung eurer AusgabeDateien ist unglücklich gewählt wenn die Dateien "ex3.md", "ex2.md", "ex1.md" in dieser Reihenfolge geprüft werden, dann bekommt - die "ex3.md" als Ausgabe z.B. "mcdc coverage1.md" - die "ex2.md" als Ausgabe z.B. "mcdc coverage2.md" - und die "ex1.md" als Ausgabe "mcdc coverage3.md" Persönliche Erfahrung: Entscheidungen, wenn möglich, nicht zahlenabhängig definieren. In diesem Fall nutzt ihr eine Methode CoverageMethode in der ihr den Coverage Type mit Zahlen wie 1,2,3. Das ist in einem Code später nicht leserlich bzw nicht schnell nachvollziehbar, was da passiert! Gerade wenn Tests geschrieben werden müssen und man erstmal schauen muss, wofür nun welche Zahl steht! Am besten eure Methoden direkt nach außen zur Verfügung stellen wie *mmbueCoverage(wahrheitstabelle)* und *mcdcCoverage(wahrheitstabelle)* und dann diese anwenden Wenn ihr die Entscheidung nicht in der Main haben wollt, ist es nützlich einen Handler dazwischen zu schalten, der dann diese Entscheidungen handelt. Es fehlt komplette Java OrdnerStruktur Convention over Configure also hier src/main/java/ -> hier die Main.java src/main/ressoruces -> hier die Dateien, die extern für das Programm benötigt werden (z.B. ex01.md) src/test/java/ -> hier der Ordner für jede Klasse (in gleicher Ordnerstruktur) die getestet werden src/test/ressources -> hier alle benötigten externen Files für das ausführen der Tests Relative Pfade nutzen, damit jeder damit arbeiten kann ohne anpassen entweder deutsch oder englisch (Methodennamen und Parameternamen) bsp throwExc und vergroessereArray - Was ist der Gedanke der Nutzung von ThrowExc? - Auslagerung der Fehler mit Message nicht unbedingt sinnvoll, dann kann man an der Stelle auch direkt die Exception schmeißen. Hat auch den Vorteil, den Text individuell aussagekräftiger zu konstruieren - "Da ist ein Datei-Fehler" ist nicht aussagekräftig. Besser wären Nachrichten in dem Format "Einlesefehler: Beim Einlesen der 4. Zeile der Datei *xy* ist ein Fehler aufgetreten" oder "Einlesefehler: Konnte Datei *xy* nicht finden" DAFAQ IHR PRÜFT IN EINEM TEST 2 SCHON GETESTETE TESTS NOCHMAL? natürlich ist der true, wenn die anderen beiden auch true sind und natürlich ist der false, wenn einer der tests false sind, also macht der Test ansich keinen Sinn oO - Was ist der Gedanke Testumgebung auszusorcen in init(int)? - Man will sehen wie bei einem Test die Testumgebung aufgebaut wird um dann mit Methodenaufruf zu verlgeichen um nachzuvollziehen was wie verglichen wird. ein *init(1)* ist da eher kontraproduktiv, da nicht leicht erkennbar ist, was wie kontrolliert wird. - also init und direkt eine assert-Zeile ist nicht leicht leserlich Ein Codereview soll nun nicht den anderen Code niedermachen, sondern das soll hier nun nur dafür dienen, wo man sich verbessern kann! Also bitte nicht persönlich nehmen, sondern eher als eine Chance von Erfahrungen zu profitieren! So, kommen wir zu dem nichtfunktionalen Teil an, Hier sollte das Thema - der Skaliberbarkeit der atomaten Bedinungen, - Erweiterbarkeit in Eingabeformat und Bedinungsüberdeckungskriterien - sowie Wartbarkeit hier werd ich ein wenig mehr auf den Code eingehen. ---------------------------- Es ist super, das ihr mit Javadoc gearbeitet habt, aber leider nicht durchgängig, das wäre ein guter Punkt zum Thema Wartbarkeit gewesen. So haben wir die ein oder andere Methode gefunden, wo man erstmal herausfinden musste, was sie machen und warum. Um 2 Beispiele zu nennen: ---------------------------- was macht boolean Gültigkeit(wahrheitstabelle)? Ziel der Methode ist vermutlich die Prüfung, ob es keine Duplikate an Testcases gibt. Dazu wurde ein StringArray erstellt, welches alle atomaren Teile als 1 Element hinzufügt und anschließend überprüft, indem durch alle Elemente durchitteriert wird und überprüft, ob kein nachfolgendes Element dem gerade zu prüfenden Element entspricht. - Wenn auf Duplikate geprüft werden soll ohne Reperatur bei gefunden Fehler, empfiehlt sich immer ein Array in ein Set zu werfen und zu vergleichen, ob deren Size/Length gleich groß sind. Aber ist das z.B. eine Aufgabe, die an der Stelle gemacht werden sollte wo je nach typecase entschieden wird, welche Methode ausgeführt wird? Wäre das nicht eher eine Aufgabe, die man beim einlesen machen sollte? Und sollte da ein Fehler auftreten, dann beendet ihr das System mit System.exit() ohne weiteren Hinweis warum! ---------------------------- Was war die Idee hinter prüfeWahtheit? Vermutung: Die aus Datei ausgelesene Tabelle wird um 1 Spalte erweitert. In dieser letzten Spalte wird eine 1 gesetzt, wenn die Bedinungsüberdeckung diesen Testcase als benötigt makieren will. Nachdem für MMBUE geprüft wurde, sollte diese Methode wohl die letzte Spalte wieder "reseten", damit bei der Prüfung zu MCDC die Spalte nicht noch einträge von MMBUE hat. - 1. Wahrheitstabelle ist kein Attribut des Bedinungsüberdeckungs-Objektes, auf dem gearbeitet wird, daher muss dieser nicht angepasst werden. Des weiteren wäre sonst sehr empfehlenswert, wenn es die Ressourcen des Programmausführenden Systems zulassen, ein Attribut extra pro Bedinungsüberdeckung zu erstellen statt ein Attribut für alle, damit man dann auch Seiteneffekte verhindert. - 2. Als Tip vielleicht nochmal anschauen, wie der Lebenslauf von Variablen sind. Die Wahheitstabelle übergebt ihr einer Methode. Diese Methode nutzt nun diesen übergebenen Wert und dekonstruiert ihn im Anschluß nach Abschluß der Methode. - 3. prüfeWahrheit() hat als Rückgabewert eine Tabelle, und mit diesem zurückgegebene Wert wird nichts weiter gemacht. Also lasst ihr die Methode was berechnen und zurückgeben, ohne damit dann weiter zu arbeiten - 4. Wenn möglich, niemals übergebene Parameter modifizieren. Erstellt einfach ein neues Objekt und modifiziert diese. Erleichtert auch die Wartbarkeit, falls die Methode nocheinmal überarbeitet werden muss, und man dafür ggf diese übergebenen Werte brauch. In einer kleinen Methode ist dies vielleicht noch nachvollziebar, sollte diese Methode aber komplexer werden, erkennt man ggf Fehler erst später, da man nicht gesehen hat, das die Parameter modifiziert wurden. Apropos Wartbarkeit: ---------------------------- Es fehlt komplette Java OrdnerStruktur "Convention over Configure" also hier src/main/java/ -> hier die Main.java src/main/ressoruces -> hier die Dateien, die extern für das Programm benötigt werden (z.B. ex01.md) src/test/java/ -> hier der Ordner für jede Klasse (in gleicher Ordnerstruktur) die getestet werden src/test/ressources -> hier alle benötigten externen Files für das ausführen der Tests damit hat man auch nicht das Problem das man ohne Single Point of Controll den ganzen Code nach absoluten Pfaden durchsuchen muss, um diese zu ändern. man könnte dann Relative Pfade von dem Projekt selber nutzen, damit jeder damit arbeiten kann ohne anpassen zu müssen ---------------------------- Als nächsten Tipp für die Struktur von Projekten: Wenn man versucht zu erklären, was eine Klasse macht, und dort die Wörter "und" oder "oder" nutzen kann, sollte man überlegen, diese Klasse weiter zu splitten, da diese dann meistens mehrere Verantwortungen haben. In diesem Fall habt ihr hier eine Klasse Bedinungsüberdeckung, 1. welche aus einem MD File eine Wahrheitstabelle ausliest 2. UND entscheidet, was zu tun ist, wenn eine CoverageMethode ausgewählt wurde 3. UND welche die MMBUE Bedinungsüberdeckung auf eine Wahrheitstabelle anwenden kann 4. UND welche die MCDC Bedingungsüberdeckung auf eine Wertetabelle angewendet wird 5. UND das Ergebnis dann wieder in eine Datei gespeichert Das bedeutet das diese Datei ziemlich viele Verantwortungen hat. Das hat mehrere Nachteile: Wenn man nun in einem Team arbeitet und jeder implementiert davon nun 1 Verantwortung, muss bei Fehlern erst geschaut werden, wer dafür Verantwortlich ist (Bedinungsüberdeckung in Zeile 921 ist nicht klar). Dann müss geprüft werden warum, 1 Grund könnte dann ein Seiteneffekt sein oder weil 2 verantwortliche zur Bearbeitung eine weiteren Klassenattribut erzugen von gleichen Namen gesetzt haben und beide dann darauf gearbeitet haben Daher ist es ratsam auf Verantwortungen in mehrere Klassen auszulagern, damit sowas vermieden werden kann. -> 1 Klasse 1 Verantwortung von 1 Person umgesetzt. ---------------------------- Zurück zum Code: Namensgebung eurer AusgabeDateien ist unglücklich gewählt wenn die Dateien "ex3.md", "ex2.md", "ex1.md" in dieser Reihenfolge geprüft werden, dann bekommt - die "ex3.md" als Ausgabe z.B. "mcdc coverage1.md" - die "ex2.md" als Ausgabe z.B. "mcdc coverage2.md" - und die "ex1.md" als Ausgabe "mcdc coverage3.md" ---------------------------- entweder deutsch oder englisch (Methodennamen und Parameternamen) bsp throwExc und vergroessereArray ---------------------------- Apropos: - Was ist der Gedanke der Nutzung von ThrowExc? - Auslagerung der Fehler mit Message nicht unbedingt sinnvoll, dann kann man an der Stelle auch direkt die Exception schmeißen. Hat auch den Vorteil, den Text individuell aussagekräftiger zu konstruieren - "Da ist ein Datei-Fehler" ist nicht aussagekräftig. Besser wären Nachrichten in dem Format "Einlesefehler: Beim Einlesen der 4. Zeile der Datei *xy* ist ein Fehler aufgetreten" oder "Einlesefehler: Konnte Datei *xy* nicht finden" ---------------------------- Dann haben wir uns die Tests angeschaut: 1. Was ist der Gedanke Testumgebung auszusorcen in init(int)? Man will sehen wie bei einem Test die Testumgebung aufgebaut wird um dann mit Methodenaufruf zu verlgeichen um nachzuvollziehen, was wie verglichen wird. ein *init(1)* ist da eher kontraproduktiv, da nicht leicht erkennbar ist, was wie kontrolliert wird. - also init und direkt eine assert-Zeile ist nicht leicht leserlich und nachvollziehbar es sollte leserlich sein, was für eine Testumgebung aufgebaut wird, sollte ein Informationsaufruf schwieriger ugesetzt sein, kann eine Hilfsmethode gebaut werden, umd das leserlich umzusetzen 2. Ihr habt nur wenige Tests geschrieben, die eine nicht so hohe Codecoverage haben (66 method / 63 linecoverage) und z.b. das einlesen nicht prüfen oder das schreiben // bitte nochmal prüfen wie hoch genau! 3. ihr hab einen Test geschrieben und ihn so definiert, das 2 andere Tests durchgeführt werden? natürlich ist der true, wenn die anderen beiden auch true sind und natürlich ist der false, wenn einer der tests false sind, also macht der Test ansich keinen Sinn 4. Prüfung auf Fehlerwurf ---------------------------- Dann haben wir noch eine src.zip gefunden, die den Verdacht gab, das ihr ohne Versionierungstool wie Git gearbeitet habt. Großer Tip: Sich echt nochmal damit auseinandersetzen, damit ein Arbeiten in einem Team deutlich leichter wird ohne Zipdatei verschieben und händisches Copy Paste daraus! ---------------------------- Konstruktor mit datei und danach ladedatei mit gleichem warum? ladedatei gibt tabelle zurück die wieder reingeschoben wird zur weiteren bearbeitung auf dem gleichen objekt -> das objekt könnte sich das dann auch selber vorhalten