[alg] Slechtste programmeervoorbeelden deel 4

[Paprika]

Ik ben zeker geen held in PHP, maar dit vond ik wel sterk:

PHP:

<?php
$huidige_tijd_een = date('G');
$huidige_tijd_twee = date('i');
$huidige_tijd_een = $huidige_tijd_een * 100;
$huidige_tijd_final = $huidige_tijd_een + $huidige_tijd_twee;
if ($huidige_tijd_final >= 2330 && $huidige_tijd_final <= 2359)
{
// een functie
}
else 
{
// was leeg
}
?>

Op z'n minst zou dat toch wel zoiets kunnen zijn, of mis ik nou iets?

PHP:

<?php
$huidige_tijd = date('Gi');
if ($huidige_tijd >= 2330)
    echo "Ja";
else 
    echo "Nee";
?>

[ Voor 3% gewijzigd door Paprika op 24-09-2011 15:08 ]

[Onbekend]

$huidige_tijd_final = date('G') * 100 + date('i'); is het zelfde als die vier regels, maar date('Gi') gebruiken is ook goed.

(Je mist trouwens wel nog een if-conditie, maar daar ging het niet om denk ik....)

[Paprika]

Springt ie niet vanzelf naar 00:00 bij 12 uur 's nachts waardoor die 2e if niet nodig zou zijn (gezien na 2359 0000 zou komen)?

[Onbekend]

Daar heb je inderdaad gelijk in. Ik ging er even snel van uit dat het maximum 2399 was.

[Patriot]

Phoenix1337 schreef op vrijdag 23 september 2011 @ 08:33:
[...]


Het gaat wel gebeuren ik heb in januari nog een afstudeerproject nodig.

Dingen herschrijven (en zeker je gehele codebase) mag je over het algemeen voor niks, of in ieder geval heel weinig doen Ervan uitgaande dat je een stagiaire bent kost je dus niet zoveel

Of je hebt het geluk dat het mag omdat het voor een afstudeerproject is. Maar als je héél véél mazzel hebt, dan is het gewoon een baas waarvan het mag. Moet je hééél zuinig op zijn, want dat kom je nooit meer tegen

[MBV]

Ik heb afgelopen week nog een heel stuk herschreven. Het skelet laten staan, en alle code vervangen door nieuwe, waarmee de gewenste functionaliteit wel mogelijk was. Met de oude structuur was dat nauwelijks te doen, dat had veel meer tijd gekost aan bugs fixen (hoop ik tenminste, het is nog niet af )

[ Voor 7% gewijzigd door MBV op 24-09-2011 23:13 ]

[Tim]

Paprika schreef op zaterdag 24 september 2011 @ 15:23:
Springt ie niet vanzelf naar 00:00 bij 12 uur 's nachts waardoor die 2e if niet nodig zou zijn (gezien na 2359 0000 zou komen)?

Eens in de zoveel jaar niet: Wikipedia: Schrikkelseconde

[Matis]

Tim schreef op zondag 25 september 2011 @ 17:48:
Eens in de zoveel jaar niet: Wikipedia: Schrikkelseconde

Ik zie heel Nederland als staan:

10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 1, 0 Gelukkig nieuwjaar!

[Alex)]

C#:

protected void save()
{

500 regels verder...

C#:

}

Geweldig weer

[bindsa]

Alex) schreef op maandag 26 september 2011 @ 10:02:

C#:

protected void save()
{

500 regels verder...

C#:

}

Geweldig weer

Wat is daar mis mee dan? Dat is niet heel raar IMHO, gewoon dingen collapsen in je editor en gaan

[Janoz]

T.H. Lassche schreef op maandag 26 september 2011 @ 11:37:
[...]


Wat is daar mis mee dan?

Alles. Een class met dit aantal regels zou ik al als een potentieel lelijk ding willen aanmerken, laat staan een enkele methode. En dan heb ik het nog niet eens over de naamgeving. Als je 500 regels nodig hebt gebeurt er vast veel meer dan alleen 'save'.

[Slurpie]

Opzich is het niet zo raar, er kan een hoop moeten gebeuren voordat er iets is opgeslagen.
Oa: validaties, meldingen creeeren , gegevens verwerking en na controle van de save actie .500 regels hoeft niet te zeggen dat alles wat er gebeurt daar is uit getyped. Alleen de aan roepen en verwerking van diverse objecten kan al een aardig aantal regels in beslag nemen.

[TJHeuvel]

Alex) schreef op maandag 26 september 2011 @ 10:02:

C#:

protected void save()
{

500 regels verder...

C#:

}

Geweldig weer

C#:

public class Inrichten
{
 public static Inrichten Instance;

1653 regels verder...

C#:

}

De 'god' klass van een huidig project, verantwoordelijk voor zo'n 50% van de gehele applicatie. Uiteraard om hem makkelijk toenaderbaar te maken staat 'ie in de global namespace, en is het een singleton.



[/code]

[Phoenix1337]

Ik heb hier een class 2312 regels code helft van de functies is static, de andere helft niet en het ergste is nog dat hij intern instanties van zichzelf aanmaakt om daar dan weer een functie op aan te roepen.

[alienfruit]

Maar laat Jenkins e.d. niet het alarm afgaan bij zulke grote klasses? Dat is toch het hele idee van Jenkins?

[Dragor]

alienfruit schreef op maandag 26 september 2011 @ 14:49:
Maar laat Jenkins e.d. niet het alarm afgaan bij zulke grote klasses? Dat is toch het hele idee van Jenkins?

Jenkins is geen code-checking tool, maar een Continuous Integration Server. Tenzij jij een job maakt om de grootte van klasses te checken zal Jenkins er niets om geven.

[Sv3n]

Moet je wel zelf een plugin aanzetten die daar op checkt, standaard doet Jenkins dat niet. Overigens is het imo. handiger om dan in je IDE al die tool aan te zetten en daar de warnings te zien. Of als je nog strenger wilt zijn een hook op je svn/git die commits die niet voldoen gewoon niet accepteert.

[Janoz]

Slurpie schreef op maandag 26 september 2011 @ 11:59:
Opzich is het niet zo raar, er kan een hoop moeten gebeuren voordat er iets is opgeslagen.
Oa: validaties, meldingen creeeren , gegevens verwerking en na controle van de save actie .500 regels hoeft niet te zeggen dat alles wat er gebeurt daar is uit getyped. Alleen de aan roepen en verwerking van diverse objecten kan al een aardig aantal regels in beslag nemen.

Het is zeker wel raar. Met alleen het aanroepen kom je echt niet tot 20 schermen aan code. Dit is per definitie een onderhoudshel. Ik kan me er niks anders bij voorstellen.

alienfruit schreef op maandag 26 september 2011 @ 14:49:
Maar laat JenkinsSonar e.d. niet het alarm afgaan bij zulke grote klasses? Dat is toch het hele idee van JenkinsSonar?

FTFY

[ Voor 16% gewijzigd door Janoz op 26-09-2011 15:15 ]

[alienfruit]

Sonar??? Verder zijn er zulke plugins voor Eclipse dan? Ik heb zelf ooit een scriptje gescreven die de regels telt...

[Sv3n]

http://www.sonarsource.org/ dus
Checkstyle kan het ook.
PMD waarschijnlijk ook wel.

[Janoz]

sonar is een aggregerende tool van (oa) rapportages van checkstyle, pmd en vele andere metrics tooling. Ideaal om de kwaliteit van je project in de gaten te houden. En ja, er is voor sonar ook een eclipse plugin.

[.oisyn]

Janoz schreef op maandag 26 september 2011 @ 11:54:
[...]

Alles. Een class met dit aantal regels zou ik al als een potentieel lelijk ding willen aanmerken, laat staan een enkele methode. En dan heb ik het nog niet eens over de naamgeving. Als je 500 regels nodig hebt gebeurt er vast veel meer dan alleen 'save'.

En door de zin die erna kwam ging je sarcasme-alarm niet af?

[Janoz]

Nee, nog niet. Teveel prutsers in mijn vakgebied tegen gekomen waardoor ik niet meer sta te kijken van de meest vreemde beweringen die met droge ogen gedaan worden ..

[SjaakCobain]

Phoenix1337 schreef op maandag 26 september 2011 @ 12:03:
Ik heb hier een class 2312 regels code helft van de functies is static, de andere helft niet en het ergste is nog dat hij intern instanties van zichzelf aanmaakt om daar dan weer een functie op aan te roepen.

Daar praat je volgens mij ook over stuk antiek van 10 jaar oud.

[Phoenix1337]

SjaakCobain schreef op maandag 26 september 2011 @ 16:23:
[...]

Daar praat je volgens mij ook over stuk antiek van 10 jaar oud.

Hoezo volgens mij je weet over welke class ik het heb toch ?

[.oisyn]

Phoenix1337 schreef op maandag 26 september 2011 @ 12:03:
Ik heb hier een class 2312 regels code helft van de functies is static, de andere helft niet en het ergste is nog dat hij intern instanties van zichzelf aanmaakt om daar dan weer een functie op aan te roepen.

Dat klinkt als jQuery

[Grijze Vos]

SjaakCobain schreef op maandag 26 september 2011 @ 16:23:
[...]

Daar praat je volgens mij ook over stuk antiek van 10 jaar oud.

En dat maakt het dan ineens begrijpelijk ofzo? :x "Seperations of Concerns" wordt al langer dan 10 jaar gebezigd in ons vakgebied.

[SjaakCobain]

Dat niet, maar ik heb voor het bedrijf gewerkt waar phoenix nu nog voor werkt, dus ik weet inderdaad over welke class het gaat

[Verwijderd]

Janoz schreef op maandag 26 september 2011 @ 11:54:
Als je 500 regels nodig hebt gebeurt er vast veel meer dan alleen 'save'.

Hoeft niet noodzakelijkerwijs hoor. Als je eens in de gaten gaat houden hoe ongelooflijk slecht zaken worden geoptimaliseerd anno nu dan zou het mij niet eens verbazen dat er iemand gewoon echt geen idee had van hoe je efficient één specifieke functie uitvoert.

Hoezo, cynisch?...

[alienfruit]

Zometeen maar eens slijmen voor een nieuwe stoel!

[Haan]

C#:

DateTime dt = DateTime.Now;
try
{
    service.WriteMessage(
        string.Format(string.Format("{0}:{1}:{2} {3}", dt.Hour.ToString("d2"),
                                    dt.Minute.ToString("d2"), dt.Second.ToString("d2"),
                                    p_sMessage)));
}

Gebeurt nog zeer regelmatig dat mensen niet lijken te weten dat DateTime patterns niet voor niets bestaan

[ValHallASW]

https://extranet.fisher.co.uk/insight2_fr/mainSearch.do

FFFFFFFFFFFFFFUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUU

Het is zo'n zoekfunctie a la conrad, waar je niets kunt vinden. Als je in een nieuwe tab een andere zoekopdracht invult dan wordt de zoekopdracht in je eerste tab ook anders. Zoeken met aanhalingstekens werkt niet, en zoeken op iets anders dan vrije tekst kan niet. Succes met het zoeken van een 4mm T-stukje. In het Frans trouwens, want de artikelcodes zijn per land anders.

Oh, maar dan zijn we er nog niet. Als een site kut is dan kan je altijd nog een catalogus-pdfje pakken. Altijd handig. Probeer maar: http://www.fr.fishersci.c...ues&id_site=FFR&langue=FR

Kijk, er zijn er een aantal Disponible en PDF. Nouja, dat PDF moet je met een korreltje zout nemen. Het is namelijk een flash-bladeraar: http://www.fisherscientif...s.com/pompe-raccord-tube/ . En nee, die leest ook niet op de achtergrond PDFjes in: wel een flashfile per pagina.

Maar ValHallASW, er zit een printknop, dus dan maak je toch gewoon zelf een PDFje?



Af en toe vraag ik me toch af of dit soort leveranciers wel iets willen verkopen. Ze zijn namelijk /allemaal/ zo hopeloos....

[ Voor 5% gewijzigd door ValHallASW op 27-09-2011 14:16 ]

[Styxxy]

Haan schreef op dinsdag 27 september 2011 @ 13:43:
Gebeurt nog zeer regelmatig dat mensen niet lijken te weten dat DateTime patterns niet voor niets bestaan

Dat is gelukkig nog één goed ding dat ze ons aanleren in school (ook al vergeet de helft dat dan ook weer).

[Grijze Vos]

Deze ben ik vandaag ook al minstens 15x tegengekomen:

C#:

try
{

  [40+ regels code]
}
catch(Exception ex)
{
  throw;
}

Kabouter Wesley kom er maar in: "en wat is daar het praktisch nut van".

[.oisyn]

Het praktisch nut is het kunnen plaatsen van breakpoints.

[ Voor 24% gewijzigd door .oisyn op 27-09-2011 15:46 ]

[Sv3n]

Uncaugth excepties breakpointen kan in C# toch ook gewoon neem ik aan?

[roy-t]

Sv3n schreef op dinsdag 27 september 2011 @ 15:54:
Uncaugth excepties breakpointen kan in C# toch ook gewoon neem ik aan?

Ja hoor je kunt zelfs kiezen of je wilt breaken op first chance exceptions (dus ook exceptions die gebeuren in ee try-catch block) of alleen uncaught exceptions. Ik vermoed dat diegene die daar throw heeft gezet beide niet wist .

[.oisyn]

Daarbij gaan jullie beide even compleet voorbij aan het feit dat die feature álle exception van een bepaald type catcht, en niet alleen diegene die gethrowd worden in dat stuk code.

[Slurpie]

Janoz schreef op maandag 26 september 2011 @ 15:14:
[...]

Het is zeker wel raar. Met alleen het aanroepen kom je echt niet tot 20 schermen aan code. Dit is per definitie een onderhoudshel. Ik kan me er niks anders bij voorstellen.

Met alleen aanroepen aanzich kom je er inderdaad niet aan, daarom zij ik ook met checks en andere meuk misschien wel en dan hoeft het niet eens een onderhouds hel te zijn mits er voldoende commentaar bij staat.

Daarbij vind ik het nog al raar als een klasse of een functie niet meer dan een bepaald aantal regels zou mogen zijn omdat het anders per definitie slecht is.

Liever wat meer regels dan 100 functie aanroepen die meer dan 200 karakters lang zijn imho.

[YopY]

Haan schreef op dinsdag 27 september 2011 @ 13:43:
Gebeurt nog zeer regelmatig dat mensen niet lijken te weten dat DateTime patterns niet voor niets bestaan

Of het is een aanwijzing / 'code smell' dat de patterns te complex / niet vanzelfsprekend zijn . Een alternatieve aanpak is JodaTime's DateTimeFormatterBuilder, dat iets meer 'in code' is.

[Grijze Vos]

Kom nu weer een leuke tegen.

Alle functies die via een webservice worden aangeroepen hebben het volgende pattern:

C#:

SomeSingleton.SecurityContext = ValidateToken(token); // throwt als user geen valid token heeft
return SomeFunction(...);

En hierbij gebruikt SomeFunction al dan niet de singleton die erboven geset wordt.
Werkelijk een godswonder dat dit nog geen bug heeft opgeleverd in de testfase.

[alex3305]

Grijze Vos schreef op woensdag 28 september 2011 @ 13:08:
[...]

En hierbij gebruikt SomeFunction al dan niet de singleton die erboven geset wordt.
Werkelijk een godswonder dat dit nog geen bug heeft opgeleverd in de testfase.

Hopelijk niet alleen positief getest ? Dat doen sommige mensen namelijk wel, en dan claimen ze dat alles bugvrij is. Als je dan echter negatief gaat testen...

[Verwijderd]

alex3305 schreef op woensdag 28 september 2011 @ 13:12:
[...]

Hopelijk niet alleen positief getest ? Dat doen sommige mensen namelijk wel, en dan claimen ze dat alles bugvrij is. Als je dan echter negatief gaat testen...

Goed punt. Scenario's testen waarvan je zeker weet dat je code er doorheen komt... ja, zo kan iedereen het wel.

[Janoz]

Slurpie schreef op dinsdag 27 september 2011 @ 16:46:
Met alleen aanroepen aanzich kom je er inderdaad niet aan, daarom zij ik ook met checks en andere meuk misschien wel en dan hoeft het niet eens een onderhouds hel te zijn mits er voldoende commentaar bij staat.

Laat me asjeblieft een voorbeeld zien van een methode van 500 (!) regels code die niet korter zou kunnen. En als die 500 regels ook nog vol staan met allemaal checks dan wil ik niet eens weten in welke richting de cyclomatic complexity van die methode gaat.

Daarbij vind ik het nog al raar als een klasse of een functie niet meer dan een bepaald aantal regels zou mogen zijn omdat het anders per definitie slecht is.

Misschien vind jij het raar, maar in het veld is het een heel gebruikelijke metric om kwaliteit en onderhoudbaarheid van een stuk software te kunnen bepalen. Wat precies de grenzen zijn wordt uiteraard druk bediscussieerd, maar dat zijn discussies over aantallen van 5 tot max 20. Iedereen die serieus bezig is met de kwaliteit van software zal met afkeer naar methoden van 500 regels kijken.

Liever wat meer regels dan 100 functie aanroepen die meer dan 200 karakters lang zijn imho.

Wanneer je überhaupt in 1 methode al 100 andere functies aanroept kan ik me onmogelijk voorstellen dat die enkele methode nog door een mens te bevatten is. Daarnaast kan ik me ook niet voorstellen dat die methode niet in duidelijkere en behapbaardere delen op te splitsen is. Methoden korter maken is niet simpel het aantal regels terugbrengen door minder commentaar erin te zetten en meer op dezelfde regel te zetten. Methoden maak je korter door bepaalde delen te extracten naar aparte methoden met duidelijke naamgeving. Op die manier doet de originele methode nog steeds hetzelfde, maar is de inhoud voor een ontwikkelaar een stuk behapbaarder (en ook nog eens makkelijker te testen)

[Verwijderd]

Los daarvan is het sowieso ook efficienter om dan toch maar wél wat meer algemene functies, classes, methods, etc aan te maken die in zo'n log beest worden gebruikt.

Want wie weet waar je die algemene functies, etc nog meer voor zou kunnen gebruiken?

Wat ik bedoel is dat ik mij niet kan voorstellen dat die 500 regels code allemaal volledig uniek zijn en enkel en alleen van toepassing zijn op die specifieke monster functie.

[SPee]

Slurpie schreef op dinsdag 27 september 2011 @ 16:46:
[...]
Met alleen aanroepen aanzich kom je er inderdaad niet aan, daarom zij ik ook met checks en andere meuk misschien wel en dan hoeft het niet eens een onderhouds hel te zijn mits er voldoende commentaar bij staat.

Daarbij vind ik het nog al raar als een klasse of een functie niet meer dan een bepaald aantal regels zou mogen zijn omdat het anders per definitie slecht is.

Must read: Clean code

Teveel regels code maakt het moeilijk om terug te lezen en begrijpen wat er allemaal gebeurd.
Het uit elkaar trekken van grote lappen code maakt het overzichtelijker (als je het goed doet!). Wat zie je liever, 1 functie aanroep waar alle validaties gedaan worden, of alle validaties op willekeurige plekken in je methode?
Het is niet "absoluut verboden" om lange functies te hebben. Je moet daar alleen een verdomd goede reden voor hebben. En veelal is die er niet.
En met kleinere stukjes code is het beter te lezen en is soms die commentaar niet nodig. Ik zou zeggen: lees het boek, staat er allemaal in!

[Grijze Vos]

alex3305 schreef op woensdag 28 september 2011 @ 13:12:
[...]

Hopelijk niet alleen positief getest ? Dat doen sommige mensen namelijk wel, en dan claimen ze dat alles bugvrij is. Als je dan echter negatief gaat testen...

Nee hoor, ze hebben een simultane test gedaan met 4 gebruikers die tegelijkertijd shit lopen te doen.
Nu is de kans best klein dat de ene thread de andere net inhaalt in die paar ms, maar zeker niet onmogelijk.

[alex3305]

Grijze Vos schreef op woensdag 28 september 2011 @ 13:44:
[...]

Nee hoor, ze hebben een simultane test gedaan met 4 gebruikers die tegelijkertijd shit lopen te doen.
Nu is de kans best klein dat de ene thread de andere net inhaalt in die paar ms, maar zeker niet onmogelijk.

Even lekker

C#:

Thread.sleep(100);

gebruiken . Dat is pas bugfixen .

[Zsub]

Sure is Wij hadden een probleem met login en opslaan van credentials.

Even wait()'en loste dat prima op, alsdus mijn baas

[MBV]

Janoz schreef op woensdag 28 september 2011 @ 13:22:
[...]

Laat me asjeblieft een voorbeeld zien van een methode van 500 (!) regels code die niet korter zou kunnen. En als die 500 regels ook nog vol staan met allemaal checks dan wil ik niet eens weten in welke richting de cyclomatic complexity van die methode gaat.

SaveSettings(), die bij alle componenten in het systeem langsgaat om alle settings op te slaan (en dus Whatever.Save(filestream) voor 200 componenten aanroept, met een separator ervoor en erna zit je aan 600 regels). Of hadden ze daar observer voor uitgevonden?

De langste functies die ik zelf heb geschreven zaten in een parser: in de java grammar zitten soms 30 elementen, en als je dan voor elk element op een andere manier wilt kunnen aggregeren, dan moet je wat. De langste zat zeker aan 200 regels.

[...]

Misschien vind jij het raar, maar in het veld is het een heel gebruikelijke metric om kwaliteit en onderhoudbaarheid van een stuk software te kunnen bepalen. Wat precies de grenzen zijn wordt uiteraard druk bediscussieerd, maar dat zijn discussies over aantallen van 5 tot max 20. Iedereen die serieus bezig is met de kwaliteit van software zal met afkeer naar methoden van 500 regels kijken.

Het is ook de beste metric die ik tot nu toe ben tegen gekomen, maar 20 is wel erg weinig. 2 schermen was toch ongeveer de limiet? Ik heb net een functie van 50 en 75* regels C++ geschreven om respectievelijk een configfile en een regel daaruit een te parsen, en ik heb geen idee hoe dat korter moet, of hoe ik het kleiner op kan hakken zonder dat het onoverzichtelijker wordt. Het doet niks meer dan met STL functies zoiets parsen: "Naam (5,85)", waarbij 5 t/m 85 een range is, en Naam moet worden opgezocht met een functie van een andere klasse. Cyclomatic complexity = 6, niet bijzonder hoog.

C++:

  for (; pinDescription != pinDescriptions.end(); pinDescription++)
  {
    //match the following syntax: <PinName>(<minimum>,<maximum>)
    //e.g. ADC_0(42,481)
    size_t startpos = 0;
    size_t endpos = pinDescription->find('(');
    if (endpos == aLine.npos) {
      LogError(logging::x, GetTestName().c_str() <<
               ": Couldn't parse pinDescription: missing ( in '" << pinDescription->c_str() << "'");
      return false;
    }
    string pinName = pinDescription->substr(startpos, endpos-startpos);
    //remove all spaces from the pinname
    pinName.erase(remove_if(pinName.begin(), pinName.end(), isspace), pinName.end());

    int minimum = -1;
    startpos = endpos + 1; //endpos was the character '(', now we need the character after
    endpos = pinDescription->find(',');
    if (endpos == aLine.npos) {
      LogError(logging::x, GetTestName().c_str() <<
               ": Couldn't parse pinDescription: missing , in '" << pinDescription->c_str() << "'");
      return false;
    }
    string minimumString = pinDescription->substr(startpos, endpos-startpos);
    istringstream(minimumString) >> minimum;

    int maximum = -1;
    startpos = endpos + 1; //endpos was the character ',', now we need the character after
    endpos = pinDescription->find(')');
    if (endpos == aLine.npos) {
      LogError(logging::x, GetTestName().c_str() <<
               ": Couldn't parse pinDescription: missing ) in '" << pinDescription->c_str() << "'");
      return false;
    }
    string maximumString = pinDescription->substr(startpos, endpos-startpos);
    istringstream(maximumString) >> maximum;

    LogInfo2(logging::x, GetTestName().c_str()
                                   << ": parsed pinDescription '" << pinDescription->c_str() << "' into "
                                   << pinName.c_str() << "(" << minimumString.c_str() << "," << maximumString.c_str() << ") into"
                                   << pinName.c_str() << "(" << minimum << "," << maximum << ")");

    Pin pin;
    if (!whatever->GetPin(pinName, pin))
    {
      LogError(logging::x, GetTestName().c_str() <<
               ": Couldn't find pinName " << pinName.c_str());
      return false;
    }
    else
    {
      iPinRanges[pin] = TRange(minimum, maximum);
      LogInfo(logging::x, GetTestName().c_str() <<
              ": Found Pin : " << pinName.c_str() << ",device=" << pin.Device << " id=" << pin.PinId);
    }
  }

[...]

Wanneer je überhaupt in 1 methode al 100 andere functies aanroept kan ik me onmogelijk voorstellen dat die enkele methode nog door een mens te bevatten is. Daarnaast kan ik me ook niet voorstellen dat die methode niet in duidelijkere en behapbaardere delen op te splitsen is. Methoden korter maken is niet simpel het aantal regels terugbrengen door minder commentaar erin te zetten en meer op dezelfde regel te zetten. Methoden maak je korter door bepaalde delen te extracten naar aparte methoden met duidelijke naamgeving. Op die manier doet de originele methode nog steeds hetzelfde, maar is de inhoud voor een ontwikkelaar een stuk behapbaarder (en ook nog eens makkelijker te testen)

Als het nog te bevatten is gaat het waarschijnlijk om een functie die je beter had kunnen laten afhandelen met een abbonnee-achtig systeem, zoals dat savesettings()-voorbeeld dat ik aan het begin gaf.

_{Ja, ik heb de eerste paar regels weggelaten, voor de oplettende lezer For-loopje dat pinDescriptions vult.}

[Patriot]

Janoz schreef op woensdag 28 september 2011 @ 13:22:
[...]

Laat me asjeblieft een voorbeeld zien van een methode van 500 (!) regels code die niet korter zou kunnen. En als die 500 regels ook nog vol staan met allemaal checks dan wil ik niet eens weten in welke richting de cyclomatic complexity van die methode gaat.


[...]

Misschien vind jij het raar, maar in het veld is het een heel gebruikelijke metric om kwaliteit en onderhoudbaarheid van een stuk software te kunnen bepalen. Wat precies de grenzen zijn wordt uiteraard druk bediscussieerd, maar dat zijn discussies over aantallen van 5 tot max 20. Iedereen die serieus bezig is met de kwaliteit van software zal met afkeer naar methoden van 500 regels kijken

Die max van 20, dat is dan puur code, mag ik hopen?

[Janoz]

Zodra het boven de 20 uitkomt is het niet per definitie slecht, maar voornamelijk niet meer per definitie goed. Je zult een reden moeten hebben waarom hij langer is. (Een equals methode van een domein object met veel properties bv). Echter, wanneer jij 10 regels commentaar nodig hebt bij 20 regels code, dan kun je je afvragen of die code wel zo best is.Ik maak hierbij trouwens onderscheid tussen commentaar die de werking van de code uitlegt (die dus tussen de code staat) en commentaar die uitlegt wat de methode doet (die dus boven de methode staat)

@MBV
Een parser is natuurlijk weer een verhaal apart. (grenzen zijn ook een beetje afhankelijk van het type software). Maar bij veel parsewerk zou ik overwegen om naar een parsergenerator te kijken.

[ Voor 15% gewijzigd door Janoz op 29-09-2011 09:25 ]

[Grijze Vos]

MBV schreef op woensdag 28 september 2011 @ 18:15:
[...]

SaveSettings(), die bij alle componenten in het systeem langsgaat om alle settings op te slaan (en dus Whatever.Save(filestream) voor 200 componenten aanroept, met een separator ervoor en erna zit je aan 600 regels).

Klinkt als een slecht ontwerp.

[Makkelijk]

PHP:

if( !stristr($_SERVER['REQUEST_URI'], 'ajax') ) {
    $_SERVER['REQUEST_URI'] = preg_replace(array(
        '/\/updates\/(.+)$/',
        '/\/inspanningen\/(.+)$/',
        '/\/idee\/(.+)$/',
        '/\/interessant\/(.+)$/'
    ), array(
        '/alle_inspanningen/updates/updates_item/t/$1' ,
        '/alle_inspanningen/alle_inspanningen_item/t/$1',
        '/alle_inspanningen/interessant_idee/interessant_idee_item/type/1/t/$1',
        '/alle_inspanningen/interessant_idee/interessant_idee_item/type/2/t/$1'
    ), $_SERVER['REQUEST_URI']);
}

I should be ashamed of myself...

[YopY]

Die C++ code / parser kan wel vereenvoudigd worden door het toepassen van een paar refactorings. Extract method voor de validaties (indien mogelijk exceptions gooien ipv controleren op een false return value). D'r zijn vast wel slimmere / kortere manieren om waarden uit een string te halen anders dan substrings. Ik zie ook wat duplicatie in het bepalen van de minimum en maximum - ook daar kun je weer een extract method op loslaten.

Grijze Vos schreef op donderdag 29 september 2011 @ 09:30:
[...]

Klinkt als een slecht ontwerp.

Klinkt als een bracque implementatie van het Visitor pattern. Snelle refactoring is om al die componenten in een lijst te zetten en te gaan itereren.

...Maar als al die componenten statics zijn (oid) wordt dat lastig.

@Makkelijk, was een gewone string replace (hihi ) niet makkelijker geweest dan een regex? (noot: dit komt van iemand die nog steeds geen regex geleerd heeft)

[ Voor 24% gewijzigd door YopY op 29-09-2011 10:51 ]

[MBV]

YopY schreef op donderdag 29 september 2011 @ 10:50:
Die C++ code / parser kan wel vereenvoudigd worden door het toepassen van een paar refactorings. Extract method voor de validaties (indien mogelijk exceptions gooien ipv controleren op een false return value). D'r zijn vast wel slimmere / kortere manieren om waarden uit een string te halen anders dan substrings. Ik zie ook wat duplicatie in het bepalen van de minimum en maximum - ook daar kun je weer een extract method op loslaten.

Android native code, dus geen exceptions, als je de validaties apart doet moet je volgens mij de string 2x parsen, en als er slimmere/kortere manieren zijn om die waarden eruit te halen dan zie ik die graag. Die 2 regels voor min/max zou je misschien kunnen vervangen door 1 regel functie, dat scheelt 3 regels ja, maar daar wordt het niet direct overzichtelijker van.
I.v.m. geen exceptions zal ik boost maar niet proberen, die leeft daarvan.

[...]

Klinkt als een bracque implementatie van het Visitor pattern. Snelle refactoring is om al die componenten in een lijst te zetten en te gaan itereren.

...Maar als al die componenten statics zijn (oid) wordt dat lastig.

Ik heb ze zowel voor wannabe-visitor als wannabe-observer gezien. Het was een antwoord op een opmerking dat de code dan nooit te begrijpen zou zijn. En ik heb dat wannabe-observer-antipattern al veel te vaak gezien, bij voorkeur met een functie willekeurig in het midden die iets totaal anders doet

@Makkelijk, was een gewone string replace (hihi ) niet makkelijker geweest dan een regex? (noot: dit komt van iemand die nog steeds geen regex geleerd heeft)

Die vraag zal ik even beantwoorden: in dit geval wel, aangezien hij is vergeten met ^ te markereren dat het aan het begin van de regel moet staan. /whatever/updates/blaat wordt dus ook vervangen, terwijl je dat meestal niet wilt.

[Slurpie]

Janoz schreef op woensdag 28 september 2011 @ 13:22:
[...]

Je geeft zelf al aan dat een parser hier al snel aan komt. Heb er zelf ook een paar gehad die door het aantal velden ruim 200 regels was. Echt 500 regels heb ik zelf nog nooit gehaald maar kan me wel voorstellen dat het is sommige gevallen wordt gehaald.

Daarbij ging het mij er om het punt dat het aantal regels van een functie/klasse bepaald of het goed in elkaar is gezet. Als een functie voor mij part 1000 regels. als daar een goede reden voor is dat prima. Het voorbeeld wat werdt gegeven was dat een functie Save werd aangeroepen en 500 regels pas ophield. Dat daarna gelijk hier iedereen er over heen valt zonder te weten of er een wel of niet een goede reden is.

Als er overmatige checks in voor komen. for veel geneste foreach of if else constructies dan mag je zeker wel af gaan vragen of het goed in elkaar steekt.

Code moet gewoon normaal lees baar zijn of het nu heel lang is maakt mij dan niet uit.

[Patriot]

Janoz schreef op donderdag 29 september 2011 @ 09:23:
Zodra het boven de 20 uitkomt is het niet per definitie slecht, maar voornamelijk niet meer per definitie goed. Je zult een reden moeten hebben waarom hij langer is. (Een equals methode van een domein object met veel properties bv). Echter, wanneer jij 10 regels commentaar nodig hebt bij 20 regels code, dan kun je je afvragen of die code wel zo best is.Ik maak hierbij trouwens onderscheid tussen commentaar die de werking van de code uitlegt (die dus tussen de code staat) en commentaar die uitlegt wat de methode doet (die dus boven de methode staat)

Het is dus gewoon een regel die nergens meer op slaat. Een methode van minder dan 20 regels is namelijk ook niet per definitie goed. Het is wat mij betreft geen regel als er zoveel uitzonderingen zijn.

[BenoitRoosens]

@MBV: ik merk nog enkele andere zaken op in jouw code:
1) std::(w)string*, wat is de reden hiervoor? Lijkt mij perfect mogelijk met stack semantics (sneller).
2) pinDescription++. Gebruik bij iterators, zeker in zo'n loops altijd de '++' prefix! Die is soms sneller, nooit trager. De postfix variant levert een extra temporary op voor niets. Sommige optimizers lezen hierdoor, maar gewoon best practice.
3) Initializeren van je locale variabelen kan sneller: int maximum(-1), size_t startpos(0), .... Deze constructor vorm is sneller met sommige compilers, nooit trager. Trouwens "Pin pin", is de default constructor daar goed genoeg?

In het algemeen stel ik dan de vraag ofdat je een C++11 feature enabled compiler gebruikt? Wat jij doet lijkt mij 100X makkelijker met regex...

[MBV]

@BenoitRoosens: Bedankt voor de kritiek, ook al had ik hem daarvoor niet neergezet. Jouw suggesties nr 2 en 3 maken mijn code niet makkelijker te lezen of korter, hooguit marginaal sneller. Ik hoop dat GCC die dingen al wegfiltert, zeker puntje 3. Maar wat bedoel je met stack semantics? Ik kan die kreet alleen op MSDN vinden.
Die Pin instantie wordt geinitialiseerd door de regel erna, dat is een out-parameter (je moet wat zonder exceptions). En C++0x: ik heb in mijn post erna al aangegeven dat het voor Android NDK is, het was al lang zoeken om daar uberhaupt STL te kunnen gebruiken...

Nadeel van regexes is dat het al snel write-only code wordt, en dat je geen idee hebt hoe snel de code is die wordt uitgevoerd. Maar je hebt gelijk: het grootste gedeelte kan je vervangen door:

code:

\w*\([a-zA-Z0-9]*\)\w*(\w*\([0-9]*\)\w*,\w*\([0-9]\)\w*)\w*$

Wat vind jij makkelijker te lezen? Dan ben ik benieuwd wat je aan escapes toe moet voegen om dit te laten bouwen in C++, waarschijnlijk elke \ dubbel zetten.

[.oisyn]

Maar wat bedoel je met stack semantics

Dat je dingen op de stack maakt ipv op de heap. Sowieso, gebruik nooit rauwe pointers, maar shared_ptr of unique_ptr.

[ Voor 27% gewijzigd door .oisyn op 29-09-2011 15:42 ]

[BenoitRoosens]

@MBV: De GCC optimizer zal inderdaad waarschijnlijk punt 3 voor jou doen, als je nu op oude Nintendo compilers werkt ofzo is het een ander verhaal . Punt 2 was gewoon een opmerking hoor, trouwens met iterators altijd de prefix gebruiken, std::map kan je rare kuren opleveren.

Welke GCC versie heb je? De laatste versies hebben std::regex, en dan kan je met volgens mij nog steeds even snel maar korter werken. Het manueel parsen van strings vind ik in het algemeen niet zo'n goed idee, het is tijdrovend en error prone. Die regex is nu niet de makkelijkste te lezen maar het kan beter:

code:

(\w*)[(](\d*)[,](\d*)[)]

Snel een commentaar erbij wat het is en klaar. Ik heb onmiddelijk capture groups ingesteld. Het wordt dan wel heel makkelijk:

C++:

using namespace std;
//Some comments about the regular expression.
const regex check("(\w*)[(](\d*)[,](\d*)[)]");

smatch match;
string input("ADC_0(42,481)");
bool matched(regex_match(input, match, check));

if(matched)
{
    //match[1] = ADC_0
    //match[2] = 42
    //match[3] = 481
}
else
{
    //Error.
}

Stack semantics zijn alles wat geen pointer is, en dus niet in de heap gealloceerd is maar op de stack:

C++:

string blah("test"); //Stack
auto blah(new string("test")); //Heap

Dit gaat gewoon sneller, intern zal de STL string dynamisch memory alloceren (behalve bij de small string optimization, in Visual C++ 160 characters), maar jij werkt met een stack variabele. Je moet niet inzitten met de memory te releasen eenmaal je ermee klaar bent, out of scope = no memory leak bij een stack variabele. Als die input string een input parameter is kan je die gewoon als (const) reference doorgeven, of als kopie mocht dit nodig zijn.

Ik hoop dat dit ietwat duidelijk is .

[Alex)]

HTML:

<asp:Button runat="server" CssClass="bigbutton" ID="Button6" Text="Refresh" OnClick="Button3_Click"></asp:Button>

[.oisyn]

BenoitRoosens schreef op donderdag 29 september 2011 @ 15:42:
Dit gaat gewoon sneller, intern zal de STL string dynamisch memory alloceren (behalve bij de small string optimization, in Visual C++ 160 characters)

16, inclusief de trailing zero voor c_str(), dus effectief 15.

[Rhapsody]

Alex) schreef op donderdag 29 september 2011 @ 15:48:

HTML:

<asp:Button runat="server" CssClass="bigbutton" ID="Button6" Text="Refresh" OnClick="Button3_Click"></asp:Button>

Het is bijna obfuscated ;-)

[MBV]

BenoitRoosens schreef op donderdag 29 september 2011 @ 15:42:
Welke GCC versie heb je? De laatste versies hebben std::regex, en dan kan je met volgens mij nog steeds even snel maar korter werken. Het manueel parsen van strings vind ik in het algemeen niet zo'n goed idee, het is tijdrovend en error prone. Die regex is nu niet de makkelijkste te lezen maar het kan beter:

code:

(\w*)[(](\d*)[,](\d*)[)]

Ik wilde even mijn punt duidelijk maken dus had niet echt mijn best gedaan om hem eenvoudig te lezen te maken Maar jouw regex slikt geen whitespace tussen de capture groups, toch? Aangezien ik nog wel eens zoiets wil kunnen doen:

code:

blaat  ( 5,10)
foo    (10, 5)

GCC versie die we gebruiken is 4.4.3, maar ik weet niet of regexes ondersteund worden. Ik kan me zo voorstellen dat dat exceptions nodig heeft. Oh ja, het moet ook kunnen bouwen in VS2005...

C++:

using namespace std;
//Some comments about the regular expression.
const regex check("(\w*)[(](\d*)[,](\d*)[)]");
//knip

Moet je \w niet escapen?

Stack semantics zijn alles wat geen pointer is, en dus niet in de heap gealloceerd is maar op de stack:

C++:

string blah("test"); //Stack
auto blah(new string("test")); //Heap

Dus je doelde op dit:

code:

string blah = "x"; //heap
string blah("x"); //stack

Sorry, te lang in Java geprogrammeerd In C++0x wordt dat wel weer geoptimaliseerd met move-semantics en weet ik veel Verder doe ik nergens expliciet new, dus als dat het niet is weet ik niet wat je bedoelt.

Als die input string een input parameter is kan je die gewoon als (const) reference doorgeven, of als kopie mocht dit nodig zijn.

Ik hoop dat dit ietwat duidelijk is .

Argument const& doen wist ik wel, maar hoe wist jij dat ik dat vergeten was? Intranet kent jou niet

[.oisyn]

MBV schreef op donderdag 29 september 2011 @ 16:17:
Dus je doelde op dit:

code:

string blah = "x"; //heap
string blah("x"); //stack

Nee, hij bedoeld dat je een string op de stack kan maken, en dat je een string op de heap kan maken met new (en dat je dan ook een string* hebt en geen string).

Optimalisaties zoals bovenstaande code (ctor initialization) gaan nergens over. Het enige probleem dat je mogelijk kan hebben is dat als T geen copy ctor heeft, T t = u niet legaal is en je wel T t(u) moet doen.

[Janoz]

Slurpie schreef op donderdag 29 september 2011 @ 14:04:
[...]


Je geeft zelf al aan dat een parser hier al snel aan komt. Heb er zelf ook een paar gehad die door het aantal velden ruim 200 regels was. Echt 500 regels heb ik zelf nog nooit gehaald maar kan me wel voorstellen dat het is sommige gevallen wordt gehaald.

Ik zie niet wat een parser met het aantal velden te maken heeft. Een parser wordt ingewikkelder wanneer de te parsen syntax ingewikkelder wordt. Maar wanneer dat het geval is kun je beter naar een parser generator kijken. Een parser generator kan rustig methoden van 100-en regels genereren (vaak joekels van switch statements) maar omdat dat niet door mensen gelezen hoeft te worden is dat geen probleem (parser generatoren genereren vaak ook code die bol staat van de goto's )

Daarbij ging het mij er om het punt dat het aantal regels van een functie/klasse bepaald of het goed in elkaar is gezet. Als een functie voor mij part 1000 regels. als daar een goede reden voor is dat prima. Het voorbeeld wat werdt gegeven was dat een functie Save werd aangeroepen en 500 regels pas ophield. Dat daarna gelijk hier iedereen er over heen valt zonder te weten of er een wel of niet een goede reden is.

Daar kan geen goede reden voor zijn. Zulke methoden worden enkel door mensen geschreven die (al dan niet bewust) schijt hebben aan de mensen die hun code moet gaan onderhouden (en het treurige is vaak dat ze dat waarschijnlijk zelf zijn)

Laat mij je code van 200 regels zien en ik maak er onderhoudbare code van.

Als er overmatige checks in voor komen. for veel geneste foreach of if else constructies dan mag je zeker wel af gaan vragen of het goed in elkaar steekt.

Code moet gewoon normaal lees baar zijn of het nu heel lang is maakt mij dan niet uit.

Een methode van 1000 of zelfs 200 regels is niet leesbaar.

Patriot schreef op donderdag 29 september 2011 @ 14:46:
Het is dus gewoon een regel die nergens meer op slaat. Een methode van minder dan 20 regels is namelijk ook niet per definitie goed. Het is wat mij betreft geen regel als er zoveel uitzonderingen zijn.

Als je met 50km/u door rood rijdt voldoe je keurig aan de regels omtrent de maximum snelheid, maar toch kun je een bekeuring krijgen. Is de snelheidslimiet nu ook een onzinregel?

[orf]

Makkelijk schreef op donderdag 29 september 2011 @ 10:20:

PHP:

if( !stristr($_SERVER['REQUEST_URI'], 'ajax') ) {
    $_SERVER['REQUEST_URI'] = preg_replace(array(
        '/\/updates\/(.+)$/',
        '/\/inspanningen\/(.+)$/',
        '/\/idee\/(.+)$/',
        '/\/interessant\/(.+)$/'
    ), array(
        '/alle_inspanningen/updates/updates_item/t/$1' ,
        '/alle_inspanningen/alle_inspanningen_item/t/$1',
        '/alle_inspanningen/interessant_idee/interessant_idee_item/type/1/t/$1',
        '/alle_inspanningen/interessant_idee/interessant_idee_item/type/2/t/$1'
    ), $_SERVER['REQUEST_URI']);
}

I should be ashamed of myself...

Yeah, you should.
Zijn dit je korte url's?

[BenoitRoosens]

MBV schreef op donderdag 29 september 2011 @ 16:17:
[...]

Ik wilde even mijn punt duidelijk maken dus had niet echt mijn best gedaan om hem eenvoudig te lezen te maken Maar jouw regex slikt geen whitespace tussen de capture groups, toch? Aangezien ik nog wel eens zoiets wil kunnen doen:

code:

blaat  ( 5,10)
foo    (10, 5)

GCC versie die we gebruiken is 4.4.3, maar ik weet niet of regexes ondersteund worden. Ik kan me zo voorstellen dat dat exceptions nodig heeft. Oh ja, het moet ook kunnen bouwen in VS2005...

[...]

Moet je \w niet escapen?

[...]

Dus je doelde op dit:

code:

string blah = "x"; //heap
string blah("x"); //stack

Sorry, te lang in Java geprogrammeerd In C++0x wordt dat wel weer geoptimaliseerd met move-semantics en weet ik veel Verder doe ik nergens expliciet new, dus als dat het niet is weet ik niet wat je bedoelt.

[...]

Argument const& doen wist ik wel, maar hoe wist jij dat ik dat vergeten was? Intranet kent jou niet

GCC 4.3.3 is al een oude versie (4.7 in de maak nu), dus vergeet die regex voor het moment . VS2005 is ook al niet meer van de nieuwste... Voor regex'en te testen raad ik ten sterkste de volgende website aan: http://gskinner.com/RegExr/ Deze laat je toe om in de webapp je regular expressions te testen, uitermate handig!

Argument const& doen wist ik wel, maar hoe wist jij dat ik dat vergeten was? Intranet kent jou niet
Een veelgemaakte fout, maar pure guess anders Ik gok dat pinDescription een parameter is, en dat je de pointer meegeeft aan die functie. Als dit het geval is zou je die beter als const reference doorspelen...

[Makkelijk]

MBV schreef op donderdag 29 september 2011 @ 11:07:
[...]
Die vraag zal ik even beantwoorden: in dit geval wel, aangezien hij is vergeten met ^ te markereren dat het aan het begin van de regel moet staan. /whatever/updates/blaat wordt dus ook vervangen, terwijl je dat meestal niet wilt.

Was ik het maar vergeten... nee, dit was de makkelijkste manier om het op localhost, in een submap, ook nog te laten werken . Maar als je de server array gaat overschrijven, dan ben je al zo verkeerd bezig dat dit ook niet meer uitmaakt We werken nog aan echte routing...

[CodeCaster]

quote: http://battlelog.battlefield.com/bf3/gate/?reason=chars

We are currently having issues with passwords containing
special characters (such as &%<>).

Until we have fixed this, you can change your password to
only use alphanumeric characters at http://profile.ea.com

_! Welkom in 2011, DICE.

[ Voor 3% gewijzigd door CodeCaster op 30-09-2011 14:39 ]

[Cloud]

Mijn god. Daar is de -smiley wel echt op van toepassing hoor. Voor een game die meer kost dan sommige Hollywood-producties is dat wel een fail van epische proporties.

[Davio]

Cloud schreef op vrijdag 30 september 2011 @ 14:46:
Mijn god. Daar is de -smiley wel echt op van toepassing hoor. Voor een game die meer kost dan sommige Hollywood-producties is dat wel een fail van epische proporties.

Is gewoon een iets te enthousiaste security developer aan de slag geweest.
Je weet maar nooit uit welke hoek die gekke SQL Injections en/of HTML fuckups komen.

Misschien mag "OR 1==1" ook wel niet als wachtwoord.

[Noxious]

Ik kwam vandaag dit pareltje tegen:


Wat overigens door ReSharper ( tool) werdt omgezet in:


Véél leesbaarder hoor

[Keiichi]

Noxious schreef op vrijdag 30 september 2011 @ 16:25:
Ik kwam vandaag dit pareltje tegen:
[afbeelding]

Wat overigens door ReSharper ( tool) werdt omgezet in:
[afbeelding]

Véél leesbaarder hoor

Misschien is de 'nieuwe' code wel wat sneller uitvoerbaar.

[Avalaxy]

Lijkt me stug, LINQ is over het algemeen gewoon een extra laag bovenop de foreach die dus alleen maar extra overhead toevoegt. Puur syntactische suiker.

[RobIII]

Avalaxy schreef op vrijdag 30 september 2011 @ 16:33:
Lijkt me stug, LINQ is over het algemeen gewoon een extra laag bovenop de foreach die dus alleen maar extra overhead toevoegt. Puur syntactische suiker.

Nou....

[Noxious]

Oh het zal mogelijk vast sneller zijn, dunno hoe een compiler daarmee omgaat, maar het ging mij vooral om de leesbaarheid

[Avalaxy]

RobIII schreef op vrijdag 30 september 2011 @ 16:35:
[...]

Nou....

Even lezen hoor, misschien dat ik er later op reageer

Ik zat er een tijdje terug nog naar te zoeken, hoe een foreach zich verhoudt tot een LINQ query, maar daar zag ik dat de LINQ vaak nog iets trager was (dat was op stack overflow).

[Patriot]

Janoz schreef op donderdag 29 september 2011 @ 21:30:
[...]

Als je met 50km/u door rood rijdt voldoe je keurig aan de regels omtrent de maximum snelheid, maar toch kun je een bekeuring krijgen. Is de snelheidslimiet nu ook een onzinregel?

Dat is een andere situatie. Zonder te diep in te gaan op de analogie (want een analogie blijft braque): Het rode licht is een extra beperking, geen uitzondering, op de snelheidsregel.

Natuurlijk zijn er altijd wel wat uitzonderingen voor regels te verzinnen, maar in dit geval vind ik gewoon dat er teveel mogelijke uitzonderingen zijn waar ook nog eens aardig wat discussie over mogelijk is. Het is meer een richtlijn voor jezelf (ik zit op de 30 regels, kan het misschien beter?) dan een regel waar je waarde aan zou moeten hechten.

[.oisyn]

RobIII schreef op vrijdag 30 september 2011 @ 16:35:
[...]

Nou....

Dat je met LINQ meer kan wil niet zeggen dat dat in elk geval ook opgaat. In de code van JapyDooge worden echt geen Expression objects gegenereerd, de lambda compileert gewoon naar een delegate die voor alle elementen wordt aangeroepen.

[RobIII]

.oisyn schreef op vrijdag 30 september 2011 @ 17:07:
[...]

Dat je met LINQ meer kan wil niet zeggen dat dat in elk geval ook opgaat.

Dat heb ik ook helemaal nergens gezegd; ik reageerde op:

Avalaxy schreef op vrijdag 30 september 2011 @ 16:33:
Lijkt me stug, LINQ is over het algemeen gewoon een extra laag bovenop ...

waarmee ik hoopte enige nuance te kunnen aanbrengen

[.oisyn]

Hij zei "over het algemeen", waarop jij reageerde met "nou...", waardoor je lijkt te impliceren dat de gepostuleerde stelling niet "over het algemeen" opgaat

[Avalaxy]

Hihi. Don't fight guys Het was sowieso een leerzame toevoeging, terecht of niet

[YopY]

Verplichte link naar TDWTF. Ik zou de code copy/pasten... maar daar is 'ie iets te breed voor .

[Onbekend]

Leuke wachtwoordgenerator. Alleen ik vertrouw hem niet zo erg.....

[Verwijderd]

YopY schreef op zaterdag 01 oktober 2011 @ 21:00:
Verplichte link naar TDWTF. Ik zou de code copy/pasten... maar daar is 'ie iets te breed voor .

zou btw wel goeie huiswerk opgave zijn hoeveel iteraties er plaats vinden;)
lijkt erg op iets wat ik gemaakt heb:

C++:

bool alAnimationList::onLoad( TiXmlElement *parent )
{
    TiXmlElement* sibbling = parent->FirstChildElement("nodename");
    //AAAARGGGGHHH
    if(sibbling)
    {
        const char* name = sibbling->Attribute("Value");
        if(name)
        {
            alSceneNode *nodePtr = NULL;
            
            wxString txt = alGlobalVars::constCharToWxString(name);
            if(txt == wxString(wxT("alCamera")))    //special case
            {
                //TODO fixme
                //nodePtr = alFrame::getActiveViewport()->getCamera();
            }
            else
            {
                nodePtr = alEngineInit::getWorld()->getNodeByName(txt);
            }
            if(nodePtr)
            {
                alAnimatableData *animDataPtr = new alAnimatableData();
                if( nodePtr->getAnimatableData(animDataPtr) )
                {
                    animDataPtr->setSceneNode(nodePtr);
                    
                    TiXmlElement *elem = parent->FirstChildElement("Animation");
                    if(elem)
                    {
                        for( elem; elem ; elem = elem->NextSiblingElement( ))
                        {
                            sibbling = elem->FirstChildElement("infostring");
                            if(sibbling)
                            {
                                name = sibbling->Attribute("Value");
                                if(name)
                                {
                                    txt = alGlobalVars::constCharToWxString(name);
                                    alAnimationData *dataNew = animDataPtr->getDataByName(txt);
                                    if(dataNew)
                                    {
                                        int id;
                                        sibbling = elem->FirstChildElement("identifier");
                                        sibbling->QueryIntAttribute("Value", &id);                                      
                                        if( id == dataNew->getIdentifier())
                                        {
                                            sibbling = elem->FirstChildElement("numanimsystems");
                                            sibbling->QueryIntAttribute("Value", &id);
                                            if( id == dataNew->getSize())
                                            {
                                                int cnt = 0;
                                                TiXmlElement *animSys = elem->FirstChildElement("animsystem");
                                                for( animSys; animSys ; animSys = animSys->NextSiblingElement())
                                                {
                                                    alAnimationSystem *systemPtr = dataNew->getAnimationSystem(cnt);
                                                    double r, g, b;
                                                    animSys->QueryDoubleAttribute("R", &r);
                                                    animSys->QueryDoubleAttribute("G", &g);
                                                    animSys->QueryDoubleAttribute("B", &b);
                                                    systemPtr->setCurveColor(alVector3f(r, g, b));

                                                    TiXmlElement *animSpline = animSys->FirstChildElement("animspline");
                                                    for( animSpline ; animSpline; animSpline = animSpline->NextSiblingElement())
                                                    {
                                                        double x, y, z;
                                                        double angle, length;
                                                        int splineid;
                                                        animSpline->QueryDoubleAttribute("X", &x);
                                                        animSpline->QueryDoubleAttribute("Y", &y);
                                                        animSpline->QueryDoubleAttribute("Z", &z);
                                                        animSpline->QueryIntAttribute("Id", &splineid);
                                                        animSpline->QueryDoubleAttribute("Angle", &angle);
                                                        animSpline->QueryDoubleAttribute("Length", &length);                                                        
                                                        
                                                        alAnimationSpline *spline = systemPtr->addPoint(alVector3d(x, y, z), splineid);
                                                        spline->setControlPointAngle(angle);
                                                        spline->setControlPointLength(length);
                                                        spline->updateControlPoints();
                                                    }
                                                    cnt++;
                                                }
                                            }

                                        }
                                    }
                                }
                            }
                        }
                    }
                    m_animations.push_back(animDataPtr);

                }
                else
                {
                    delete animDataPtr;
                }
            }
        }
    }   
    return true;
}

OUCHH;)

[R4gnax]

Noxious schreef op vrijdag 30 september 2011 @ 16:25:
Ik kwam vandaag dit pareltje tegen:
[afbeelding]

Wat overigens door ReSharper ( tool) werdt omgezet in:
[afbeelding]

Véél leesbaarder hoor

Ik maak eerder bezwaar tegen het feit dat de hele functionaliteit van die functie fout geimplementeerd is. Het feit dat de replacement niet atomisch is kan voor leuke problemen zorgen. Neem maar eens de environment variablen %FOO% en %BAR%, waarbij de waarde van variable %FOO% de string "%BAR%" is. Oops!

[Noxious]

R4gnax schreef op zondag 02 oktober 2011 @ 12:25:
[...]


Ik maak eerder bezwaar tegen het feit dat de hele functionaliteit van die functie fout geimplementeerd is. Het feit dat de replacement niet atomisch is kan voor leuke problemen zorgen. Neem maar eens de environment variablen %FOO% en %BAR%, waarbij de waarde van variable %FOO% de string "%BAR%" is. Oops!

Dat zou inderdaad een issue kunnen zijn ja

[Janoz]

Patriot schreef op vrijdag 30 september 2011 @ 16:56:
Dat is een andere situatie. Zonder te diep in te gaan op de analogie (want een analogie blijft braque): Het rode licht is een extra beperking, geen uitzondering, op de snelheidsregel.

Ik reageer op "Een methode van minder dan 20 regels is namelijk ook niet per definitie goed."en daarop is die analogie exact van toepassing. Je houden aan 1 regel betekend nog niet dat je niet een andere regel overtreden hebt.

Natuurlijk zijn er altijd wel wat uitzonderingen voor regels te verzinnen, maar in dit geval vind ik gewoon dat er teveel mogelijke uitzonderingen zijn waar ook nog eens aardig wat discussie over mogelijk is. Het is meer een richtlijn voor jezelf (ik zit op de 30 regels, kan het misschien beter?) dan een regel waar je waarde aan zou moeten hechten.

Teveel uitzonderingen? Hoeveel uitzonderingen zijn er dan? Voor zover ik weet maar 1 "Heb een duidelijk verhaal waarom het echt niet korter kan"

Verder wisselt het max natuurlijk. Voor een object georiënteerde implementatie van een simpele CRUD applicatie is hij anders dan een procedureel geschreven simulatie. Hell, bij een functionele taal zou je je functies eigenlijk niet groter dan een regel of 5 moeten hebben denk ik.

[PiepPiep]

Misschien ietsje offtopic, maar wat ik ook altijd grappig vind om te zien zijn sommige code optimalizaties van automatische tooltjes.

code:

if (voorwaarde)
    variabele = waarde1;
else
    variabele = waarde2;

// wordt dan
variabele = voorwaarde ? waarde1 : waarde2;

Dit is prima, maar als de voorwaarde al uit meerdere vergelijkingen bestaat en dus op het scherm bijna de volle breedte is en hetzelfde met waarde1 en waarde2 dan krijg je nu dus een regel van zo'n 3 schermen breed.
Op zo'n moment denk ik toch doe maar niet.

[Patriot]

Janoz schreef op maandag 03 oktober 2011 @ 11:31:
[...]

Ik reageer op "Een methode van minder dan 20 regels is namelijk ook niet per definitie goed."en daarop is die analogie exact van toepassing. Je houden aan 1 regel betekend nog niet dat je niet een andere regel overtreden hebt.

Ok, granted, als je het zo bedoelde dan klopt er meer van de analogie. Maar zeg het de volgende keer maar meteen zo

[...]

Teveel uitzonderingen? Hoeveel uitzonderingen zijn er dan? Voor zover ik weet maar 1 "Heb een duidelijk verhaal waarom het echt niet korter kan"

Which is to say: Praktisch oneindig veel uitzonderingen. De regel geeft zoveel ruimte voor toetsing a.d.h.v andere regels dat het in feite alleen maar luiheid aanmoedigt: Als je onder de grens zit dan hoef je niet te kijken of het beter kan, het voldoet al aan de lengte-eis.

EDIT:
Ik snap overigens gerust dat je soms uitzonderingen op dit soort regels moet maken, in dit geval komt het echter op mij over alsof de regel niets toevoegt aan andere regels die je kunt hanteren om je code zo optimaal mogelijk te schrijven.

[ Voor 12% gewijzigd door Patriot op 03-10-2011 13:58 ]

[Spockz]

PiepPiep schreef op maandag 03 oktober 2011 @ 13:08:
Misschien ietsje offtopic, maar wat ik ook altijd grappig vind om te zien zijn sommige code optimalizaties van automatische tooltjes.

code:

if (voorwaarde)
    variabele = waarde1;
else
    variabele = waarde2;

// wordt dan
variabele = voorwaarde ? waarde1 : waarde2;

Dit is prima, maar als de voorwaarde al uit meerdere vergelijkingen bestaat en dus op het scherm bijna de volle breedte is en hetzelfde met waarde1 en waarde2 dan krijg je nu dus een regel van zo'n 3 schermen breed.
Op zo'n moment denk ik toch doe maar niet.

Hoe is dat optimalisatie? Het is precies hetzelfde?

[Verwijderd]

Ik vroeg me precies hetzelfde af. Een moderne compiler compileert dat toch naar precies dezelfde executable?

[Avalaxy]

Bij mijn weten is een ternary operator inderdaad alleen maar syntactic sugar.

Vind ze persoonlijk wel fijn

[ Voor 18% gewijzigd door Avalaxy op 03-10-2011 14:29 ]

[NMe]

Ik vind ze ook fijn, maar alleen als het statement daarbij niet buiten beeld verdwijnt. De machinecode die eruit komt is in elk geval gelijk bij mijn weten.

[Janoz]

Ternary operator gebruik ik eigenlijk alleen maar in template achtige oplossingen en bij simpele returns (om er een null check bij te doen oid)

Patriot schreef op maandag 03 oktober 2011 @ 13:57:
Which is to say: Praktisch oneindig veel uitzonderingen. De regel geeft zoveel ruimte voor toetsing a.d.h.v andere regels dat het in feite alleen maar luiheid aanmoedigt: Als je onder de grens zit dan hoef je niet te kijken of het beter kan, het voldoet al aan de lengte-eis.

Als je onder de grens zit kun je inderdaad onderuitgezakt zitten wachten. Totdat de cyclomatic complexity te hoog blijkt of dat er een cyclische referentie gevonden wordt.

Ik snap overigens gerust dat je soms uitzonderingen op dit soort regels moet maken, in dit geval komt het echter op mij over alsof de regel niets toevoegt aan andere regels die je kunt hanteren om je code zo optimaal mogelijk te schrijven.

Het voordeel van deze regel (en bijvoorbeeld ook de complexity) is dat ze simpel automatisch te bepalen zijn en toch een erg goed beeld geven over de staat van een stuk sourcecode. Erg handig om:
1 - zonder uitgebreide code review alvast een inschatting te kunnen maken over de kwaliteit van een stuk sourcecode.
2 - op te nemen in je continuous integration om kwaliteit blijvend in de gaten te kunnen houden.

[ Voor 81% gewijzigd door Janoz op 03-10-2011 14:48 ]

[Olaf van der Spek]

PiepPiep schreef op maandag 03 oktober 2011 @ 13:08:
Dit is prima, maar als de voorwaarde al uit meerdere vergelijkingen bestaat en dus op het scherm bijna de volle breedte is en hetzelfde met waarde1 en waarde2 dan krijg je nu dus een regel van zo'n 3 schermen breed.
Op zo'n moment denk ik toch doe maar niet.

code:

variabele = voorwaarde 
  ? waarde1 
  : waarde2;

[PiepPiep]

Het is idd syntactic sugar en met kleine stukjes vind ik het zelf wel duidelijker, maar dus niet als het daardoor buiten het scherm komt.
De oplossing die Olaf geeft gebruik ik dan meestal ook wel.