Er dette god kode/design?

[Drogin]

Har et javaprosjekt der vi har en klasse vi persisterer, som også må være synkronisert med et eksternt system.
Dvs forandrer vi objektet, må også det eksterne systemet oppdateres.

Måten det er løst på, er at vi har et persisteringslag absolutt alle objekter i hele systemet går igjennom, der sjekkkes det om objektet er dette objektet som det andre systemet bruker, og hvis ja, så sender vi oppdatering til det andre systemet også.
Med andre ord kjøres omtrent denne javakoden for absolutt alle objekter i hele systemet før de persisteres:

if(objectToPersist instanceof MyObject) {
    MyObject myObject = (MyObject) objectToPersist;
    externalSystem.newOrUpdate(myObject);
}

 

Jeg synes dette høres helt horribelt ut med tanke på ytelse og generell design-filosofi.
En senior-resurs synes dette er en elegant løsning. De fleste trekker på skuldrene og bryr seg ikke.

Noen meninger? Er dette lurt? eller dumt?

Endret 29. september 2020 av Drogin

[henrikwl]

Det er vanskelig, for ikke å si umulig, å si noe skikkelig vettugt om 3 kodelinjer uten å se den større konteksten, men med mindre det skjer veldig spesielle ting i koden rundt så er ikke dette spesielt elegant nei. Java har hatt generics i 16 år nå, så hvis man bruker "instanceof" så er det en "bad code smell" som indikerer at man nok kunne ha tenkt annerledes ett eller annet sted i prosessen.

[Drogin]

Ja, instanceof codesmellen er jo en ting.

Jeg tenker også det er dumt å instanceof'e og "behandle" objekter unødvendig så langt nede i stacken.
En ting er jo alle de tusen objektene som går igjennom der unødvendig.

En annen ting er design. Slik det er laget er jo synkroniseringen helt frikobelt fra klassen som skal synkroniseres. Denne synkroniseringsmekanismen vil være veldig gjemt og vanskelig å finne for en utvikler som ikke kjenner systemet og får i oppgave å gjøre noe med synkroniseringen.

[henrikwl]

På måten du snakker om persisteringslaget på høres det ut som om dere skriver deres eget? Det er i seg selv en code smell – jeg håper i det aller minste at det bygger på JDO/JPA eller et annet modent persisterings-API?

Objektpersistering er på mange måter nesten et eget fagfelt, så med mindre det dere lager er et persisteringsbibliotek så ser jeg omtrent ingen grunn til å jobbe med persistering på dette detaljnivået. Skoleoppgaver vil være en sånn grunn.

Endret 29. september 2020 av henrikwl

[Drogin]

henrikwl skrev (27 minutter siden):

På måten du snakker om persisteringslaget på høres det ut som om dere skriver deres eget? Det er i seg selv en code smell – jeg håper i det aller minste at det bygger på JDO/JPA eller et annet modent persisterings-API?

Objektpersistering er på mange måter nesten et eget fagfelt, så med mindre det dere lager er et persisteringsbibliotek så ser jeg omtrent ingen grunn til å jobbe med persistering på dette detaljnivået. Skoleoppgaver vil være en sånn grunn.

Det er riktig. Det er et gammelt legacy-prosjekt som moderniseres og får fjernet "teknisk gjeld".
En teknisk gjeld jeg ønsker å fjerne er som du sier å fjerne hele "persisteringslaget" som vi har skrevet selv, og overlate det til JPA/Hibernate. Fjerne bit for bit i laget sakte men sikkert.

Overnevnte kode er den av dette laget som er selv-skrevet persisteringskode. Tidligere var det helt egen kode, men vi jobber med å koble inn JPA/Hibernate. men det er et sinnrikt system med branchende veier igjennom en kjede av persisteringsklasser.

Endret 29. september 2020 av Drogin

[henrikwl]

Ahh… Legacy kode. ❤️

Jeg er usikker på overheaden med instanceof operatoren, men hvis det er som du sier at alle objekter i systemet enten de skal persisteres eller ikke flyter gjennom den sjekken der så kan det være en flaskehals. Men prematur optimalisering skaper ofte mer problemer enn det løser, så med mindre JProfiler eller noe sier at akkurat den metoden der denne kodesnutten bor er veldig grådig på tid så tror jeg ikke nødvendigvis det er krise at den koden er som den er.

Endret 29. september 2020 av henrikwl

[quantum]

Drogin skrev (På 29.9.2020 den 9.44):

En annen ting er design. Slik det er laget er jo synkroniseringen helt frikobelt fra klassen som skal synkroniseres. Denne synkroniseringsmekanismen vil være veldig gjemt og vanskelig å finne for en utvikler som ikke kjenner systemet og får i oppgave å gjøre noe med synkroniseringen.

Det er jo helt umulig å mene noe utifra den koden du postet, som andre også påpeker, men generelt er jo "helt frikoblet" noe man prøver å oppnå i design... når det gjelder lesbarheten kan det dog være et problem å skjønne noe som helst ja, ref. diverse smarte AOP-løsninger med regexp-baserte pointcuts som bare er definert i noen bortgjemte xml-filer...