Bruk tilstandsfunksjoner i servicer #388
Conversation
|
Testet OK. |
|
|
||
| RedisKey.of(fail.transaksjonId, Key.INNTEKT).write(JsonObject(emptyMap())) | ||
|
|
||
| // TODO bruk finalize (sjekk andre servicer for tilsvarende feil) |
There was a problem hiding this comment.
Skjønner ikke denne helt - hvorfor kalles inProgress her, og hva med TODO'en
There was a problem hiding this comment.
Legger til TODO-en fordi jeg ikke ønsket å endre for mye av oppførselen til servicene med denne PR-en, men heller fokusere på det som spinner under panseret. Så inProgress kalles fordi det var original oppførsel. Se https://github.com/navikt/helsearbeidsgiver-inntektsmelding/pull/388/files#diff-babcc7677280f328041f0947da9bc0f396dd0deb964b07106d1d30d7c49ebf1bL219-L249.
There was a problem hiding this comment.
Ah, skjønte det nå - litt vanskelig å lese diffen
| val arbeidstakerKey = RedisKey.of(feil.transaksjonId, Key.ARBEIDSTAKER_INFORMASJON) | ||
| redisStore.set(arbeidstakerKey, ukjentArbeidstaker().toJsonStr(PersonDato.serializer())) | ||
| return Transaction.IN_PROGRESS | ||
| private inline fun medTransaksjonIdOgForespoerselId(message: JsonMessage, block: (UUID, UUID) -> Unit) { |
There was a problem hiding this comment.
Virker litt omfattende, samt litt voldsomt å duplisere denne metoden - kunne man heller hatt en felles metode?
Evt helst en hentTransaksjonId() og en hentForespoerselId() fra message, dette bør være en ganske vanlig ting som gjøres mange flere steder også
There was a problem hiding this comment.
For transaksjonId så leser man alltid fra melding, men forespoerselId blir av og til lest fra Redis. Da tenker jeg at fremgangsmåten du beskriver blir mer hensiktsmessig ved en senere aneldning.
Jeg ønsker nemlig å forenkle servicene ved å merge verdier fra innkommende meldinger og fra Redis, slik at man slipper å bry seg om det i koden. Når det er gjort vil en hentForespoerselId-funksjon være enklere å ta i bruk.
There was a problem hiding this comment.
Fint å dele opp i disse abstrakte supermetodene, det blir noe enklere flyt og lesbarhet - men fortsatt er dette "det samme" pattern som før, altså servicer med state og noen litt mystiske super- og delegerTil-klasser...
Jeg tror at det ville vært mulig å gå bort fra, eller i det minste drastisk redusere bruken av servicer med state i sin nåværende form - synes at vi bør ta en fot i bakken alle sammen for å se på alternativer før vi evt fortsetter å forbedre / videreutvikle et litt broken design. Ikke at denne endringen gjør det verre altså, på ingen måte. :)
@mortenbyhring Jeg er enig, men en ny løsning vil ta tid. Jeg tenker at noen endringer på servicene er verdt investeringen, mens vi venter på noe annet. Jeg tror også noen av endrginene jeg har nevnt vil gjøre det lettere å skrive seg bort fra servicene. |
Bytter til tilstandsfunksjoner for å ikke miste kontroll over flyten i servicene. Overgir altså ikke lenge kontrollen til CompositeEventListener.
Diffen er bedragersk stor. Det er en god del som kun er endring av indensjon, men det har ikke git klart å fange opp skikkelig.