Test altijd je code. Ook als je denkt: dit moet gewoon goed gaan.

Ingediend door Dirk Hornstra op 26-jan-2023 22:48

In 2019 heb ik al eens gedeeld dat 1 + 2 wel 3 is, maar dat 0.1 + 0.2 niet altijd 0.3 is: link.

Ik liep nu zelf ook weer tegen twee gevalletjes "je denkt het het zo-en-zo werkt, maar het werkt net anders".

Het eerste scenario:

Dit was mijn stukje code:



var doUpdate = object.functie1();
doUpdate = doUpdate || object.functie2();
doUpdate = doUpdate || object.functie3();

if (doUpdate) {

...

..SaveChanges();

}

Wat was de gedachte hier achter? Nou, een bepaald object heeft een aantal update functies. Dat is een "update van het object zelf". Vervolgens gaat de aanroepende code die updates wegschrijven in de database. Het object heeft geen idee dat de gegevens in de database opgeslagen worden (en dat wil ik ook graag zo houden).

Mijn gedachte hier was dus: ik ga update-functie1 aanroepen, update-functie2 en update-functie3. Als minimaal 1 van deze functies een "TRUE" terug  geeft, dan is er dus wat aangepast en moeten we de database dus bijwerken.

Het probleem zit hier in de "of". Stel dat functie1 "TRUE" terug geeft, dan is er dus wat veranderd en moet de data bijgewerkt worden. De compiler zegt dan bij de volgende regel TRUE óf functie2. Omdat TRUE of FALSE als resultaat TRUE geeft en TRUE of TRUE ook resultaat TRUE geeft zegt de compiler: mooi, ik hoef functie2 dus helemaal niet uit te voeren om te kijken wat het resultaat is, want dat weet ik toch al. Qua performance is dat natuurlijk geweldig. Maar qua intentie van mijn code is dat helemaal niet de bedoeling. Want ik wil dat functie2 en functie3 ook uitgevoerd worden, daar kunnen namelijk ook waardes in aangepast worden en die wijzigingen moeten ook opgeslagen worden. Als die functies niet aangeroepen worden, worden de gegevens ook niet gewijzigd!

En de "of" vervangen met een "en" gaat ook niet werken. Want stel dat functie1 een TRUE terug geeft, dus bijwerken. Zou functie2 een FALSE terug geven (die hoeft niets te wijzigen), dan is door de "en" het resultaat ook ineens FALSE, waardoor je wijzigingen van functie1 niet opgeslagen worden.

Je kunt voor elke functie een variabele aanmaken. Dus var resultFunctie1, resultFunctie2, resultFunctie3, die is het resultaat van elke functie en dan ga je een if (resultFunctie1 || resultFunctie2 || resultFunctie3) ... doen, dat werkt wel. Maar als je dat ziet, dan voel je jezelf een beetje "vies" met al die variabelen. Als er straks nog 20 losse functies komen, ga je dan ook weer 20 losse variabelen daarvoor maken? En dan wordt je variabele || variabele || lijst wel heel erg lang.

Vreemd eigenlijk dat ik hier nu tegen aanliep, want eigenlijk had ik verwacht dat ik wel eens eerder zoiets gedaan had. Ik heb het nu opgelost door de resultaten in een lijst te plaatsen en te zeggen: "als 1 van de resultaten in de lijst "TRUE" is, dan gaan we updaten":



var resultList = new List<bool>();
resultList.Add(object.functie1());
resultList.Add(object.functie2());
resultList.Add(object.functie3());
if (resultList.Any(rec => rec == true)) {
....
...SaveChanges();

}
 

Ik kwam hier toevallig achter omdat er een ander probleem was (deze automatische taak was vorige maand voor het laatst gestart, dus ik wilde controleren of er ergens in de flow wat fout ging). Er ging ineens een lichtje bij mij branden toen mijn breakpoint op functie2 en functie3 "zomaar" overgeslagen werd.

Maar... ook deze oplossing is niet goed!

Als je goed kijkt zie je waar het door komt, het bepalen of "het object wijzigingen heeft", dat voer je uit "buiten het object". Dat betekent dat als je ergens anders ook iets met die functies doet (bijvoorbeeld alleen functie1 aanroepen) je ook daar moet controleren "buiten het object" of het object bijgewerkt is. Dat ruikt al en beetje naar "copy-paste code". Het object moet namelijk zélf bijhouden of het bijgewerkt moet worden, dat noem je Domain Driven Design.  Dus de code zou zo kunnen worden:



object.functie1();
object.functie2();
object.functie3();
if (object.IsUpdated()) { ...


class ClassOfObject {
private bool _isUpdated {get;set;}
public ClassOfObject(){ _isUpdated = false;}
public void functie1(){... if ... _isUpdated = true; ...}
public void functie3(){... if ... _isUpdated = true; ...}
public void functie3(){... if ... _isUpdated = true; ...}
public bool IsUpdated() { return _isUpdated;}

Het tweede scenario:

Als je met GIT werkt, dan weet je dat een repository (de plek waar de code van 1 project opgeslagen is) een boomstructuur is. Deze boom heeft minimaal 1 branch (tak). Maar als je wat zaken gaat ontwikkelen, of je moet wat fouten oplossen, dan maak je een soort kopie van de code, zodat je daar ongestoord in kunt werken en die later weer kunt samenvoegen naar de originele branch. Dus je kunt wel 20 takken (branches) hebben die bij 1 boom (repository) horen. Elke tak heeft zijn eigen versies van bepaalde NPM packages. Dat willen we bijhouden, dus we gaan al die takken controleren.

Het probleem is hier dat we "vanuit de takken" terug gaan naar de boom. De branch "main" kan gisteren bijgewerkt zijn, de branch "coole-features" die net aangemaakt is, is nog niet gesynchroniseerd en ook de branch "nare-bug" is nog maar net aangemaakt. We willen natuurlijk de branches die nog niet eerder geïnventariseerd zijn controleren. Daarna de branches die het langst geleden bijgewerkt zijn en zo door naar de meest recente update.

Ook hier zag ik dat steeds de branches verwerkt werden die we al net hadden bijgewerkt. Dus we gaan weer de branches die in januari al bijgewerkt zijn doorlopen, maar die in december voor het laatst bijgewerkt zijn, die laten we liggen.

De oorzaak lag hier in het feit dat we eerst de branches opvragen en die sorteren op basis van "datum van bijwerken". Daar hadden we (nog) de juiste volgorde. Vervolgens gingen we van die branches de repository opvragen, want dat is de plaats waar we moeten beginnen (je start in de boom en duikt daarna de takken in). Ook dat ging nog wel goed, maar daarna gingen er wat zaken fout.

Zo had ik al aangegeven dat we in dit geval de branch "coole-features" en "nare-bug" opvragen die bij dezelfde boom horen. Dus de boom kwam 2x in onze lijst. Daar had ik een .Distinct(..) actie op gezet, zodat we een repository maar één keer gaan verwerken (anders doe je dubbel, driedubbel, vierdubbel, ...hetzelfde). En daarbij vroegen we alleen het ID op van de repository (in onze database, dus gewoon een oplopend nummer). En ja... de code begon die zelf te sorteren, waardoor we onze eerdere sortering kwijt raakten. En begon ie weer met dezelfde repository als die de code de vorige keer ook al verwerkt had.

 

Moraal van dit verhaal?

"Wij" developers vinden het lekker om code te tikken. Dit zo, dit zo, kijk eens hoe mooi het werkt! Ruim binnen de gestelde tijd klaar. Tijdens het bouwen van je code wel 20 keer door collega's gestoord. Maar je zat toch weer gauw in je flow, toch?

Het probleem is echter dat ik misschien net in die code van object.functie1() en object.functie2() zat toen ik gestoord werd, er andere zaken waren en ik de volgende dag pas verder ging met deze code. Het niet meer zelf getest heb. En zo sluipen de bugs je code in.

Wees kritisch op jezelf. En vraag je collega's eens of ze jouw code willen testen/controleren/reviewen.

En doe geen aannames. Als je "denkt" dat iets zo-en-zo werkt, dat is niet genoeg. Je moet "zeker weten" dat iets zo-en-zo werkt. En anders moet je een test maken die laat zien/valideert hoe het werkt.