#190 Registrere hendelser i cpa-repo #138
Conversation
OleksandrChmyrNAV
changed the title
| package no.nav.emottak.cpa.util | ||
|
|
||
| import no.nav.emottak.cpa.log | ||
| import no.nav.emottak.message.model.ValidationRequest | ||
| import no.nav.emottak.utils.common.parseOrGenerateUuid | ||
| import no.nav.emottak.utils.kafka.model.Event | ||
| import no.nav.emottak.utils.kafka.model.EventType | ||
| import no.nav.emottak.utils.kafka.service.EventLoggingService | ||
|
|
||
| interface EventRegistrationService { | ||
| suspend fun registerEvent( | ||
| eventType: EventType, | ||
| validationRequest: ValidationRequest, | ||
| eventData: String = "{}" | ||
| ) | ||
| } | ||
|
|
||
| class EventRegistrationServiceImpl( | ||
| private val eventLoggingService: EventLoggingService | ||
| ) : EventRegistrationService { | ||
| override suspend fun registerEvent( | ||
| eventType: EventType, | ||
| validationRequest: ValidationRequest, | ||
| eventData: String | ||
| ) { | ||
| log.debug("Registering event for requestId: ${validationRequest.requestId}") | ||
|
|
||
| try { | ||
| val requestId = validationRequest.requestId.parseOrGenerateUuid() | ||
|
|
||
| val event = Event( | ||
| eventType = eventType, | ||
| requestId = requestId, | ||
| contentId = "", | ||
| messageId = validationRequest.messageId, | ||
| eventData = eventData | ||
| ) | ||
| log.debug("Publishing event: $event") | ||
|
|
||
| eventLoggingService.logEvent(event) | ||
| log.debug("Event published successfully") | ||
| } catch (e: Exception) { | ||
| log.error("Error while registering event: ${e.message}", e) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| class EventRegistrationServiceFake : EventRegistrationService { | ||
| override suspend fun registerEvent( | ||
| eventType: EventType, | ||
| validationRequest: ValidationRequest, | ||
| eventData: String | ||
| ) { | ||
| log.debug("Registering event $eventType for validationRequest: $validationRequest and eventData: $eventData") | ||
| } | ||
| } |
There was a problem hiding this comment.
Dette skurrer litt for meg pga. double takes når jeg leser koden: Vi logger at vi "publiserer event" her, men metoden vi kaller heter logEvent, og den ligger i eventLoggingService. Det gir inntrykk av at vi blander begreper, noe som gjør det vanskeligere å forstå hva som faktisk skjer. Vi burde ta et steg tilbake og vurdere om både metodenavnet og servicenavnet bør endres for å bedre gjenspeile det den egentlig gjør.
I tillegg mener jeg logglinjen som sier at et event publiseres, burde flyttes inn i eventLoggingService, hvis det faktisk er der publiseringen skjer. På den måten blir det lettere å se hvilket ansvar tjenesten har, og vi får samlet all logging av event-publisering på ett sted. Hvis det allerede logges der inne, kan loggen i denne klassen fjernes.
Til slutt, og kanskje viktigst ifh til navngiving: Klassenavnet EventRegistrationServiceImpl sier lite om hva implementasjonen faktisk gjør. Vi bør unngå generiske navn som dette, og heller bruke noe som beskriver implementasjonen tydeligere,, for eksempel KafkaEventPublisherService (e.l.), hvis det er det den gjør. Samtidig bør vi også unngå legacy mønstre som IInterface-prefiks eller Impl-postfix, slike navn gir lite verdi og gjør koden mindre tydelig.
There was a problem hiding this comment.
Takk for en grundig semantisk analyse! 🙌
- Angående
Publishing eventer jeg helt enig. Det er bare et ord til for den samme handlingen. Opprinnelig ide var å kalle det som skjer iemottak-utils(altså kommunikasjon med Kafka) "logging events" og det som skjer i andre tjenester (altså byggingEventobjekter ut av domene modeller) "registering events". EventLoggingServicehar ingen logging i det hele tatt, fordi klassen ligger iemottak-utils. Det betyr at man må sende logger objekt som avhengighet ved oppretting av servicen. Det ble utført da jeg ville ha så få avhengigheter iemottak-utilssom mulig 🙂-Implpostfix markerer at det er den ekte utførelsen av grensesnittet i motsetning til klassen med-Fakepostfiks, som markerer at utførelsen er for testformål. Hvis det blir andre ekte utførelser av servicen, kan vi sikkert bruke andre postfixer for dem.- Hvis det blir neste runde av navngiving show kan vi bestemme hvilke mønstre gir verdi til koden. Som du så i går det finnes delte meninger på saken.
There was a problem hiding this comment.
Samtidig bør vi også unngå legacy mønstre som IInterface-prefiks eller Impl-postfix, slike navn gir lite verdi og gjør koden mindre tydelig.
@ivanskodje I Kotlin finnes det til og med en fin måte å implementere et interface på så vi slipper disse Impl-klassene osv. Jeg foreslo det tidligere ref: #135 (comment)
Da kan du f.eks. ha en default implementasjon ala: MyService og evt tester og fakes som fakeService osv. Trenger du en ny implementasjon beskriver du hva den er i implementasjonsnavnet f.eks AnewService.
There was a problem hiding this comment.
Samtidig bør vi også unngå legacy mønstre som IInterface-prefiks eller Impl-postfix, slike navn gir lite verdi og gjør koden mindre tydelig.
@ivanskodje I Kotlin finnes det til og med en fin måte å implementere et interface på så vi slipper disse Impl-klassene osv. Jeg foreslo det tidligere ref: #135 (comment)
Da kan du f.eks. ha en default implementasjon ala: MyService og evt tester og fakes som fakeService osv. Trenger du en ny implementasjon beskriver du hva den er i implementasjonsnavnet f.eks AnewService.
Litt enig med Oleksandr i den tråden du delte, men jeg er ikke enig i at vi skal bruke Impl postfix i noen som helst omstendigheter. Det har også ingenting med Test klassenavn å gjøre ved bruk av Fake som Oleksander tok opp som eksempel i denne tråden. Det er litt ironisk, fordi Test klasse navnet er fint fordi det beskriver hva det er, og ikke bare Impl. Det er bedre å gi implementasjonen et navn som beskriver hva den faktisk gjør, siden Impl-navngiving er et antipattern.
Det gir oss ingen informasjon om implementasjonens hensikt, og gjør det vanskeligere å forstå eller navigere i koden. Slike navn overlever ofte refaktorering og blir stående selv om innholdet endres, noe som bidrar til teknisk gjeld.
Men, default implementasjon trikset som du delte er fortsatt mye bedre alternativ enn å bruke Impl som er resultat av ikke ideelt navngiving 😅
There was a problem hiding this comment.
- Hvis det blir neste runde av navngiving show kan vi bestemme hvilke mønstre gir verdi til koden. Som du så i går det finnes delte meninger på saken.
There was a problem hiding this comment.
Tabs Vs Spaces neste-neste uke også! 🔥 😃
|
|
||
| private fun <T> cpaRepoTestApp(testBlock: suspend ApplicationTestBuilder.() -> T) = testApplication { | ||
| application(cpaApplicationModule(postgres.dataSource, postgres.dataSource, oracle.dataSource)) | ||
| val eventRegistrationService = EventRegistrationServiceFake() |
There was a problem hiding this comment.
Synes denne ble fin og tydelig - vi ser raskt og enkelt at dette er en egen test implementasjon 👍
ivanskodje
left a comment
ivanskodje
left a comment
There was a problem hiding this comment.
Synes det blir bra. Bra bruk av interface for å gjøre koden lettere å teste også 👍
Jeg har lagt inn en kommentar rundt navngiving og lesbarhet, der jeg tror det kan være lurt å vurdere noen justeringer for å unngå mulige utfordringer senere.
Lagt til registrering av hendelser: