Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notes code review #30

Open
ModellbahnFreak opened this issue Sep 7, 2022 · 11 comments
Open

Notes code review #30

ModellbahnFreak opened this issue Sep 7, 2022 · 11 comments

Comments

@ModellbahnFreak
Copy link
Contributor

For notes that we receive during he review

@ModellbahnFreak
Copy link
Contributor Author

ModellbahnFreak commented Sep 7, 2022

Doku

  • Übersichtsdiagramm
    • Welche Kompoonenten gibts, wie hängen die zusammen
  • Readme in submodulen für Infos beim durchklicken
  • Alles sehr technisch dokumentiert
    • Maintainen schwer aus Fachlicher sicht, weil Module nicht sofort kla
    • Mehr Doku auf fachlicher ebene
    • Beispiele
    • "Wie sieht ein sync konkret aus"
    • Ohne einblick in Metamodell hätte er es nicht verstanden
    • Wäre cool
    • Mehr als nur "was ist es" sondern auch "wofür wird es eingesetzt"
  • Es fehlen Informationen was technische voraussetzung ist
  • Klassendiagramm ist unübersichtlich aber gut dass es existiert
    • Wäre ggf. hilfreich das in Doku zu haben
    • Wenn es Leuten zu unübersichtlich ist, dann benutzen die das nicht
    • Ggf. in getrenntem Repo
  • Review Doku selbst war enorm unhilreich
    • Liste an repos ist unübersichtliuch
    • Keine Reihenfolge in der man sich durcharbeiten soll
    • Speziell fürs Review eine Review-Doku
    • Ggf. Mit Anmerkungen wer wo hin schauen soll wenn es viel code ist
  • Doku Gropius-backend ist eine Zeile (readme)
    • Ist nicht unser ernst
    • VErweist auf Dokusaurus
    • VErdient das Wort Doku nicht
  • Doku sagt, es ist model driven
    • Ist vmlt mit annotationen
    • Nochmal dokumentieren, was da model driven ist
    • Auch Annotationen sind schwierig zu verstehen wenn man sich den code anschaut
    • Auch nochmal ein Beispiel dokumentieren damit man das versteht
  • Abkürzungen noch irgendwo dokumentieren
    • Erleutern
    • Für einsteiger schwierig
  • Unter API=>properties überschneiden sich Dinge
  • Gropius ist manchmal klein geschrieben

@ModellbahnFreak
Copy link
Contributor Author

ModellbahnFreak commented Sep 7, 2022

Deployment

  • Hat nicht getan weil kein Java 17
  • Es haben Leute in IntelliJ ausgeführt => MUSS funktionieren!!!!
  • Mit gropius Repo ging deploy gut und die readme hat gereicht
    • Besser hervorheben: Submodule müssen gepullt werden
    • Gitkraken tut das nicht

@ModellbahnFreak
Copy link
Contributor Author

ModellbahnFreak commented Sep 7, 2022

Graphglue

  • Struktur der Graphglue Doku unhilfreich
    • Kommt erst deploy und technischer Krams, nicht "was tut ds ding, warum ist es toll"
    • Damit Leute wissen ewas sache ist warum sie es nutzen wollen
  • Beispiel in der graphglue doku ist massiv misslungen
    • Redet von einem Baum mit Wurzen
    • Schwierig zu verstehen
    • Besseres Beispiel z.B. Kunde mit Einkaufswagen
    • Klassisch mit UML-Diagramm mit Multiplizitäten, Stereotypen
    • Dann Code-Beispiel wie man das in Code umsetzt
  • Auch noch ein Beispiel für nen Client
    • "Ich hab jetzt Code geschrieben, wie bekomm ich die Infpormationen aus dem Server raus"
    • Was passiert, wenn ich keine Permissions hab
    • Was muss ich überhaupt noch an Code schreiben um die API da raus zu bekommen
  • Es feht doku warum es graphglue gibt und was es für Alternativen gibt
    • Kann man anhand des Beispiels machen
    • "Jede blöde Relation muss man von Hand coden => hier ist die Lösung"

@ModellbahnFreak
Copy link
Contributor Author

ModellbahnFreak commented Sep 7, 2022

Tests

  • KEin einziger automatisierter test
    • Ist ein must have wenn andere Leute das maintainen wollen
    • Hilft auch fpr versatändnis
    • ISt schwierig sich ohne einzuarbeiten
  • Gibt keine richtige CI
    • Deployed nur Doku
    • Auch kompilieren, schauen, dass tests laufen
    • Hilft auch bei Sachen wie z.B. eigenem deploy

@ModellbahnFreak
Copy link
Contributor Author

ModellbahnFreak commented Sep 7, 2022

Doku 2

  • Es fehlt an Doku für LEute die nicht mitgearbeitet haben
  • In Github bei ALLEn Projekten noch readmes/abouts und jeweils Link zur Doku
    • Halbsatz und Link zur Doku ist ein bisschen knapp

@ModellbahnFreak
Copy link
Contributor Author

ModellbahnFreak commented Sep 7, 2022

Low level graphglue

  • Gib ne Methode "generateconnectiongraph"
    • "generateconnectionfielddefiniteion ZEile 82
    • Wenn Lambda größer ist, dann Return hinschreiben, damit klar
    • Wenn in Kotlin schwierig, dann ggf in ne Helpermethode
    • Mindestens mal ne Leerzeile absetzen
    • Ist geschmackssache
  • Doku "works in almost all cases"
    • Kiene infos WANN es nicht funktioneiert
    • Wird dadurch negative informationen
  • Gibt nen Hinweis auf graphiql
    • Nicht klar was das ist
    • Link
    • Screenshot von dem entsprechenden (zu erstellenden) Beispiel
  • Doku unter "Moddeling" zu Relations ist total unverständlich
    • Gibt nen Abschnitt Relations ohne Text
    • Dann zwei Unterabschnitte die sagen, was das eine und wqwas das andere kann
    • Dann ne comparison
    • Nicht klar was dass sagen soll
    • "Hab ich da Optionen beim Programmieren"
    • Wäre wichtig, die Relationen einzeln durchzugehen
      • Klar machen wsa das ding damit macht
      • Wie kann ich fdas konigurieren
      • Was für Queries kann ich am Ende dagegen laufen lassen
  • Gibt am Ende ne Page "ContextProperties"
    • "Wtf"
    • Keine verständliche Erklärung was das bedeuten soll
  • Gerade das Graphglue muss super dokumentiert sein
    • Wenn man das maintainen will ist das ja das wichtige
  • Im code ist graphglue gut dokumenteirt (kein nicht dokumenteirter teil)
  • Verwirrend: int as int+1 => auf die schnelle nicht gepeilt, was das macht
    • Magic number
    • Irgendwo im graphglue core (execution)
    • nodequerparser Zeile 359
    • In ne Konstante oder Methode auslagern dass man das dokumentieren kann
    • Z.B. extension function => erhöht lesbarkeit
    • BEschreibt feature

@ModellbahnFreak
Copy link
Contributor Author

ModellbahnFreak commented Sep 7, 2022

Gropius-backend & Graphglue

  • Klasse Artefact
    • Annotiert
    • Manches kommt ok vor
    • Meistes durch doku von graphglue verstanfden
    • Uri ist als ordered angegeben => nicht sinnvoll?
    • Aus Dömänensicht nicht verstanden
  • Optinal Properties
    • Die waren auch ordered unf filterable
    • Nicht thematisiert, was mit optinal properties passiert wenn optinal properties sortiert und gefiltert wird
    • Nichts was irgendwie sinn macht
    • Vlt fehlt auch in graphglue doku was passieren SOLL
  • Braucht man Klassen aus Domänenmodell nochmal an anderer Stelle?
    • Oder nur um Data transfer objects zu erzeugen
    • Nicht klar, weil nie nen Client gesehgen
    • Macht Abfrage und bekommt Bäume und Wurzeln zurück
    • Wenn VErwendung wo anders dann sind die Konstruktoren Horror
    • => Builder machen, für jedes Objekt
    • Regel immer nur ein PArameter
    • Wenn das rein bei uns drinnen ist, dann kann man damit leben
    • "Wie steuere ich das Ding überhaupt an?"
    • Muss ich das dto im client dann mit den vielen Parametern erzeugen
    • => Mal ein Beispiel wie nutze ich das
    • ABER: Man generiert da ja ein Graphql schema und VErwendung hängt dann von Client GRaphql Library ab
    • Irgendwo hängt client ja mit graphglue zusammen
      • Man muss dann beschreiben wie aus der internen dsl das schema erzeugt wird
      • 2-3 Hinweise wären Hilfreich
  • Kann man nicht nen Anatz oben drauf setzen wo man ein graphisches Modell baut und dann den Backend Code draus generiert
    • Versuch aus Code Modell zu reverse engineering zu machen
    • Z.B. Emf, uml, ...
    • Wenn Code single source of truth sein soll und kein grafik input, dann wäre gut, die Möglichkleit zu haben eins zu generieren
  • Komplettes Gropius ist MAXIMAL an eine Datenbanktechnologie gekoppelt
    • Früher Datenbank in Mitte
    • Hat sich mit DDD nicht bewährt
    • NE Anmerkung, dass Domänenmodell unabhängig von allen Annotationen verwendet werden kann
    • Domönenmodell stärker herausarbeiten
  • Code Qualität ist gut
    • Bis auf fehlende unit tests
    • Kaum Probleme zu finden
    • Teils sogar ZU VIEL dokumentiert
      • z.B. Wrapper etc. ggf. nicht dokumentieren

@ModellbahnFreak
Copy link
Contributor Author

ModellbahnFreak commented Sep 7, 2022

Gropius backend public/internal API

  • Es steht bei internal, dass sie NICHT veröffentlicht werden soll
    • Auflokalem System könnte man auf die API zugreifen
    • Auf dev system nicht relevenat
    • Auf production ggf. sicherheiotslücke
    • Mit Auth token o.ä. absichern
    • Gewundert das zu sehe

@ModellbahnFreak
Copy link
Contributor Author

Github submodule

  • PAckage struktur unübersichtlich
    • zwei packages, fast alle Funktionalität ineinem Package
    • Mehr Struktur, dass man besser durchsteigt
    • Manchmal schwierig, aber für Übersicht
  • In den Models für Mongo ist jeder PArameter mit @ Indexed annotiert
    • Für jeden Parameter wird index erzeugt
    • Mehr Storage space
    • Kann auch langsam machen
    • Schauen, ob man alle Indices WIRKLICH braucht
    • Vorsichtig sein

@ModellbahnFreak
Copy link
Contributor Author

ModellbahnFreak commented Sep 7, 2022

Gropius BAckend

  • Properties mit Annotationen voneinander abtrennen damit klarer wie viele Properties
  • In core=> src=>model=>componentet steht "ist THE Template"
    • Wir haben aktuell viele Templates, spezifizieren
  • Gleich Klasse gib nen Attribut URI
    • Aber Attribut heist url => ähnlich, warum
  • ? und ? sind im Paket Architektur
    • Eher im Paket Issue erwartet
    • Kann man sich drüber streiten
  • core=>src=>authorization gabs ne defensive Prüfung mit IllegalArgumentException
    • Sollte IllegalStateException sein? Weil kein direkter Parameter geprüft wird

@ModellbahnFreak
Copy link
Contributor Author

Login Service

  • fehlt doku
    • Manchmal gibt es welche, meistens fehlt sie
  • Regex match für Pfade schwer verständlich, lieber in Variable auslagern

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant