Mark Seemann stillede for nylig et spørgsmål på Twitter, der lød noget i retning af ”Skal Delete(int id) kaste en exception hvis det element, id repræsenterer, ikke eksisterer, eller skal den bare undlade at gøre noget?”.
Ræsonnementet for ikke at gøre noget er naturligvis, at hvis vi beder om at få slettet noget, og det ikke er der, så er sluttilstanden jo den ønskede, uanset om vi slettede noget eller ej.
Mit svar var, at med den signatur skulle metoden kaste en exception, og i dette indlæg vil jeg forsøge at uddybe lidt. Der er to grunde til, at jeg synes, en exception er på sin plads i dette tilfælde.
For det første er vi vant til, at Delete() på framework-typer som f.eks. File, Directory og DataRow alle smider passende exceptions, hvis de kaldes med et element, der ikke findes. Så hvis vi vil holde os til Principle of least surprise, er en exception det rette valg.
For det andet, og måske vigtigere, vil det være umuligt for den kaldende kode at afgøre, hvorvidt operationen var en succes eller ej, hvis Delete() ignorerer eventuelle fejl. Da vi ikke kan sige noget om, hvilken kontekst Delete() vil blive kaldt i, er det ikke rimeligt, at tage beslutninger på den kaldende kodes vegne. Vi ved med andre ord ikke, om koden har brug for at skelne mellem succes og fejl.
Hvis koden kalder Delete(), må vi antage, at det er fordi applikationens tilstand af en eller anden grund får det til at give mening. Koden antager altså, at det kan lade sig gøre. Ved at ignorere en eventuel fejlsituation, fratager vi den kaldende kode muligheden for at opdage, at denne antagelse ikke holdt stik. Vi risikerer altså at skjule en fejl i koden.
Mark fulgte desuden op med følgende spørgsmål: Gør det nogen forskel om Delete() kaldes i et miljø med flere tråde? Nej, ikke i min optik.
File.Delete() er i høj grad underlagt denne problemstilling. Uanset at vi kan undersøge, om en given fil eksisterer lige inden, vi kalder Delete(), er denne operation altid underlagt en race condition. Filen kan forsvinde mellem vores kald til Exists() og Delete(), og den eneste måde, vi kan opdage, om dette er tilfældet, er, hvis Delete(), kaster en exception. Derefter må det være op til den kaldende kode, om den vil reagere på dette eller ej. Ved at undlade at reportere fejlen, fratager vi den kaldende kode muligheden for at reagere.
Derfor synes jeg, en exception er det rette valg i denne situation.
Du giver lutter gode argumenter for at kaste exceptions, men der er efter min mening også gode argumenter for det modsatte – og det var derfor jeg oprindeligt spurgte, idet jeg var oprigtigt i tvivl. Jeg havde selv været inde på mange af de overvejelser du her bringer, men der er også argumenter, der trækker i den anden retning (men bemærk i øvrigt at jeg aldrig sagde noget om metodens returværdi).
For det første er der argumentet om Principle of Least Astonishment (POLA). Du har ret i at hvis man er vant til de BCL-metoder, du nævner, vil det være overraskende hvis en anden Delete-metode opfører sig anderledes. Jeg er dog personligt ikke velbevandret i disse metoder, og finder det faktisk overraskende at de opfører sig som beskrevet. Under alle omstændigheder er det jo personligt hvad der overrasker en, så selvom jeg selv holder meget af POLA, er argumentet i sidste ende subjektivt.
Et vigtigere argument vedrører concurrency. Lad os forestille os en web-applikation hvor brugere kan vedligeholde forskellige Domain Entities. To forskellige brugere bliver begge gjort opmærksom på at en bestemt Entity skal slettes. De går begge ind på den relevante web-side og markerer den Entity, der skal slettes. Den ene bruger bliver dog afbrudt af sin telefon, så den anden bruger gennemfører sletningen. Applikationen kalder den meget omdiskuterede Delete-metode.
Efter sin telefonsamtale trykker den første bruger også på Delete-knappen og applikationen kalder dermed Delete-metoden igen. Hvad bør der ske i dette scenario?
Brugeren er i sidste ende fløjtende ligeglad med om objektet allerede er blevet slettet. Forretningsmålet er at slette objektet.
Hvis man kaster en exception i dette scenario, giver man brugeren en dårlig oplevelse af applikationen. Dvs. at hvis Delete-metoden kaster en exception bliver applikationen nødt til at håndtere og undertrykke den.
Et vigtigt Guiding Principle for Exception Design i BCLet er at man kun bør kaste exceptions i exceptionelle situationer, eller hvis man simpelt hen ikke kan fortsætte eksekvering. Ingen af disse betingelser er opfyldt her.
Det er et forventeligt scenario under Optimistic Concurrency at Delete vil blive kaldt med et ID på en Entity der ikke længere eksisterer. Det har intet med multi-threading at gøre (men er selvfølgelig en race condition). Derfor bør vi ikke kaste en exception af den grund.
Det er også muligt at fortsætte eksekveringen af Delete-metoden selv hvis emnet ikke længere eksisterer. I dette tilfælde bliver metoden blot en no-op, og hele metoden derved Idempotent.
Da man ikke bør benytte Exceptions til flow control vil jeg altså argumentere for at det er forkert at kaste exceptions hvis argumentet er, at man gerne vil have mulighed for at kommunikere til kalderen om emnet rent faktisk blev slettet eller ej.
Hvis man endelig vil give sig selv (eller andre) den mulighed, bør man i stedet returnere en boolean, der indikerer om emnet konkret blev slettet eller ej. Dette er f.eks. tilfældet for Remove-metoden på det generiske ICollection-interface, men jeg må også konstarere at jeg aldrig har benyttet mig af denne returværdi.
Grundlæggende er jeg kommet frem til den holdning at formålet med en Delete-metode er at sikre at et givent element ikke længere eksisterer. Om det allerede tidligere er blevet slettet uden at jeg opdagede det er meget sjældent interessant.
Hej Mark. Tak for dit uddybende svar. Hvis det var et spørgsmål med kun et fornuftig svar, havde du næppe stillet det. Jeg er klar over, at dette ikke er sort/hvidt.
Min argumentation hænger også på den noget begrænsede kontekst dit tweet gav. Under andre omstændigheder kan man sagtens forsvare, at undlade at kaste en exception. Jeg synes dog, at det i alle tilfælde er en dårlig ide at fjerne muligheden for at kunne skelne mellem de to situationer.
DELETE i SQL er et godt eksempel på en kommando, der ikke brokker sig i tilfældet af, at den ender med ikke at slette nogle poster. Den returnerer til gengæld antal af slettede poster, og derved har vi mulighed for at finde ud af, om operationen egentlig var overflødig. Du skriver, at det er sjældent, at vi har brug for den viden, men hvis grænsefladen ikke gør det mulig at fremskaffe denne information, kan vi i hvert fald ikke håndtere disse tilfælde, uanset hvor sjældne de end må være.
Min primære bekymring er således, at den kaldende kode skal være i stand til at skelne mellem, hvornår antagelserne var forkerte, og hvornår de var korrekte. Om dette er vigtig eller ej, kan ikke være op til Delete() at bestemme.
Hvis Delete() kaldes hyppigt med elementer, der allerede er slettet, ville en returværdi være mere passende end en exception, men det er i realiteten blot en optimering. Pointen er, at det kun er den kaldende kode, der kan tage stilling til om det er vigtigt, at kunne skelne mellem de to situationer.
Jeg er ikke enig med dig i, at en exception i dette tilfælde bruges som flow-kontrol. Jeg antog, at din signatur i realiteten var void Delete(int id). I så fald vil jeg opfatte det som en exception at forsøge at kalde metoden med en id, der peger på et ikke eksisterende element. Den kaldende kode beder om noget, der ikke giver mening givet den nuværende tilstand. Det er en exception. Havde metoden haft en bool eller en int som returtype, ville jeg ikke smide en exception men derimod returnere den relevante værdi på baggrund af metodens udfald.
Du har da helt ret i at det er mest optimalt hvis den kaldende kode gives muligheden for at skelne, men så foretrækker jeg en returværdi – den kan jeg nemlig også vælge at ignorere
Her kommer YAGNI dog i spil, men det er igen meget afhænigt af kontektsten.
Min oprindelige kontekst var et af Safewhere’s produkter hvor vi har en meget stor grad af kontrol over alle kaldere af det aktuelle API. I det tilfælde har vi valgt at implementere Delete som en Idempotent metode uden returværdi, da vi ikke forventer at have behov for at skelne mellem de to situationer. Hvis behovet mod forventning skulle opstå, kan vi let retrofitte en returværdi på det tidspunkt.
Det ville forholde sig væsentligt anderledes hvis der var tale om et API der skulle benyttes af mange forskellige kaldere uden for min umiddelbare kontrol (såsom et genbrugeligt library). I det tilfælde ville jeg nok vælge at returnere en bool.
Som du skriver er det ikke sort/hvidt
Jeg vil forvente at en exception vil blive thrown, hvis recorden i databasen ikke eksisteret, da vi befinder os i .Net kode context og ikke i en database context.
Pragmatisk kan man lave TryDelete metoder, hvis man ikke ønsker exceptions – eller en overload Delete(bool throwIfNotFound)
@Anders: Jeg har det fint med, at metoden kan returnere en værdi afhængig af udfaldet, men jeg bryder mig ikke om metoder, der skifter opførsel afhængig af inputflag. Jeg er klar over, at der er flere eksempler på dette i BCL, men det ændrer ikke ved, at jeg synes, at det er en code smell, idet metoden tydeligvis varetager mere end en opgave (eller den samme opgave på mere end en måde).
@Brian: En overload Delete(bool throwIfNotFound) er en code smell, men TryDelete kunne være en mulighed, hvis flow diktere behov for at kunne kalde en delete methode uden risiko for at der bliver throw exceptions.
Exception skal også throws ved logiske forretnings fejl – hvilket det må være, når der forsøges at slette en record i databasen, som ikke findes.
Det handler om flertallets mavefornemmelse om en Delete metode skal thow exceptions eller ej. Hvad forventer udviklerne? Jeg har desværre set for mange systemer, hvor custom kode-basen gør ét og BCL/FCL gør noget andet.
Et relateret diskussions emne: Skal en Login metode throw exceptions, fx hvis brugeren ikke findes?
Interessant diskussion, Exception Handling er altid spændende at debattere fordi der findes mange synspunkter.
Mit synspunkt er nu ganske simpelt, inspireret af YAGNI og KISS, nemlig kast altid exceptions videre til kalderen og lad afgørelsen ligge der, medmindre de kan håndteres.
Der findes dog en enkelt undtagelse; exceptions som ikke giver nogen umiddelbar mening bør omskrives, eks.vis database PK-violations, null reference pga. manglende parameter validering mv.
Jeg er ikke tilhænger af magiske bools som returns eller metoder med inputflag, exceptions er IMHO til for at gøre det nemmere for udvikleren, ikke sværere
En lille detalje.
File.Delete kaster rent faktisk _ikke_ en exception såfremt filen ikke eksisterer.
En exception kastes derimod såfremt den angivne sti er forkert.
Det ser ud til, at du har ret. Dokumentationen siger ellers følgende “Deletes the specified file. An exception is not thrown if the specified file does not exist.” Men du har ret i, at det ikke ser ud til, at den smider FileNotFoundException. Underligt. Så lærte jeg også noget
Jeg har set lidt mere på det, og
File.Deletehar faktisk en lidt spøjs opførsel. Med Reflector, kan vi se, at det dybest set blot er en covermetode for Kernel32.DeleteFile.DeleteFilereturnererERROR_FILE_NOT_FOUND, hvis filen ikke findes. Ergo, er det oplagt, hvorfor dokumentationen siger, som den gør, men hvis vi ser på implementeringen afFile.Delete, checker den specifikt på om fejlen erERROR_FILE_NOT_FOUND(errorCode 2), og i så fald ignorerer koden fejlen.@Anders: Din seneste kommentar var desværre røget ind som spam af uvisse årsager. Askimet plejer ellers at være ret skarp, men den smuttede altså.
Jeg synes ikke som udgangspunkt, at login skal smide en exception (men det afhænger af APIets udformning), ligesom jeg heller ikke ville forvente, at File.Exists skal smide en exception. I begge tilfælde er det noget, vi ved, kan gå galt, så det er altså control flow. File.Delete ville jeg derimod forvente skulle smide en exception (som dokumentationen også siger, men som desværre viser sig ikke at være tilfældet).
Jeg vil starte med at antage at vi ikke diskuterer:
void Delete(int id) // throws FoobarException
versus:
void Delete(int id) // do not throws FoobarException
men derimod:
void Delete(int id) // throws FoobarException
versus:
bool Delete(int id) // do not throws FoobarException
eller:
int Delete(int id) // do not throws FoobarException
Det er ikke godt med en metode som slet ikke afslører om der skete noget.
Selv en Command ExecuteNonQuery af en DELETE SQL returnerer jo også antallet af rækker.
Jeg kan ikke se, at POLA kan bruges som argument for exception. Det er allerede påpeget at File Delete ikke altid smider exception. Jeg vil påpege at diverse collections Remove heller ikke smider exception.
En kompetent programmør vil slå en Delete metode op for at finde ud af, hvordan den håndterer ikke fundet situationen.
(og derfor skal det %&¤”%¤¤!/%! stå i dokumentationen !)
Jeg er helt enig med Mark i at beslutning om exception eller ej skal afhænge af om ikke fundet er en forventet situation eller en reel fejl.
Hvis det i den pågældende klasse er forventet, at Delete ikke finder noget, så bør den returnere en status værdi.
Hvis det i den pågældende klasse er tegn på. at noget er helt galt, hvis Delete ikke finder noget, så bør den smide en exception.
I den meget typiske situation, hvor klassen er af generel karakter og kan bruges både i situationer hvor ikke fundet er forventet og en alvorlig fejl, så er situationen lidt mere grumset.
Jeg vil betragte det som mest oplagt at returnere en status værdi. Min begrundelse er at det er pænere at konvertere en status værdi til en exception i den kaldende kode end at catche en exception og enten ignorere den eller konvertere til en status værdi.