Java-Version/ Java-Sprache

SlaterB · 10. April 2015 um 12:32

welche Art von Java-Sprache soll der Quellcode des Forums sprechen, mit welcher Java-Version gearbeitet?

Code a la


        Optional<Session> session = Optional.ofNullable(context.getSession());
        Optional<FlashScope> flashScope = Optional.ofNullable(context.getFlashScope());

        return session.
                flatMap(userService::findUserBySession).
                filter(User::isActive).
                filter(user -> user.isAdministrator() || user.isModerator()).
                map(user -> filterChain.next(context)
                ).orElseGet(() -> {
            flashScope.ifPresent(f -> f.error("security.noAdminAuthorization"));
            return Results.redirect("/errors");
        });

wie viele verstehen den hier im Forum?
also im Sinne einer Java-Muttersprache, eine Methode auf einen Blick erkannt,

nicht im Sinne einer Java-Fremdsprache „… die erste Zeile macht also ungefähr das … 3 Min. später, nachschlagen, erinnern … die zweite Zeile wandelt das zu … und dann … usw.“

Streams und Filter sind legitime Elemente, gibt es in Java sein Anfang an (IO), in J2EE SessionFilter usw. auch schon ewig,
bewußt für wichtige Dinge eingesetzt ganz normal,

dass es das auch für normale Listen gibt ist in Ausnahmesituationen, wo man es fast extra nachbauen würde, auch in Ordnung,
ist aber nicht das Brot der täglichen for-Schleifen in Java, in jeder Simpel-Methode, je nach Herangehensweise

im ganzen Java-Insel Buch ist davon nicht die Rede,

im JUnit-Github-Projekt gibt es das nicht, falls das als gewisse Referenz zählt,
heute in Jahre 2015 noch etwas was im Grunde jeder Einsteiger der Sprache in Version 1 vor 20 Jahren sofort lesen kann,
nicht zu unterschätzender Vorteil

muss das hier ein derartiges Pionier-Projekt sein,
obwohl keine allgemein Tutorials/ öffenliche API wie Hibernate usw. derartiges verwenden/ dem User aufdrängen?
hier in einem vermeintlichen Community-Projekt für viele?

wieviele Foren-Probleme/ Fragen wurden bisher zu solchen Code gestellt?

natürlich schade und frech, den einzigen Programmierer Landei vor dem Kopf zu stoßen, ohne ihn wäre ja der Ofen gleich ganz aus,
aber ich fürchte mit solchen Code wird es ein Projekt von Landei und vielleicht wenigen einzelnen Profi-Interessierten in diese Richtung alleine,
deswegen früh zu entscheiden/ zu bedenken

für mich ein großes Hindernis, selbst wenn mit der Zeit zu lernen (wie PHP und anderes) kein Interesse in eine quasi andere Sprache zu wechseln,
das sind ja nicht nur einzelne Befehle sondern alles drumherum, Breakpoints setzen, System.out.println()-Ausgaben zwischendurch, StackTrace,

verschiedene Anteile des Forums in verschiedenen Dialekten klingen auch nicht attraktiv

wer hier im Forum gewisses Interesse zeigt, bitte mit Meinung melden, welche Art von Code soll herauskommen?,
aber falls noch eine andere Richtung, ginge das natürlich maximal hin zur Eigenüberzeugung von Landei,
vielleicht bisher nur auf die Schnelle so umgesetzt und flexibel? bei so viel Scala aber kaum vorzustellen ;(

edit: vielleicht sollte das hier gleich ein Scala-based Forum werden?

in Java könnte ein anderes entstehen, verschiedene parallel im Wettstreit wäre lustig ;),
aber wie zuvor umso mehr zu befürchten dass schlicht gar nichts passiert,
ich habe kaum die Zeit, außer für solche nervig langen Postings

mit jforum - Powering Communities als Grundlage allerdings…, solange Spielerei wäre auch Code klauen noch nicht schlimm, hmm…

system · 10. April 2015 um 15:01

So wie ich es verstanden habe Java8.

Die Filter haben mir auch noch nicht so großartig gefallen, da es 3 Filterklassen sind die nahezu identisch aussehen. Das würde ich, wenn ich es mal mit der DB zum laufen bekomme mal ausprobieren, ob ich dies geschickter hinbekomme.

Aber es ist ja durchaus möglich einen dieser bislang 3 Filter java7 konform zu erstellen. Dann sieht man auch den Unterschied und kann entscheiden, wie man in Zukunft programmieren möchte.

Vieles das in die Sprache einzug hält ist über Jahre hinweg entstanden. 2007 habe ich das erste mal davon gehört das Lambdas kommen sollen und es hat sehr lange gebraucht, bis diese letztendlich verfügbar waren.

Optional ist einfach da, bzw. ein Versuch mit den ganzen NPE zum Ende zu kommen.

Optional als Rückgabewert der Methodensignatur heißt für den Aufrufer, “Achtung, hier kann ein null zurückkommen”.
Und erzähl mir niemand, dass seine Software noch nie eine NPE im Produktivbetrieb geworfen hat. Schöner wäre es, wenn also context.getSession() ein Optional statt einem möglichen null zurückgeben würde. context.getFlashScope() entsprechend

Aber ich hab den Schnippsel mal umgeschrieben in Java7 konformes Java

if(session != null){
  for(User user : userService.findUserBySession()){
    if(user.isActive()) {
      if(user.isAdministrator() || user.isModerator()){
        return filterChain.next(context);
      }
    }
  }
} 
FlashScope flashScope = context.getFlashScope();
if(flashScope != null) {
  flashScope.error("security.noAdminAuthorization");
}
return Results.redirect("/errors");

Hilft der Entscheidungsfindung und weiteren Diskussion bestimmt weiter.

Landei · 10. April 2015 um 15:40

@SlaterB : Natürlich sieht das erstmal ungewohnt aus, es ist aber nicht wirklich schwierig. Was es vielleicht schwierig macht ist der Versuch, dem Code-Schnippsel mit der API-Doc auf die Spur kommen zu wollen, weil da kaum etwas zum Konzept erklärt wird - wie wenn man eine fremdsprachigen Text mit einem Wörterbuch entziffern will, aber die Grammatik nicht kennt.

Fast alle der von mir verwendeten Aufrufe sind Ketten von Optionals, mit gerade einmal 3 Transformations-Methoden (map, flatMap, filter, die im Übrigen genau wie in den Collection-Stream-Klassen funktionieren). Dazu kommen noch die terminalen Methoden ifPresent und orElseGet.

Konventionell - mit Null-Checks statt Optional - stände da einfach:

Session session = context.getSession();
FlashScope flashScope = context.getFlashScope();

User user = userService.findUserBySession(session);
if (user != null && user.isActive() && 
   (user.isAdministrator() || user.isModerator())) {
   return filterChain.next(context);
}

if (flashScope != null) {
   flashScope.error("security.noAdminAuthorization");
}
return Results.redirect("/errors");

Die Frage ist: Wie oft musst du hier drüberschauen, um dich zu vergewissern, dass nirgendwo eine NPE fliegen könnte? Bei meinem Code ist das keine Frage, oder besser gesagt, sie wird vom Compiler beantwortet.

Optional<A> kann man sich so vorstellen, als ob es zwei Unterklassen gäbe: Eine für den Fall, das kein Wert da ist (also eine Art Null-Ersatz) und eine, die nur ein einfacher Wrapper um einen Wert ist (in Scala und Haskell ist die analoge Klasse auch so implementiert, in Java leider nicht). Was man gerade vor sich hat, kann man mit der Methode isEmpty testen

Nun zu den Methoden:

map transformiert einen Wert “innerhalb” eines Optional mit der der gegebenen Funktion:

Optional<String> bla = Optional.of("bla");
Optional<Integer> len = bla.map(s -> s.length()); //len enthält den Wert 3

Wäre bla dagegen leer gewesen, wäre logischerweise auch len leer.

Manchmal hat man aber eine Transformations-Funktion, die nicht einen “einfachen” Wert liefert, sonder selbst ein Optional<X>. map würde dann ein Optional<Optional<X>> liefern - und das ist kaum das, was man will. Hier kommt flatMap ins Spiel, es klopft das geschachtelte Optional<Optional<X>> sozusagen zu einem einfachen Optional<X> “flach” (daher der Name). War das Optional vorher schon leer, bleibt es leer (wie bei map). Auch wenn die Funktion ein leeres Optional liefert, ist das Ergebnis leer.

filter wandelt einfach “volle” Optionals, die den übergebenen Test nicht bestehen, in “leere” um.

Will man jetzt einen Wert aus einem Optional holen, kann man get() benutzen, allerdings fliegt einem das um die Ohren, wenn man das Optional leer ist. Deshalb endet die Kette oft mit einem orElseGet, das auch wenn möglich den Wert aus dem Optional holt, aber in “leeren” Falle stattdessen den übergebenen Default-Wert verwendet.

Die Syntax ist auch nicht viel anders als das, was wir jeden zweiten Tag mit einem StringBuilder anstellen, nur das halt einige Argumente Lambdas sind. Aber an die müssen wir uns sowieso früher oder später gewöhnen, und Optional ist eher eine der einfacheren Gelegenheiten dafür. Wie erwähnt begegnet uns ein ganz ähnliches Konzept bei den neuen Collection-Streams wieder, und spätestens da wird es selbst für den interessant, der seine Null-Checks aus unerfindlichen Gründen lieber von Hand erledigt.

SlaterB · 10. April 2015 um 23:52

mal abgesehen davon dass es kein gutes Argument ist, hier erst noch verwendete Sprachelemente in ihrer Funktion zu erklären (?!),

ist es schon interessant wie unterschiedlich die beiden geposteten normalen Codes sind,
findUserBySession() mal eine Liste, die evtl. leer ist oder ein einzelnes Objekt, evtl. null, kommt hier natürlich aufs gleiche hinaus

spätes edit: bezeichnend, dass man am Code nicht erkennen kann, ob findUserBySession ein Element oder eine Liste liefert die dann von den Filtern zusammengekürzt wird

Optional ist längst nicht so sehr das Problem wie das andere, aber die Verwendung hier auch schon recht fragwürdig, kann natürlich alles noch Prototype sein,
wenn hier, wie oft noch in der Anwendung wird auf Vorhandensein von FlashScope & Co. reagiert?

im LoggedInFilter und AdminAuthorizationFilter steht genauso

        Optional<FlashScope> flashScope = Optional.ofNullable(context.getFlashScope());

das will doch niemand lesen?

die drei Filter bestehen übrigens wie man sich denken kann aus 3x nahezu denselben Code, nur minimal in den User-Bedingungen unterschieden,
das ist ja schon arg verboten (aber Prototype)

zu dem Optional hier jedenfalls, eine erste saubere Version wäre natürlich

 
User user = context.getUserMandatory(); // schon intern Fehlerabbruch wenn nicht da, 
// evtl. auch gleich Active-Prüfung was immer das ist, 
// getUserActive() schließt Vorhandensein gleich mit ein, elegant kürzer benannt,
// und testet vielleicht etwas was sowieso überall benötigt wird,

// falls intern Session nicht da, dann dort Fehler in allgemeiner getSession()-Methode, hier gar nicht mehr erst geholt

if (user.isActive() && (user.isAdministrator() || user.isModerator())) {
   return filterChain.next(context);
}
 
flashScope.error("security.noAdminAuthorization");
return Results.redirect("/errors");

die nächste Form wäre, das noch Framework-unabhängiger zu formulieren, wobei Session schon zuvor gekürzt:

if (user.isActive() && (user.isAdministrator() || user.isModerator())) {
   return Command.ContinueFilter; // oder was auch immer allgemeines
}
 
addError("security.noAdminAuthorization"); // geht an Context, Flash oder anderes
return Command.Error;

noch allgemeiner wird es nur zu einer boolean-Methode

User user = getUserMandatory(); 
if (user.isActive() && (user.isAdministrator() || user.isModerator())) {
   return true;
}
 
addError("security.noAdminAuthorization"); // geht an Context, Flash oder anderes

// evtl. noch Hinweise auf gewünschte abweichende Fehlerseite setzen

return false;
}

spätes edit:


  setErrorIfNotAccepted("security.noAdminAuthorization");
  // andere Hinweise optional im Filter-System vorher angeben, könnte auch alles in Enum-Konstanten Standard-Felder sein

  User user = getUserMandatory(); 
  return user.isActive() && (user.isAdministrator() || user.isModerator()));
}

Null-Prüfungen und Optional-Unsicherheiten sind in so einer Methode recht störend
und programmweit meist unnötig oder nach Laden von Usern zu einer Id usw. schlicht zur Unterscheidung nötig,

ob man


        if (existingUser.isPresent()) {
            return false;
        }

        // neuen User anlegen

oder


        if (existingUser != null) {
            return false;
        }

        // neuen User anlegen```
schreibt kommt aufs gleiche hinaus..

aber wie gesagt, Optional in seiner Grundfunktion noch eine Spielerei, letztlich sowieso weitgehend zu kürzen,
gegen manche nützliche Verwendungen ist nichts einzuwenden

aber die verrückten Maps und flats und filter, wo einfach nur der aktuelle User geholt 
und simples `if (user.isActive() && (user.isAdministrator() || user.isModerator())) ` zu prüfen ist..

ThreadPool · 11. April 2015 um 00:11

[QUOTE=Landei]
Manchmal hat man aber eine Transformations-Funktion, die nicht einen „einfachen“ Wert liefert, sonder selbst ein Optional<X>. map würde dann ein Optional<Optional<X>> liefern - und das ist kaum das, was man will. Hier kommt flatMap ins Spiel, es klopft das geschachtelte Optional<Optional<X>> sozusagen zu einem einfachen Optional<X> „flach“ (daher der Name). […][/QUOTE]

Ich halte flatMap immer noch für einen ungünstigen Namen, weil eigentlich nichts flachgeklopft sondern verkettet wird.

Davon mal abgesehen, die öffentlichen Updates für Java 7 werden irgendwann diesen April eingestellt, für einen verlängerten Service wie z.B. kritische Bugfixes darf man pro Endnutzerinstallation 100$ hinlegen, Mindestabnahme sind 100 Lizenzen. Es wird also einige geben die auf Java 8 wechseln und dann natürlich auch langsam damit beginnen die neuen Features zu verwenden. Wer in der Softwareentwicklung arbeitet, unabhängig in welchem Zweig, muss sich ständig weiterbilden, egal ob man das als lästig oder anstrengend empfindet. Es geht nicht nur darum zu wissen wo die Reise hingeht, sondern auch um konkurrenzfähig zu bleiben und den Marktwert zu erhalten.

*** Edit ***

Das liegt aber auch an der Umsetzung von Methodenreferenzen. Normalerweise gibt ein User::isActive ein Predicate<User> zurück wenn man es zuweist, ohne Zuweisung kann der Compiler leider nichts damit anfangen. Ginge ersteres könnte man da auch so schreiben filter( (User::isActive).and((User::isAdministrator).or(User::isModerator)) ). Ich denke bei der Verwendung von neuen Features geht es auch darum zu schauen wie weit einen die Ideen in Java tragen, welche Schwächen die neuen Features haben, wie man damit umgehen kann und ab wann es richtig unangenehm wird damit zu arbeiten. Ich persönlich hätte mir sogar gewünscht das in einer komplett anderen JVM basierten funktionalen Sprache aufzuziehen (nicht Scala ;)).

SlaterB · 11. April 2015 um 00:31

funktionale Programmierung gibt es seit 30 Jahren ohne Durchsetzung, Scala seit 5 Jahren und länger

die Reise wird nie dahingehen dass man an jeder simplen Stelle im Programm statt

if (user.isActive() && (user.isAdministrator() || user.isModerator())) ```
auf einmal
```                flatMap(userService::findUserBySession).
                filter(User::isActive).
                filter(user -> user.isAdministrator() || user.isModerator())

schreibt?!,
das wird es nie geben oder wenn dann ist das jedenfalls nicht Java

vollkommen legitim auf so eine Art zu programmieren, nur ist ja wohl klar dass das ein fundamentaler Bruch ist

bisschen langweilig wiederholend, aber wie in einem gewissen Thema zuvor : welche Projekte weltweit setzen das ein?
welche allgemeinen Paper/ professionelle Tipps-Blogs usw. wie etwa
[JavaSpecialists 225] - Hiding Interface Methods
JavaFX FileChooser Example | Examples Java Code Geeks
Java 8 - 20 Examples of Date and Time API
was auch immer, verwenden so eine Sprache, wenn es mal nicht direkt um die neuen Features an sich geht,
sondern um normale Java-Probleme?

[QUOTE=ThreadPool;115460]
Das liegt aber auch an der Umsetzung von Methodenreferenzen. Normalerweise gibt ein User::isActive ein Predicate<User> zurück wenn man es zuweist, ohne Zuweisung kann der Compiler leider nichts damit anfangen. [/QUOTE]

die beiden Zeilen dazu, ob als filter in ein oder zwei Zeilen oder als if sind nicht das Problem, alles drumherum bis auf noch eine Fehlermeldung dagegen exakt gleich,
angefangen mit 3x dem Monstrum

            Optional<FlashScope> flashScope = Optional.ofNullable(context.getFlashScope());

zwei Zeilen hintereinander die schon soviel ausmachen, dass sie besser kaum mehr als an einer Stelle im Programm zu finden sein sollten

humanisiert auf

            FlashScope flashScope = context.getFlashScope();

schon eher öfters, der einfachste Nenner, abgesehen von Basisklasse und damit context. weg,
aber selbst das letztlich meist vermeidbar, im Hintergrund

Landei · 11. April 2015 um 04:41

Nur kurz zu den Filtern: Da man bei der FilterWith-Annotation nur eine Klasse ohne irgendwelche Zusatzinformationen angeben kann, braucht man leider alle drei Klassen. Allerdings könnten sie im Sinne von DRY eine gemeinsame Oberklasse haben, was ich auch gleich umsetzen werde.

  @SlaterB : Ich verstehe dein Problem nicht. Wie auch ThreadPool schon geschrieben hat, werden die (nicht mehr ganz) neuen Features bald zum nomalen Handwerkszeug gehören. Warum sollte man sie nicht verwenden und Erfahrungen damit sammeln?

Ich habe das Gefühl, dass du sehr an der Syntax klebst. Für mich sind Konzepte viel wichtiger, auch wenn man die Syntax nicht vernachlässigen kann. Ich mag das Gefühl, wenn ein Stück Code keine NPE schmeißen kann und auch in anderen Aspekten Wohlverhalten zeigt, nicht weil ich aufgepasst habe wie ein Schießhund, sondern weil es mir der Compiler garantiert. Typen sind kein notwendiges Übel, sie sind ein mächtiges Werkzeug, und Java als statische Sprache hat dieses Werkzeug bisher sträflich vernachlässigt. Je genauer wir das Verhalten über den Typ beschreiben, umso mehr Sicherheit kann uns der Compiler geben. Ja, das sieht erst einmal ungewohnt aus, und ja, vieles fehlt noch in Java 8 oder ist ungeschickt gelöst, aber ich sehe darin keinen Grund, die vorhandenen Möglichkeiten nicht zu nutzen.

Die ganze Situation erinnert mich an Java 5 mit seinen Generics - die Älteren hier werden sich vielleicht noch erinnern :grampa: Auch da gab es eine beachtliche Fraktion, die den Untergang der westlichen Zivilisation prophezeite, wenn man dieses Teufelszeug verwenden würde - es ging doch auch prima ohne. Heute muss man schon tüchtig Generics schachteln oder mit Wildcards jonglieren, um auch nur eine hochgezogene Augenbraue zu sehen.

SlaterB · 11. April 2015 um 05:03

wenn überhaupt die Framework-Filter, dann kann der Anschluss an eine Frameworkschnittstelle immer genau eine Klasse sein,
darin dann nach eigener Logik/ Konfiguration beliebige einzelne Dinge abfragen

aber das driftet ab und was ich dazu schreibe klingt schnell mal nach ‘so und nicht anders muss das hier umgesetzt werden’,
sind natürlich nur Ideen nebenher ;),

dass ich in den 4 Service-Klassen insgesamt 17x die Zeile
EntityManager entityManager = entitiyManagerProvider.get();
finde wäre nach einer gewissen Idee auch ganz schnell weggeräumt
-> nur an Queries arbeiten und an allgemeine load-Methode in Basisklasse schicken, bzw. createQuery als angebotene allgemeine Methode,
dann auch wieder beliebig austauschbar, auch für Testbarkeit, Mockbarkeit doch sicher wünschenswert

aber solche Code-Eigenheiten sind gar nicht das Problem des Threads hier, wahrscheinlich schon wieder zu voll als dass noch viele lesen,
wer immer, der/ die möge aber vor allem die Fragen des ersten Postings bedenken

Landei · 11. April 2015 um 05:41

[QUOTE=ThreadPool]I
Das liegt aber auch an der Umsetzung von Methodenreferenzen. Normalerweise gibt ein User::isActive ein Predicate<User> zurück wenn man es zuweist, ohne Zuweisung kann der Compiler leider nichts damit anfangen. Ginge ersteres könnte man da auch so schreiben filter( (User::isActive).and((User::isAdministrator).or(User::isModerator)) ). [/quote]

Ich habe es jetzt so gelöst, dass ich and und or als statische Methoden in org.fjorum.util.Predicates anbiete, man also sowas wie and(User::isActive, or(User::isAdministrator, User::isModerator)) schreiben kann - auch nicht komplizierter als Slaters if.

Ich denke bei der Verwendung von neuen Features geht es auch darum zu schauen wie weit einen die Ideen in Java tragen, welche Schwächen die neuen Features haben, wie man damit umgehen kann und ab wann es richtig unangenehm wird damit zu arbeiten.

Ich versuche mit meinen Util-Klassen die gröbsten Probleme auszubügeln. Bei manchen „Lücken“ in der API frage ich mich echt, was derjenige bei Oracle geraucht hat.

Ich persönlich hätte mir sogar gewünscht das in einer komplett anderen JVM basierten funktionalen Sprache aufzuziehen (nicht Scala ;)).

Wie wäre es mit Frege? Fehlt nur noch ein passendes Web-Framework

SlaterB · 11. April 2015 um 05:56

nur so nebenbei zu diesem Thema:

                filter(user -> user.isAdministrator() || user.isModerator())

-> filter(user -> user.isActive() && (user.isAdministrator() || user.isModerator())) ginge auch?
oder auch immer eine Variante: extra Methode filter(User::isActiveAdminOrMod)

diese ::-Schreibweise ohne Klammer (was eigentlich bei Parametern, was ist bei Aufruf an anderen lokalen Objekten, anderen User-Objekten, wieso die Klasse genannt?), ist aber generell auch etwas, was einen zu großen Bruch darstellt, IDE kommt mit Suche, Markierung usw. immerhin klar?

Landei · 11. April 2015 um 05:56

[QUOTE=SlaterB]
dass ich in den 4 Service-Klassen insgesamt 17x die Zeile
EntityManager entityManager = entitiyManagerProvider.get(); [/quote]

Anscheinend ist es keine gute Idee, den EntityManager selbst als Instanz zu halten: EntityManagerProvider vs. Provider of EntityManager · sclassen/guice-jpa Wiki · GitHub

→ nur an Queries arbeiten und an allgemeine load-Methode in Basisklasse schicken, bzw. createQuery als angebotene allgemeine Methode,
dann auch wieder beliebig austauschbar, auch für Testbarkeit, Mockbarkeit doch sicher wünschenswert

Das wäre schön, aber man braucht leider einen EntityManager, um überhaupt erst ein Query-Objekt zu bekommen.

Natürlich könnte man auch individuelle Function<EntityManager, Query> an eine generische Methode übergeben, die dann die eigentliche Arbeit ausführt - daran habe ich schon gedacht.

Momentan sprechen zwei Gründe dagegen:

Das Datenmodell wird sich noch stark ändern
Wenn das Datenmodell erst einmal halbwegs stabil ist, würde ich zu TypedQuery (oder einer Wrapperbibliothek, die das Ganze etwas verdaulicher macht) wechseln.

Deshalb ist der Service-Code auch ziemlich lieblos hingeschmiert, aber das soll definitiv nicht so bleiben.

Aber ich grüble nochmal drüber, vielleicht wird es doch Zeit für eine DAO-Schicht?

SlaterB · 14. Juli 2015 um 13:47

normale Typisierung hat schon was für sich

        return categoryService.getById(form.getCategoryId()).
                flatMap(category -> userService.getById(form.getUserId()). // category ist eine Category
                        map(user -> createNewTopic(category, user, form.getName()))).
                orElseThrow(() -> new DataIntegrityViolationException("Category or user not found"));
    }

    @Override
    public Topic createNewTopic(Category category, User user, String name) {
        Topic topic = new Topic(category, user, name);
        return topicRepository.save(topic);
    }

…

        return topicService.getById(form.getTopicId()).
                flatMap(category -> userService.getById(form.getUserId()). // category ist ein Topic!
                        map(user -> createNewReply(category, user, form.getContent()))).
                orElseThrow(() -> new DataIntegrityViolationException("Topic or user not found"));
    }

    @Override
    public Reply createNewReply(Topic topic, User user, String name) {
        Reply reply = new Reply(topic, user, name);
        return replyRepository.save(reply);
    }

die Fehlermeldungen sind mit ‘Topic or user not found’ schon in diesem leichten Stadium reichlich unpräzise,
ein normaler messerscharfer Aufbau a la

Topic topic = topicService.getById(form.getTopicId()).
if (topic == null) throw new DataIntegrityViolationException("Topic not found"));

wäre so schwer verdaulich?

ist der User bewußt nur aus dem Formular geholt, separat zum hoffentlich eingeloggten User?
theoretisch Formulare denkbar in denen ein Moderator ein Posting/ ein Thema für einen User X erstellt,
aber das muss ja nicht sein, schon gar nicht am Anfang,

ist die Flexibilität da, dass der User eher gar nicht im Formular vorkommt, sondern hier an die Session, an den eingeloggten User gedacht wird?

falls auch ein Stunde Pause und Session-Ende bedacht wird,
dann sollte der User eh bei jedem Request als Cookie oder sonstige allgemeine Info mitgeliefert werden und dessen Relogin/ Fehlermeldung vorher behandelt werden,
für Aktion wie Topic erstellen/ Reply erstellen/ einfach quasi alle (auch alle Mod-Aufgaben) besser nicht User in Formulare?

Landei · 14. Juli 2015 um 15:25

[QUOTE=SlaterB]normale Typisierung hat schon was für sich

die Fehlermeldungen sind mit ‚Topic or user not found‘ schon in diesem leichten Stadium reichlich unpräzise,
ein normaler messerscharfer Aufbau a la

Topic topic = topicService.getById(form.getTopicId()).
if (topic == null) throw new DataIntegrityViolationException("Topic not found"));

wäre so schwer verdaulich?[/quote]

Ja, wäre es es. In diesem Punkt bin ich beratungsresistent.

Optional ist an dieser Stelle „eigentlich“ der falsche Typ, was es schwer macht, mit Fehlern umzugehen. Korrekt wäre etwas wie Try aus try4j, aber wenn ich schon das Gejammer höre, wenn ich Optional benutze…

ist der User bewußt nur aus dem Formular geholt, separat zum hoffentlich eingeloggten User?
theoretisch Formulare denkbar in denen ein Moderator ein Posting/ ein Thema für einen User X erstellt,
aber das muss ja nicht sein, schon gar nicht am Anfang,

ist die Flexibilität da, dass der User eher gar nicht im Formular vorkommt, sondern hier an die Session, an den eingeloggten User gedacht wird?

falls auch ein Stunde Pause und Session-Ende bedacht wird,
dann sollte der User eh bei jedem Request als Cookie oder sonstige allgemeine Info mitgeliefert werden und dessen Relogin/ Fehlermeldung vorher behandelt werden,
für Aktion wie Topic erstellen/ Reply erstellen/ einfach quasi alle (auch alle Mod-Aufgaben) besser nicht User in Formulare?

Die userId stammt ja aus dem eingeloggten Nutzer. Ich habe noch nicht ausprobiert, was passiert, wenn man ein Formular nach Ablauf der Session absendet (ich müsste erst mal nachschauen, auf wie lang die Session überhaupt eingestellt ist). Ich schätze mal, das CRSF-Token bleibt auch nicht ewig gültig. Trotzdem wäre es eine Überlegung wert, den User innerhalb des Controllers zu ermitteln.

Landei · 14. Juli 2015 um 23:26

Ich habe nochmal scharf über die Fehlermeldungen nachgedacht, es geht in diesem Fall auch recht einfach mit Optional. Ich habe eine gewisse Abneigungen, Fehler mitten in einem Trainwreck zu werfen, aber an dieser Stelle geht es wohl in Ordnung:

public Reply createNewReply(ReplyCreateForm form) {
    return topicService.getById(form.getTopicId())
            .<Reply>map(category -> userService.getById(form.getUserId())
                    .map(user -> createNewReply(category, user, form.getContent()))
                    .orElseThrow(() -> new DataIntegrityViolationException("User not found")))
            .orElseThrow(() -> new DataIntegrityViolationException("Topic not found"));
}

Ich will wirklich keine null-Tests in meinem Code sehen, wenn es sich irgendwie vermeiden lässt. Das ist noch nicht an allen Stellen so, aber ich arbeite darauf hin. Optional hat seine Beschränkungen, aber es ist das beste, was wir zur Zeit haben, um null zu vermeiden. Allerdings werde ich versuchen, bestimmte Muster herauszufaktorieren, was die Länge und den Hässlichkeitsfaktor der Trainwrecks etwas senken wird.

SlaterB · 15. Juli 2015 um 02:14

in der ersten Hälfte meines Posting hatte ich übrigens darauf hingewiesen, was du nun selbst bei halbwegs intensiver Überarbeitung noch drinhast:

            .<Reply>map(category -> userService.getById(form.getUserId())
                    .map(user -> createNewReply(category, user, form.getContent()))
                    .orElseThrow(() -> new DataIntegrityViolationException("User not found")))
            .orElseThrow(() -> new DataIntegrityViolationException("Topic not found"));

die Variable ‚category‘ in der zweiten Zeile ist ein Topic!, nenne sie doch ‚topic‘ statt ‚category‘ wie im kopierten Code vom TopicServiceImpl

ein schönes Beispiel für völlig inakzepables fehlerträchtiges Programmierkonzept (wenn ich mal übertrieben auf den Tisch haue )

*** Edit ***

am funktioniellen Aufbau kann ich hier aber wirklich auch nur meckern und meckern

    return topicService.getById(form.getTopicId())
            .<Reply>map(category -> userService.getById(form.getUserId())
                    .map(user -> createNewReply(category, user, form.getContent()))
                    .orElseThrow(() -> new DataIntegrityViolationException("User not found")))
            .orElseThrow(() -> new DataIntegrityViolationException("Topic not found"));
}

bedenklich, dass in Zeile 4 von einem User die Rede ist von dem man zuvor nichts weiß,
in Zeile 3 wird er geladen, zum Glück userService mit ähnlichen Namen, sonst mächtig kompliziert um die Ecken zu denken,

edit: davor mit category = topic ja auch schon,
was soll eine Zeile map(category -> userService.getById(form.getUserId()) auch bedeuten?

das Linke (category/ topic) hängt mit dem Rechten (User laden) nicht zusammen, es findet gar kein Mapping oder so statt,
das Linke ist die Rettung der lokalen Variable der vorherigen Zeile,
das Rechte ist ein unabhängiger neuer einzelner Aufruf (mit Ergebnis wieder in nächster Zeile),
geht es noch verkorkster und Funktionalität noch stärker missbrauchend?

was das return letztlich ist, einer der vielen inneren Aufrufe, hier in der vierten Zeile, muss man lange suchen…

was nützt so eine kryptographische Programmierung?
mit den Einrückungen, verschiedene if/else Konstrukte (bzw. äquivalent orElseThrow),
mit den ->-Schreibweisen + map() hat man eine simple Methode gekonnt zu unverständlichen Schrott formuliert

man vergleiche dagegen normalen Code:

	Topic topic = topicService.getById(form.getTopicId());
	if (topic == null) throw new DataIntegrityViolationException("Topic not found"));
	
	User user = userService.getById(form.getUserId());
	if (user == null) throw new DataIntegrityViolationException("User not found"));
    
	return createNewReply(topic, user, form.getContent()));
}

in Sekunden für jeden Menschen sofort verständlich, 3 einfache Schritte,
zu zwei Ids die Objekte holen, eine bekannte Standardtätigkeit,

symmetrisch sauber aufgebaut statt im Funktionalfall kompliziert unterschiedlich eins davon mit map wer weiß wohin führend,
orElseThrow wer weiß wo weiter hinten

als letztes der einfache Aufruf der dritten Methode, klares sofort ersichtliches return,
simple übersichtliche fehlervermeidende Programmierung

Landei · 15. Juli 2015 um 02:47

Das “category” war natürlich ein typischer Copy-Paste-Fehler (aus TopicServiceImpl kopiert).

Dadurch, dass ich jetzt wie vorgeschlagen den User im Controller ermittle, sieht es inzwischen so aus:

@Override
public Reply createNewReply(ReplyCreateForm form, User user) {
    return topicService.getById(form.getTopicId())
            .map(topic -> createNewReply(topic, user, form.getContent()))
            .orElseThrow(() -> new DataIntegrityViolationException("Topic not found"));
}

Jetzt finde da mal ein Haar in der Suppe…

SlaterB · 15. Juli 2015 um 03:08

den Rest meines Postings zuvor mit Edits gesehen?
von der Wortwahl nicht stören lassen, zeigt nur meinen immer gleichen Wortschatz

dass ein solcher Paste-Fehler so einfach möglich ist zeigt eine Bedenklichkeit,
im Normalfall mit geänderten Typ würde man dies selten so lassen

noch kürzere Methoden als sowieso schon sind Glück, aber bleibt nicht immer so,
und strukturell ändert sich dadurch sowieso nichts,

den als letzen editieren Punkt wiederholend:
in der Zeile
.map(topic -> createNewReply(topic, user, form.getContent()))
hat das Linke mit dem Rechten nichts zu tun (edit: bzw. doch, bin noch bei früher),
das ist einfach nur Verkettung normaler Java-Zeilen (Topic laden, User laden, zwei unabhängige Schritte) in komplizierter Schreibweise
und selbst für Funktionalität eine Beleidigung, wage ich zu behaupten

lesbar jedenfalls ganz und gar nicht, im gesteigerten Sinne,
man kann nur versuchen, alle map() und sonstiges Syntax-Blahblah zu überlesen
und nur ‚topicService.getById‘ + ‚topic‘ irgendwann später wahrzunehmen und gedanklich in korrekte Reihenfolge zu bringen

oftmals gibt es denselben Aufbau für wirklich kompliziertes mit Listen, Maps, Sets, die durcheinandergewirbelt werden,
in x->y das x links auch tatsächlich ein benötigtiger Parameter in y,
all das hier nicht der Fall, aber nicht sofort ersichtlich, stattdessen simple Aufrufe genauso formuliert, böse böse

(edit: topic ist ja nun x->y-Parameter, Glück gehabt dass das User laden wegfällt (mein Tipp ), Wiederholung mehr noch auf altes Posting)

Landei · 15. Juli 2015 um 03:50

Wenn dich die Typinferenz hier stört, kannst du einfach .map((Topic topic) -> createNewReply(topic, user, form.getContent())) schreiben. Aber egal wie die Variable heißt, ihr Typ ist klar definiert, und es gäbe einen Compilierfehler, wenn ich sie rechts für einen anderen Typ einsetze. Dass der unbemerkte Copy-Paste-Fehler hier überhaupt möglich war, war dadurch bedingt, dass Topic und Reply strukturell sehr ähnlich sind: Überall wo Topic Category verwendet, verwendet Reply Topic - das hat man eher selten.

Ich denke, die Diskussion hier macht keinen Sinn. Du ziehst dich an der Syntax hoch, ich kann dir die entsprechenden Konzepte (vor allem, dass null Teufelswerk ist) anscheinend nicht vermitteln.

SlaterB · 15. Juli 2015 um 04:43

man kann es so schreiben, du tust es nicht,
die Möglichkeit dazu, es wegzulassen ist fatal, entsprechende flächendeckende Programmiersprachen unterlegen

null Teufelswerk oder nicht wurde vor Jahren mal diskutiert, aktuell noch nicht allzu viel dazu
bin zumindest froh dass bisher noch

    @Column(name = "user_name")
    private String name;
[..]
    @Column(name = "email")
    private String email;

in den Entities enthalten ist und nicht alles mit Optional zugepflastert…,

fraglich ob zu halten wenn erstmal email für Mail-Versenden gebraucht wird usw.

Landei · 15. Juli 2015 um 06:49

[QUOTE=SlaterB]man kann es so schreiben, du tust es nicht,
die Möglichkeit dazu, es wegzulassen ist fatal, entsprechende flächendeckende Programmiersprachen unterlegen[/QUOTE]

Aber das ist doch Käse, der Typ des Arguments ist doch trotzdem eindeutig bestimmt (bzw. der Compiler beschwert sich, wenn es nicht so ist). Es gibt dort keinen Unterschied(*), egal ob du da ein Lambda oder eine anonyme Klasse schreibst.

(*) Ja, ich weiß, im Bytecode wird beides unterschiedlich realisiert, aber das ist nur eine Performance-Optimierung.