[Alg] Slechtste programmeervoorbeelden deel 3

quote:

Janoz schreef op woensdag 16 juli 2008 @ 10:24:
@super-muffin:
Soms wordt in de code-guidelines opgegeven dat je niet met een negatie mag werken om de reden die ik hierboven noem. Dan krijg je inderdaad dergelijke constructies. Echt fout zou ik ze niet willen noemen (tenzij het een gevolg is van een ontwikkelaar die helemaal geen negatie kent)

Ik zou echter wel een stukje commentaar in het lege blok opnemen. Iets als '\\do nothing'.

Dan zou ik zelf eerder iets gebruiken zoals
C#:

1 2 3 4

if (tblZoek.Visible == false) {   //do iets }

Doe hetzelfde als de negative, is alleen wat meer verboze.

Zag vanochtend trouwens ook een zeer slecht voorbeeld
C#:

1 2 3 4 5 6 7 8 9 10

try {    cmd.Connection.Open();    cmd.ExecuteNonQuery(); } catch { //just ignore exception.. } finally {    cmd.Connection.Close(); }

Persoonlijk ga ik al als een stier tekeer als een developer een statement als catch(Exception ex) gebruikt (je behoort alleen fouten af te vangen welke je verwacht), echter na het zien van bovenstaande code (van een ervaren programmeur) ben ik even 10 minuten buiten gaan staan.

Het negeren van excepties is een echte no-no in mijn boek. Ook was deze developer 'vergeten' logging te gebruiken. Ook geen controle of er wel records zijn veranderd (ExecuteNonQuery geeft het aantal gewijzigde records terug).

De developer in kwestie mag vanavond in zijn vrije tijd alsnog een juiste implementatie schrijven.

quote:

Geqxon schreef op woensdag 16 juli 2008 @ 11:41:
[...]


Is op zich wel slim. De method ToBoolean geeft een boolean, die je in principe direct kan returnen, maar vergeet de ConversionException niet.

Boolean.TryParse is veel handiger en heeft ingebouwde foutcontrole.
C#:

1 2 3 4 5 6

function IsBool(string arg) {   bool result;   return Boolean.TryParse(arg, out result); }

quote:

Niemand_Anders schreef op woensdag 16 juli 2008 @ 11:54:
[...]

Boolean.TryParse is veel handiger en heeft ingebouwde foutcontrole.
C#:

1 2 3 4 5 6

function IsBool(string arg) {   bool result;   return Boolean.TryParse(arg, out result); }

Een probleem is wel dat dit een ander resultaat geeft:

quote:

When this method returns, if the conversion succeeded, contains true if value is equivalent to TrueString or false if value is equivalent to FalseString. If the conversion failed, contains false.

Tryparse geeft ook false terug als de bool waarde van wat gechecked word false is.

quote:

Niemand_Anders schreef op woensdag 16 juli 2008 @ 11:45:

Het negeren van excepties is een echte no-no in mijn boek. Ook was deze developer 'vergeten' logging te gebruiken. Ook geen controle of er wel records zijn veranderd (ExecuteNonQuery geeft het aantal gewijzigde records terug).

Toch moet ik zeggen dat ik dat nog wel in mijn code ergens heb zitten, maar het gaat dan wel over een heel specifiek geval. Het is dan wel zo dat ik een specifieke exception afvang en in commentaar heb staan dat hij geignored kan worden. Hierbij valt te denken (ik praat over java) aan de verplichte InteruptedException wanneer je sleep gebruikt en het niet uitmaakt of iets onderbroken wordt of volledig de gestelde pauze doorloopt.

Of bijvoorbeeld het volgende stukje code:
Java:

1 2 3 4 5 6

int result = DEFAULT; try {   result = Integer.parseInt(someString); } catch (NumberFormatException nfe) {   //do nothing }

Maar in alle andere gevallen wil ik inderdaad minimaal een
LOG.warn("beschrijvende melding",throwable);
in de catch body staan.

quote:

Janoz schreef op woensdag 16 juli 2008 @ 12:13:
[...]

Toch moet ik zeggen dat ik dat nog wel in mijn code ergens heb zitten, maar het gaat dan wel over een heel specifiek geval. Het is dan wel zo dat ik een specifieke exception afvang en in commentaar heb staan dat hij geignored kan worden. Hierbij valt te denken (ik praat over java) aan de verplichte InteruptedException wanneer je sleep gebruikt en het niet uitmaakt of iets onderbroken wordt of volledig de gestelde pauze doorloopt.

Of bijvoorbeeld het volgende stukje code:
Java:

1 2 3 4 5 6

int result = DEFAULT; try {   result = Integer.parseInt(someString); } catch (NumberFormatException nfe) {   //do nothing }

Maar in alle andere gevallen wil ik inderdaad minimaal een
LOG.warn("beschrijvende melding",throwable);
in de catch body staan.

Zelfs in dit geval zou je toch een melding willen geven dat de ingevoerde string geen juiste invoer was en dat je dus maar een default waarde gebruikt. Nu gaat hij een standaard waarde stilletjes gebruiken zonder dat je weet dat er foute invoer was...

quote:

Marcj schreef op woensdag 16 juli 2008 @ 12:25:
[...]


Zelfs in dit geval zou je toch een melding willen geven dat de ingevoerde string geen juiste invoer was en dat je dus maar een default waarde gebruikt. Nu gaat hij een standaard waarde stilletjes gebruiken zonder dat je weet dat er foute invoer was...

Afhankelijk van de situatie kan ik me indenken dat je niet altijd een melding wilt geven, een gebruiker zit er vaak niet op te wachten, en een logfile vullen met dat soort meldingen is ook niet echt superhandig. Maar nogmaals, afhankelijk van de situatie

quote:

Erkens schreef op woensdag 16 juli 2008 @ 12:28:
[...]

Afhankelijk van de situatie kan ik me indenken dat je niet altijd een melding wilt geven, een gebruiker zit er vaak niet op te wachten, en een logfile vullen met dat soort meldingen is ook niet echt superhandig. Maar nogmaals, afhankelijk van de situatie

Noem eens een voorbeeld?

quote:

YakuzA schreef op woensdag 16 juli 2008 @ 12:10:
[...]


Een probleem is wel dat dit een ander resultaat geeft:

[...]


Tryparse geeft ook false terug als de bool waarde van wat gechecked word false is.

Er wordt niets met 'result' zelf gedaan. Het gaat puur om de return value van TryParse en die geeft alleen maar aan of de conversie is gelukt.De geconverteerde waarde wordt via de tweede parameters (out result) terug gegeven.

Het resultaat is dus niet anders.

Uit de MSDN (System.Boolean.TryParse):

quote:

Return Value
Type: System..::.Boolean

true if value was converted successfully; otherwise, false.

quote:

Mozin schreef op woensdag 16 juli 2008 @ 10:06:
Kwam ik tegen in een van onze projecten, het is niet echt fout, maar wel een beetje omslachtig .
code:

if (tblZoek.Visible == false)
{
  // Doe iets
}
else if (tblZoek.Visible == true) 
{
  // Doe iets anders 
}

Dit duid dus echt op een beginnende programmeur, die totaal geen overzicht heeft over boolean logica en if/else constructies...want als je dat wel snapt, ga je dit niet voor de lol zo neerzetten.

quote:

LuCarD schreef op woensdag 16 juli 2008 @ 10:34:
[...]

En wat als tblZoek.Visible Null is?

Dat zal deze nooit uit zichzelf zijn. Default value van de Visible propertie is true, en alleen wanneer jij deze in runtime op null zet kan deze null worden.

slindenau wijzigde dit bericht 16-07-2008 12:58 (22%)

quote:

Marcj schreef op woensdag 16 juli 2008 @ 12:37:
[...]

Noem eens een voorbeeld?

Je geeft niet voor niets een default value op, dus er zal wel een situatie te bedenken zijn dat je een string wilt gebruiken als integer en als dat niet kan een "default" value wilt gebruiken en no questions asked

Maar als het was geschreven als:
Java:

1 2 3 4 5 6

int result; try {   result = Integer.parseInt(someString); } catch (NumberFormatException nfe) {   result = DEFAULT; }

Had je er geen probleem mee gehad?

quote:

slindenau schreef op woensdag 16 juli 2008 @ 12:56:
[...]

Dit duid dus echt op een beginnende programmeur, die totaal geen overzicht heeft over boolean logica en if/else constructies...want als je dat wel snapt, ga je dit niet voor de lol zo neerzetten.


[...]

Dat zal deze nooit uit zichzelf zijn. Default value van de Visible propertie is true, en alleen wanneer jij deze in runtime op null zet kan deze null worden.

Mwah toch zie je dat soort constructies wel veel hoor, zeker op embedded systemen waarbij geheugen beperkt is en je dus variabelen geen waarde geeft tenzij echt nodig.

Dan is het vrij standaard dat iets null kan zijn en kom je er dus met een simpele if niet uit. Al zou ik dan gewoon een switch gebruiken en geen if. (Afhankelijk van de taal en de compiler, sommige compilers zijn hier niet super efficient mee, zeker als je een default of een null waarde hebt waarin je verder niets doet)

Wel is het duidelijk dat dit niet aan de orde was in het voorbeeld, maar ik heb genoeg code gezien waarbij veel if statements op die manier in elkaar zitten.

quote:

slindenau schreef op woensdag 16 juli 2008 @ 12:56:
[...]

Dat zal deze nooit uit zichzelf zijn. Default value van de Visible propertie is true, en alleen wanneer jij deze in runtime op null zet kan deze null worden.

Als ik anders probeer .Visible van een control op null te zetten krijg ik mooi de volgene error van Visual Studio
code:

Cannot convert null to 'bool' because it is a value type

Edit:
Bovenstaand verhaal maakt het natuurlijk wel iets anders

BM wijzigde dit bericht 16-07-2008 13:09 (6%)

quote:

Janoz schreef op woensdag 16 juli 2008 @ 12:13:
[...]

Toch moet ik zeggen dat ik dat nog wel in mijn code ergens heb zitten, maar het gaat dan wel over een heel specifiek geval. Het is dan wel zo dat ik een specifieke exception afvang en in commentaar heb staan dat hij geignored kan worden. Hierbij valt te denken (ik praat over java) aan de verplichte InteruptedException wanneer je sleep gebruikt en het niet uitmaakt of iets onderbroken wordt of volledig de gestelde pauze doorloopt.

Zoals aangegeven had ik al aangegeven dat verwachte excepties zoals de interupt bij een tread gewoon afgevangen mogen worden. Echter blind het generieke exception afvangen en daar niets mee doen is een doodzonde. Op het moment dat een developer de generieke exceptie wrapped in een custom exception, dan heb ik er (veel) minder problemen mee.

quote:

Of bijvoorbeeld het volgende stukje code:
Java:

1 2 3 4 5 6

int result = DEFAULT; try {   result = Integer.parseInt(someString); } catch (NumberFormatException nfe) {   //do nothing }

Alle variabelen welke via input van derden (input controls) komt dient te worden gevalideerd. Dus het formulier had het ongeldige nummer al moeten afvangen.

Gewoon stoïcijns doorgaan en met default waardes verder werken kan ervoor zorgen dat het resultaat uiteindelijk anders is dan verwacht. Wij zijn voornamelijk actief in de financiele sector en afgegeven (lening/lease) offertes zijn bindend. Een adviseur moet er vanuit kunnen gaan dat de offerte correct wordt berekend, ook als hij ongeldige waardes invoert. Hij hoort dan op zijn fouten gewezen te worden.

Juist dit soort excepties op basis van externe waardes loggen wij zodat hierover later analyses kunnen worden losgelaten. Als heel veel adviseurs dezelfde invoer fouten maken, dan wil je dat als development bedrijf weten. Hier kun je dan op inspringen.

quote:

Maar in alle andere gevallen wil ik inderdaad minimaal een
LOG.warn("beschrijvende melding",throwable);
in de catch body staan.

Ik heb zelfs programmeurs gehad welke aangaven geen logging te gebruiken vanwege de negatieve impact op de performance. Ook onze final releases worden gewoon met debug symbols verscheept. Bij productie fouten is een leesbare stacktrace van onschatbare waarde. De ingeleverde performance neem ik dan heel erg graag voor lief als ik dan bij een probleem weet dat het probleem optrad in bestand x.cs op regel y in plaats van 'RenderReport() +631'.

quote:

TRRoads schreef op woensdag 16 juli 2008 @ 13:04:
[...]

Mwah toch zie je dat soort constructies wel veel hoor, zeker op embedded systemen waarbij geheugen beperkt is en je dus variabelen geen waarde geeft tenzij echt nodig.

Dan is het vrij standaard dat iets null kan zijn en kom je er dus met een simpele if niet uit. Al zou ik dan gewoon een switch gebruiken en geen if. (Afhankelijk van de taal en de compiler, sommige compilers zijn hier niet super efficient mee, zeker als je een default of een null waarde hebt waarin je verder niets doet)

Wel is het duidelijk dat dit niet aan de orde was in het voorbeeld, maar ik heb genoeg code gezien waarbij veel if statements op die manier in elkaar zitten.

Dat is inderdaad een ander verhaal...want hier was het inderdaad niet aan de orde. Hier is het blijkbaar ook geen nullable property (je kan best een nullable bool aanmaken), dat wist ik nog niet . Bedankt BM voor het uitproberen .

quote:

Niemand_Anders schreef op woensdag 16 juli 2008 @ 11:45:
[...]

*knip*

Persoonlijk ga ik al als een stier tekeer als een developer een statement als catch(Exception ex) gebruikt (je behoort alleen fouten af te vangen welke je verwacht), echter na het zien van bovenstaande code (van een ervaren programmeur) ben ik even 10 minuten buiten gaan staan.

Waarom hoor je alleen fouten af te vangen welke je verwacht? Wat als er een fout optreed die je niet verwacht?!? Ik bedoel: Kan je dit voor mij beredeneren?

quote:

Niemand_Anders schreef op woensdag 16 juli 2008 @ 13:55:
[...]
Ik heb zelfs programmeurs gehad welke aangaven geen logging te gebruiken vanwege de negatieve impact op de performance. Ook onze final releases worden gewoon met debug symbols verscheept. Bij productie fouten is een leesbare stacktrace van onschatbare waarde. De ingeleverde performance neem ik dan heel erg graag voor lief als ik dan bij een probleem weet dat het probleem optrad in bestand x.cs op regel y in plaats van 'RenderReport() +631'.

Inleveren van performance is ook onzine als je je log statements tussen if statements zet. Dan merk je er runtime niets van als je je loglevel op error of warn hebt staan. Maar voor debuggen is het erg fijn als je je loglevel even kan aanpassen naar debug
Java:

1 2 3 4

if(log.isDebugEnabled())  {   log.debug("dump van grote collection var: "+hugeCollection); }

quote:

Niemand_Anders schreef op woensdag 16 juli 2008 @ 13:55:
Ik heb zelfs programmeurs gehad welke aangaven geen logging te gebruiken vanwege de negatieve impact op de performance.

Hangt er een beetje vanaf waar/hoe vaak die logging code aangeroepen wordt, natuurlijk.

quote:

Ook onze final releases worden gewoon met debug symbols verscheept. Bij productie fouten is een leesbare stacktrace van onschatbare waarde.

De aanwezigheid/afwezigheid van debug symbols maakt voor performance niet echt uit, je moet alleen grotere binaries doorsturen/opslaan/inladen, en daarom is het "netter" om ze in een productieomgeving achterwege te laten. Voor sommige mensen speelt ook nog dat de aanwezigheid van debuginformatie cracking/reverse engineering eenvoudiger maakt.

Maar je weet hopelijk wel dat je b.v. een coredump van een gestripte binary prima kunt debuggen met de originele niet-gestripte variant? Je kunt een build mét debug symbols maken, een gestripte versie deployen, en coredumps uit de productieomgeving lokaal analyseren zonder enige verlies van functionaliteit. (Betekent wel dat je inderdaad in de productieomgeving geen stacktraces kunt generen, maar debuggen wil je daar toch niet doen.)

Een groter probleem is dat core dumps lastiger te analyseren worden naarmate je meer optimalisaties toepast. (Optimalisatienivo's waarbij de stack frames niet volledig worden opgebouwd zijn bijvoorbeeld heel irritant) Je zou dan eigenlijk met weinig/geen optimalisaties willen builden, maar dat heeft natuurlijk wél een significant effect op de performance.

quote:

daanmsvl schreef op woensdag 16 juli 2008 @ 14:36:
[...]

Waarom hoor je alleen fouten af te vangen welke je verwacht? Wat als er een fout optreed die je niet verwacht?!? Ik bedoel: Kan je dit voor mij beredeneren?

Fouten die je niet verwacht moeten gewoon doorkomen tot het hoogste nivo en alleen eventueel opgevangen te worden in een uncaught exception handler die een dikke vette foutmelding, of minstens een grote rode log wegschrijft.

Fouten dienen namenlijk afgehandeld te worden, en als je niet weet wat er fout is gegaan kun je het ook niet goed afhandelen. Als er een fout optreed waar je niet op gerekend had dan is er blijkbaar wat mis met het programma.

quote:

daanmsvl schreef op woensdag 16 juli 2008 @ 14:36:
[...]

Waarom hoor je alleen fouten af te vangen welke je verwacht? Wat als er een fout optreed die je niet verwacht?!? Ik bedoel: Kan je dit voor mij beredeneren?

Omdat je voor fouten die je niet verwacht ook geen afhandeling kan geven. Dit zal dus ook niet door jouw laag afgehandeld moeten worden, maar door de laag er boven (of hoger) tot het uiteindelijk door de GUI, VM o.i.d. wordt afgevangen, die er wel wat mee kan. (Melding naar gebruiker geven, VM plat gooien) Fouten opslokken of half afvangen heeft niemand wat aan.

Dit soort constructie "verbergt" ook vaak andere, programmeer, fouten, zoals NullPointerExceptions.
Bijvoorbeeld:
Java:

1 2 3 4 5 6

Product product = productDAO.getProduct(); try {    product.verwerk(); } catch (Exception exception) {    throw new ProductNotFoundException(); }

Dit is in ieder geval netter (en ook efficienter):
Java:

1 2 3 4 5 6

Product product = productDAO.getProduct(); if (product == null) {     throw new ProductNotFoundException(); } else {     product.verwerk(); }

Zeker als product.verwerk() ook nog een exceptie kan gooien. Bij het eerste voorbeeld slok je deze dus op met een ProductNotFoundException. Bij het tweede voorbeeld zou je deze exceptie wel expliciet te zien krijgen.

@MarcJ & Niemand_Anders:
Dat mijn stukje invoer van een gebruiker zou afhandelen en de gebruiker daardoor geen melding zou krijgen is een aanname die jullie doen en die helemaal niet uit mijn code voorbeeld blijkt .

@Nick_S & RWB: helder, dank

daanmsvl wijzigde dit bericht 16-07-2008 15:37 (38%)
Reden: vergeten

In een financieel pakket kwam ik o.a. dit juweeltje tegen:
Visual Basic:

1 2 3 4 5

antw = vbNo While antw = vbNo     antw = MsgBox("Geen invoer records, dus geen exportbestand. ", _     vbYesNo + vbDefaultButton2 + vbCritical, App.EXEName) Wend

Waarom iemand hier een vbYesNo neerzet en bij vbNo de msg opnieuw laat zien is mij een raadsel.

quote:

Janoz schreef op woensdag 16 juli 2008 @ 15:25:
@MarcJ & Niemand_Anders:
Dat mijn stukje invoer van een gebruiker zou afhandelen en de gebruiker daardoor geen melding zou krijgen is een aanname die jullie doen en die helemaal niet uit mijn code voorbeeld blijkt .

Ik neem aan dat je applicatie zelf getallen niet als string doorgeeft, dus is de aanname dat de invoer van buitenaf (url parameters, form post, command line arguments, etc) komt zeer plausibel.

Conversie fouten mogen gewoon niet genegeerd worden. In het minste geval moet de fout worden gelogged (bijvoorbeeld bij een automatische import van een product lijst).

quote:

Janoz schreef op woensdag 16 juli 2008 @ 15:25:
@MarcJ & Niemand_Anders:
Dat mijn stukje invoer van een gebruiker zou afhandelen en de gebruiker daardoor geen melding zou krijgen is een aanname die jullie doen en die helemaal niet uit mijn code voorbeeld blijkt .

Daarom vroeg ik ook om een voorbeeld te geven. Wanneer ga je een String parsen als int (of een ander type), welke geen gebruikersinvoer is op een of andere manier?

quote:

denheer schreef op woensdag 16 juli 2008 @ 15:45:
In een financieel pakket kwam ik o.a. dit juweeltje tegen:
Visual Basic:

1 2 3 4 5

antw = vbNo While antw = vbNo     antw = MsgBox("Geen invoer records, dus geen exportbestand. ", _     vbYesNo + vbDefaultButton2 + vbCritical, App.EXEName) Wend

Waarom iemand hier een vbYesNo neerzet en bij vbNo de msg opnieuw laat zien is mij een raadsel.

Ha ha, en dan ook nog de vbNo default maken...