[alg] Slechtste programmeervoorbeelden deel 5

[RagingPenguin]

Xantios schreef op woensdag 18 september 2019 @ 11:28:

code:

                        return 'succes';
                    }
                    else {
                        return 'error';
                    }
                }
                else {
                    return 'error';
                }
            }
            else {
                return 'error';
            }
        }
        else {
            return 'error';
        }
    }

Booleans zijn blijkbaar te moeilijk, en wat de F hoeveel genested if's heeft een mens nodig >.> *grom*

Je kan toch ook gewoon een hoop van else statements weg laten? Mis ik iets of zou het met alleen de laatste 'return error' exact hetzelfde werken?

[Xantios]

RagingPenguin schreef op woensdag 18 september 2019 @ 12:44:
[...]


Je kan toch ook gewoon een hoop van else statements weg laten? Mis ik iets of zou het met alleen de laatste 'return error' exact hetzelfde werken?

Dit kreng zit vol erfenis van mijn voorvanger, een typisch geval van een handig neefje die iets in elkaar gestoken heeft wat "gister af" moest.

Ben het beste dier momenteeel stukje bij beetje opnieuw aan het bouwen, maar soms kom je van dit soort pareltjes tegen tijdens het uitzoeken van hoe het in hemelsnaam werkt

[Giesber]

Xantios schreef op woensdag 18 september 2019 @ 11:28:

code:

                        return 'succes';
                    }
                    else {
                        return 'error';
                    }
                }
                else {
                    return 'error';
                }
            }
            else {
                return 'error';
            }
        }
        else {
            return 'error';
        }
    }

Booleans zijn blijkbaar te moeilijk, en wat de F hoeveel genested if's heeft een mens nodig >.> *grom*

Afhankelijk van wat ervoor staat is die code misschien nog redelijk overzichtelijk, en waarschijnlijk is die gewoon performant. Ik zou het zelf vermoedelijk anders doen (ik gok dat het wel overzichtelijker kan), maar ik zou dit nu niet bij de slechtste programmeervoorbeelden scharen eigenlijk.

Anders moet ik hier in sommige gevallen de halve codebase van een project gaan zetten .

[CurlyMo]

@RagingPenguin In dit geval wel, maar als het bijv. Python zou zijn dan niet.

[Xantios]

Giesber schreef op woensdag 18 september 2019 @ 13:08:
[...]

Afhankelijk van wat ervoor staat is die code misschien nog redelijk overzichtelijk, en waarschijnlijk is die gewoon performant.

Wat er voor staat is vooral een vergaar bak van inline SQL query's die absoluut niet overzichtelijk is (helaas)
Performant is natuurlijk relatief, maar gezien het allemaal in PHP 5.4 draait is het sowieso wel tijd voor een flinke bezem.

Wat ik vooral erg vreemd vind is dat ie hier 'success' of 'error' terug gooit, terwijl true en false prima zou voldoen. zeker omdat niet veel verder gecontroleerd wordt wat er uit kwam doormiddel van:

code:

if($x == 'succes') {

[ Voor 0% gewijzigd door Xantios op 18-09-2019 13:27 . Reden: Ik kan niet typen ]

[.oisyn]

Dit is sowieso al korter

code:

                        return 'succes';
                    }
                }
            }
        }
        return 'error';
    }

[downtime]

Xantios schreef op woensdag 18 september 2019 @ 13:13:
[...]


Wat er voor staat is vooral een vergaar bak van inline SQL query's die absoluut niet overzichtelijk is (helaas)
Performant is natuurlijk relatief, maar gezien het allemaal in PHP 5.4 draait is het sowieso wel tijd voor een flinke bezem.

Wat ik vooral erg vreemd vind is dat ie hier 'success' of 'error' terug gooit, terwijl true en false prima zou voldoen. zeker omdat niet veel verder gecontroleerd wordt wat er uit kwam doormiddel van:

code:

if($x == 'success') {

Er is ‘succes’ en ‘success’?

[Xantios]

Pardon, tikfoutje! Er is wel enkel 'succes'
al had het mij in deze codebase werkelijk niks verbaast als dat verschil wel zou bestaan.

[Giesber]

Xantios schreef op woensdag 18 september 2019 @ 13:28:
Pardon, tikfoutje! Er is wel enkel 'succes'
al had het mij in deze codebase werkelijk niks verbaast als dat verschil wel zou bestaan.

Ah, ik dacht dat dat een soort placeholder was om het voorbeeld te versimpelen of geen specifieke informatie te delen (in de naam van een object of zo).

[incaz]

Het was een tijdje geleden dat ik het tegengekomen was... maar een overijverige regexp die alle voorkomens van 'de' of 'com' vlagt (Duitse site ) Ook ls het deel van een woord is.. (Ik wilde de depth of the seat and the height of the sides weten. Huh.)

[kenneth]

quote: https://github.com/Micros...es/EmptyEnumerable.cs#L20

occ----ionally

En in de Apple Store werd (wordt?) het volkomen normale werkwoord "kunt" gecensureerd, evenals woorden als "gemeenschap".

Find & replace, altijd een goed idee.

[0xDEADBEEF]

Maar partition mag wel?

[ Voor 17% gewijzigd door 0xDEADBEEF op 30-09-2019 13:07 ]

[ThomasG]

kenneth schreef op maandag 30 september 2019 @ 12:47:
[...]


En in de Apple Store werd (wordt?) het volkomen normale werkwoord "kunt" gecensureerd, evenals woorden als "gemeenschap".

Find & replace, altijd een goed idee.

Ditzelfde gebeurt bij vrijwel alle multiplayer games met een profanity filter. Die zijn dan zo ingesteld dat veel Nederlandse woorden geheel ge-#$%#% worden, omdat een deel van het woord - als je wat lettertjes vervangt - op een Engels scheldwoord lijkt. En scheldwoorden uit andere talen werken dan weer wel. De Amerikaanse hyper-gevoeligheid ook

[Tk55]

Deze kon ik jullie echt niet onthouden:

Python:

import sys
import json


def download():
    global PROJECT, DATE, BUCKET, LOCAL_FOLDER
    ...


def main(kwargs_str):
    global PROJECT, DATE, BUCKET, LOCAL_FOLDER, USE_GPU

    job_parameters = json.loads(kwargs_str)

    PROJECT = job_parameters["project"]
    DATE = job_parameters["run_as_of_date"]
    BUCKET = job_parameters["bucket"]
    LOCAL_FOLDER = job_parameters["local_folder"]
    USE_GPU = job_parameters["use_gpu"]
    
    download()
    ...


if __name__ == "__main__":
    _, kwargs_string = sys.argv
    main(kwargs_string)

In werkelijkheid is het nog wat erger, met nog meer globals en functies maar dat laat ik even buiten beschouwing.

Het script wordt gedraaid door een json string mee te geven aan het programma

[PageFault]

Global en dan vooral twee keer definieren...

[RayNbow]

PageFault schreef op woensdag 2 oktober 2019 @ 10:07:
Global en dan vooral twee keer definieren...

Waar zie jij dubbele definities?

[PageFault]

RayNbow schreef op woensdag 2 oktober 2019 @ 10:16:
[...]

Waar zie jij dubbele definities?

Als je net zoveel drinkt als ik, zie je die in Download en in Main

[RayNbow]

PageFault schreef op woensdag 2 oktober 2019 @ 10:20:
[...]


Als je net zoveel drinkt als ik, zie je die in Download en in Main

Dat zijn geen definities. Daarmee declareer je dat je in de betreffende functiescopes gebruik wil maken van bepaalde globale variabelen.

[Tk55]

Sterker nog, er stond bovenaan, onder de imports:

Python:

# Constants. Please don't re-assign after initialization.
global PROJECT, DATE, BUCKET, LOCAL_FOLDER

[roemerbakker]

Het volgende moest plaatjes resizen naar 3 algemene sizes, maar is zoals je ziet complete onleesbare chaos. Ook vond ik comments blijkbaar niet nodig

PHP:

for($i = 1; $i <= self::IMAGE_COUNT; $i++)
{
    $key = 'image_'.$i;

    if ($request->hasFile($key))
    {
        $image = Image::make($request->file($key)->getRealPath());
        $large = false;
        $medium = false;

        $mime = $image->mime();  //edited due to updated to 2.x
        if ($mime == 'image/jpeg')
            $extension = '.jpg';
        elseif ($mime == 'image/png')
            $extension = '.png';
        elseif ($mime == 'image/gif')
            $extension = '.gif';
        else
            $extension = '';
        $filename = $listing->id . '_' . substr($key, 6) . substr(hash('sha512', $image), 118).$extension;

        $width = $image->width();

        if ($width >= self::IMAGE_LARGE_RES)
        {
            $large = true;
            $largeImage = $image->resize(self::IMAGE_LARGE_RES, self::IMAGE_LARGE_RES, function ($constraint) {
                $constraint->aspectRatio();
            });
            Storage::put('public/listing_images/'.$listing->id.'/large/'.$filename, $largeImage->stream()->__toString());
        }
        if ($width >= self::IMAGE_MED_RES)
        {
            $medium = true;
            $mediumImage = $image->resize(self::IMAGE_MED_RES, self::IMAGE_MED_RES, function ($constraint) {
                $constraint->aspectRatio();
            });
            Storage::put('public/listing_images/'.$listing->id.'/medium/'.$filename, $mediumImage->stream()->__toString());
        }

        $smallImage = $image->resize(self::IMAGE_SMALL_RES, self::IMAGE_SMALL_RES, function ($constraint) {
            $constraint->aspectRatio();
        });
        Storage::put('public/listing_images/'.$listing->id.'/small/'.$filename, $smallImage->stream()->__toString());

        $img = new ImageModel();
        $img->listing_id = $listing->id;
        $img->filename = $filename;
        $img->small = true;
        $img->medium = $medium;
        $img->large = $large;
        $img->save();
    }
    else
    {
        continue;
    }
}

[Mugwump]

roemerbakker schreef op woensdag 2 oktober 2019 @ 11:14:
Het volgende moest plaatjes resizen naar 3 algemene sizes, maar is zoals je ziet complete onleesbare chaos. Ook vond ik comments blijkbaar niet nodig

PHP:

for($i = 1; $i <= self::IMAGE_COUNT; $i++)
{
    $key = 'image_'.$i;

    if ($request->hasFile($key))
    {
        $image = Image::make($request->file($key)->getRealPath());
        $large = false;
        $medium = false;

        $mime = $image->mime();  //edited due to updated to 2.x
        if ($mime == 'image/jpeg')
            $extension = '.jpg';
        elseif ($mime == 'image/png')
            $extension = '.png';
        elseif ($mime == 'image/gif')
            $extension = '.gif';
        else
            $extension = '';
        $filename = $listing->id . '_' . substr($key, 6) . substr(hash('sha512', $image), 118).$extension;

        $width = $image->width();

        if ($width >= self::IMAGE_LARGE_RES)
        {
            $large = true;
            $largeImage = $image->resize(self::IMAGE_LARGE_RES, self::IMAGE_LARGE_RES, function ($constraint) {
                $constraint->aspectRatio();
            });
            Storage::put('public/listing_images/'.$listing->id.'/large/'.$filename, $largeImage->stream()->__toString());
        }
        if ($width >= self::IMAGE_MED_RES)
        {
            $medium = true;
            $mediumImage = $image->resize(self::IMAGE_MED_RES, self::IMAGE_MED_RES, function ($constraint) {
                $constraint->aspectRatio();
            });
            Storage::put('public/listing_images/'.$listing->id.'/medium/'.$filename, $mediumImage->stream()->__toString());
        }

        $smallImage = $image->resize(self::IMAGE_SMALL_RES, self::IMAGE_SMALL_RES, function ($constraint) {
            $constraint->aspectRatio();
        });
        Storage::put('public/listing_images/'.$listing->id.'/small/'.$filename, $smallImage->stream()->__toString());

        $img = new ImageModel();
        $img->listing_id = $listing->id;
        $img->filename = $filename;
        $img->small = true;
        $img->medium = $medium;
        $img->large = $large;
        $img->save();
    }
    else
    {
        continue;
    }
}

Ik vind het op zich nog best leesbaar. Beetje SRP en reuse overheen en je hebt een prima onderhoudbaar stukje code.

[CurlyMo]

roemerbakker schreef op woensdag 2 oktober 2019 @ 11:14:
Het volgende moest plaatjes resizen naar 3 algemene sizes, maar is zoals je ziet complete onleesbare chaos. Ook vond ik comments blijkbaar niet nodig

Ik zie ook het probleem niet. De code is self documenting genoeg. D.w.z. dat ik precies kan zien wat er in welke stap gebeurt en waarom, en zou het ook kunnen bugfixen. En dan ben ik niet eens primair PHP ontwikkelaar. Kan het beter, vast, is het slecht, nee.

[roemerbakker]

@CurlyMo @Mugwump Thanks I guess haha, ik ben sinds het schrijven van deze code redelijk neurotisch geworden als het gaat om code-style, comments en vooral DRY programmeren, dus misschien krijg ik er zelf gewoon meer kriebels van dan gemiddeld

[Olaf van der Spek]

roemerbakker schreef op woensdag 2 oktober 2019 @ 11:42:
@CurlyMo @Mugwump Thanks I guess haha, ik ben sinds het schrijven van deze code redelijk neurotisch geworden als het gaat om code-style, comments en vooral DRY programmeren, dus misschien krijg ik er zelf gewoon meer kriebels van dan gemiddeld

Kun je het herschrijven en het resultaat posten?

[Mugwump]

roemerbakker schreef op woensdag 2 oktober 2019 @ 11:42:
@CurlyMo @Mugwump Thanks I guess haha, ik ben sinds het schrijven van deze code redelijk neurotisch geworden als het gaat om code-style, comments en vooral DRY programmeren, dus misschien krijg ik er zelf gewoon meer kriebels van dan gemiddeld

Oh ik zou dit soort code zonder meer even refactoren hoor. Even de duplicatie opschonen, wat functies als getExtension implementeren en meer van dat soort kleinigheden. Gevalletje broken windows.

[roemerbakker]

Olaf van der Spek schreef op woensdag 2 oktober 2019 @ 12:13:
[...]

Kun je het herschrijven en het resultaat posten?

Ietsje beter in de huidige scope. Ik zou dit tegenwoordig eigenlijk in verschillende delen van de applicatie opsplitsen, in dit geval staat de hele boel namelijk in een (Laravel) controler methode, waarin ik nu nooit meer direct met modellen interacteer.

PHP:

...begin van de methode
        for($i = 1; $i <= self::IMAGE_COUNT; $i++)
        {
            $requestKey = 'image_'.$i;
            // The request always contains entries for all images, but these don't always actually contain files
            if (!$request->hasFile($requestKey))
                continue;

            // Compose some basic information about the image
            $image = Image::make($request->file($requestKey)->getRealPath());
            $extension = self::resolveFileExtension($image);
            if ($extension === null)
            {
                continue;
            }
            $filename = sprintf('%d_image_%d_%s%s', $listing->id, $i, md5($image), $extension);
            
            $sizes = [
                self::IMAGE_SMALL_RES,
                self::IMAGE_MED_RES,
                self::IMAGE_SMALL_RES
            ];
            foreach ($sizes as $size)
            {
                // Since we're going from small to large, there is no point continuing when we've reached the
                // constrains of the original image resolution
                if ($image->width() < $size)
                    break;
                
                // We'll store the resized image instances in the original size array to easily be able to check if they
                // were successful later
                $sizes[$size] = $image->resize($size, $size, function ($constraint) {
                    $constraint->aspectRatio();
                });
                self::storeImage($listing->id, $size, $filename, $sizes[$size]);
            }

            // Store details about the image in the database
            $img = new ImageModel();
            $img->listing_id    = $listing->id;
            $img->filename      = $filename;
            $img->small         = $sizes[self::IMAGE_SMALL_RES] ?? false;
            $img->medium        = $sizes[self::IMAGE_MED_RES] ?? false;
            $img->large         = $sizes[self::IMAGE_LARGE_RES] ?? false;
            $img->save();
        }
.....eind van de methode

/**
 * Resolves the file extension this image should have based on its mime data.
 *
 * @param \Intervention\Image\Image $img The image for which the file extension should be resolved
 * @return string|null
 */
private static function resolveFileExtension(\Intervention\Image\Image $img)
{
    switch ($img->mime())
    {
        case 'image/jpeg':
        case 'image/jpg':
            return '.jpg';
        case 'image/png':
            return '.png';
        case 'image/gif':
            return '.gif';
        default:
            Log::info(sprintf('Saw image with invalid mime type: %s.', $img->mime()));
            return null;
    }
}

/**
 * Store a resized image
 *
 * @param $listingId                      The ID of the listing this image belongs to
 * @param $size                           The size this image was resized to
 * @param $fileName                       The name of the file
 * @param \Intervention\Image\Image $img  The image instance
 */
private static function storeImage($listingId, $size, $fileName, \Intervention\Image\Image $img)
{
    Storage::put(sprintf('public/listing_images/%d/%s/%s', $listingId, $size, $fileName), (string) $img->stream());
}

[incaz]

@roemerbakker, het enige wat volgens mij niet helemaal goed gaat is dat de file dirs niet meer op naam van de size gaan in storeImage (/small/, /medium/, /large/) maar op de numerieke waarde zoals gedef in $sizes.

[Mugwump]

Als ik zoiets zie, dan vraag ik me wel af hoe je fouten afhandelt. Met name bij het opslaan van zaken is het niet ondenkbaar dat er iets misgaat vanwege een IO issue. Wellicht wil je dan ook nog weer wat opschonen bijvoorbeeld.
Daarnaast ben ik zelf nooit zo'n fan van constructies waarbij je opgeslagen locaties totaal niet in je model opneemt, maar zo te zien op verschillende plekken in je codebase weer opbouwt. Mocht je bij het wegschrijven daar ooit iets wijzigen, dan gaat het mis bij het uitlezen. Stop de logica voor de locatiebepaling dan op z'n minst in het model.

[robbens]

Onderstaande kom ik ook nog aardig vaak tegen, al dan niet in de comments zelf (of tijdens een code-review).

Java:

 
@Test
public void defaultCollectionsHashMapSort() {
    // It works, but I can't figure out why
    if(...) {
        assertEquals(expectedValue, someValue);
    }
}

[roemerbakker]

incaz schreef op woensdag 2 oktober 2019 @ 16:46:
@roemerbakker, het enige wat volgens mij niet helemaal goed gaat is dat de file dirs niet meer op naam van de size gaan in storeImage (/small/, /medium/, /large/) maar op de numerieke waarde zoals gedef in $sizes.

Ah ja! Foutje. Ik heb dit ook ff blind gerefactored. Dit draait al jaaaren niet meer lokaal op m'n laptopje

[roemerbakker]

Mugwump schreef op woensdag 2 oktober 2019 @ 16:48:
Als ik zoiets zie, dan vraag ik me wel af hoe je fouten afhandelt. Met name bij het opslaan van zaken is het niet ondenkbaar dat er iets misgaat vanwege een IO issue. Wellicht wil je dan ook nog weer wat opschonen bijvoorbeeld.
Daarnaast ben ik zelf nooit zo'n fan van constructies waarbij je opgeslagen locaties totaal niet in je model opneemt, maar zo te zien op verschillende plekken in je codebase weer opbouwt. Mocht je bij het wegschrijven daar ooit iets wijzigen, dan gaat het mis bij het uitlezen. Stop de logica voor de locatiebepaling dan op z'n minst in het model.

Eens. Ik zou het tegenwoordig ook zeker niet meer op deze manier doen en inderdaad de volledige paden in het model opnemen. Ik had deze code ff snel (ongetest want het draait niet meer lokaal) gerefactored en wilde een beetje dezelfde scope gebruiken (geen model migraties etc) om het voor het forum een beetje overzichtelijk te houden.

[Tk55]

Sterker nog, als je het gewoon in functies zet, kun je de paden gescheiden houden van de implementatie van het resizen. Dan krijg je iets als de volgende pseudo code:

code:

img = loadImage(path)
resized_img = resizeImage(img)
saveImage(resized_img)

Hooguit een for loop eromheen voor het laden van bestanden uit een directory en je hebt al behoorlijk nette code. Al ziet het er al behoorlijk leesbaar uit.

[ Voor 3% gewijzigd door Tk55 op 03-10-2019 09:29 ]

[BarôZZa]

roemerbakker schreef op woensdag 2 oktober 2019 @ 15:31:
[...]


Ietsje beter in de huidige scope. Ik zou dit tegenwoordig eigenlijk in verschillende delen van de applicatie opsplitsen, in dit geval staat de hele boel namelijk in een (Laravel) controler methode, waarin ik nu nooit meer direct met modellen interacteer.

PHP:

...begin van de methode
        for($i = 1; $i <= self::IMAGE_COUNT; $i++)
        {
            $requestKey = 'image_'.$i;
            // The request always contains entries for all images, but these don't always actually contain files
            if (!$request->hasFile($requestKey))
                continue;

            // Compose some basic information about the image
            $image = Image::make($request->file($requestKey)->getRealPath());
            $extension = self::resolveFileExtension($image);
            if ($extension === null)
            {
                continue;
            }
            $filename = sprintf('%d_image_%d_%s%s', $listing->id, $i, md5($image), $extension);
            
            $sizes = [
                self::IMAGE_SMALL_RES,
                self::IMAGE_MED_RES,
                self::IMAGE_SMALL_RES
            ];
            foreach ($sizes as $size)
            {
                // Since we're going from small to large, there is no point continuing when we've reached the
                // constrains of the original image resolution
                if ($image->width() < $size)
                    break;
                
                // We'll store the resized image instances in the original size array to easily be able to check if they
                // were successful later
                $sizes[$size] = $image->resize($size, $size, function ($constraint) {
                    $constraint->aspectRatio();
                });
                self::storeImage($listing->id, $size, $filename, $sizes[$size]);
            }

            // Store details about the image in the database
            $img = new ImageModel();
            $img->listing_id    = $listing->id;
            $img->filename      = $filename;
            $img->small         = $sizes[self::IMAGE_SMALL_RES] ?? false;
            $img->medium        = $sizes[self::IMAGE_MED_RES] ?? false;
            $img->large         = $sizes[self::IMAGE_LARGE_RES] ?? false;
            $img->save();
        }
.....eind van de methode

/**
 * Resolves the file extension this image should have based on its mime data.
 *
 * @param \Intervention\Image\Image $img The image for which the file extension should be resolved
 * @return string|null
 */
private static function resolveFileExtension(\Intervention\Image\Image $img)
{
    switch ($img->mime())
    {
        case 'image/jpeg':
        case 'image/jpg':
            return '.jpg';
        case 'image/png':
            return '.png';
        case 'image/gif':
            return '.gif';
        default:
            Log::info(sprintf('Saw image with invalid mime type: %s.', $img->mime()));
            return null;
    }
}

/**
 * Store a resized image
 *
 * @param $listingId                      The ID of the listing this image belongs to
 * @param $size                           The size this image was resized to
 * @param $fileName                       The name of the file
 * @param \Intervention\Image\Image $img  The image instance
 */
private static function storeImage($listingId, $size, $fileName, \Intervention\Image\Image $img)
{
    Storage::put(sprintf('public/listing_images/%d/%s/%s', $listingId, $size, $fileName), (string) $img->stream());
}

Ik vind je oude code beter. Bij de nieuwe misbruik je de $sizes array. Je stopt er zowel afmetingen als afbeeldingen in, een van de snelste wegen naar vieze bugs, want de informatie is niet eenduidig. Wat gebeurt er bij jou als self::IMAGE_SMALL_RES een waarde heeft van 2? Ik wil images van maximaal 2 pixels breed/hoog, maar nu overschrijft hij de self::IMAGE_SMALL_RES waarde in die array met een afbeelding(de $sizes[$size] = $image->resize), waardoor waarschijnlijk de for loop crasht.

Overigens krijg ik zelf altijd juist enorme jeuk van overbodige comments:

// Since we're going from small to large, there is no point continuing when we've reached the
// constrains of the original image resolution
if ($image->width() < $size)
break;

Die if-statement vind ik tig keer duidelijker dan de zin erboven.

De comment erna hetzelfde. Vaak worden comments gebruikt om onlogische dingen uit te leggen:

// We'll store the resized image instances in the original size array to easily be able to check if they
// were successful later

[Matis]

Één van de weinige keren dat ik commentaar gebruik in een stuk code, is wanneer er een (onlogische) business keuze gemaakt is. Dan plemp ik daar de URL naar het ticket in, zodat een toekomstig ontwikkelaar terug kan lezen waarom er zulke gekke keuzes gemaakt worden.

[Blokker_1999]

Matis schreef op vrijdag 4 oktober 2019 @ 06:49:
Één van de weinige keren dat ik commentaar gebruik in een stuk code, is wanneer er een (onlogische) business keuze gemaakt is. Dan plemp ik daar de URL naar het ticket in, zodat een toekomstig ontwikkelaar terug kan lezen waarom er zulke gekke keuzes gemaakt worden.

Is dat niet gevaarlijk voor wanneer men eens van ticketing systeem veranderd?

[Matis]

Blokker_1999 schreef op vrijdag 4 oktober 2019 @ 08:01:
Is dat niet gevaarlijk voor wanneer men eens van ticketing systeem veranderd?

Daar heb je een punt. Al verwacht ik (ik weet het, aannames) dat door in ieder geval een URL te plaatsten er bij de volgende ontwikkelaar hopelijk een belletje gaat rinkelen en hijzelf het onderzoek start naar de toenmalige besluitvorming.

[Verwijderd]

code:

// See ticket <id> (<url>)

Ergens ben ik benieuwd hoe vaak goed functionerende code gelezen of aangepast wordt. En daarmee het nut van overdocumenteren.

Die ‘it works and I wonder why’ is een mooi voorbeeld van Voodoo-programmeren.

Ik heb zelf wel eens zulke code geschreven (en de exception gezien )

code:

throw new Exception(“This code should never, ever be executed. If it does my logic is flawed!”);

[Mugwump]

Matis schreef op vrijdag 4 oktober 2019 @ 06:49:
Één van de weinige keren dat ik commentaar gebruik in een stuk code, is wanneer er een (onlogische) business keuze gemaakt is. Dan plemp ik daar de URL naar het ticket in, zodat een toekomstig ontwikkelaar terug kan lezen waarom er zulke gekke keuzes gemaakt worden.

Dat soort dingen zet ik eerder in commit messages. Vervuilt je code niet en vaak wil je in dit soort gevallen toch even de historie zien.

[boe2]

Dit soort comments vervuilen je code niet imo. Niet iedereen gaat een blame uitvoeren op code die weinig steek lijkt te houden. Wat je dan krijgt is dat iemand de boel wil verbeteren en de oude bug opnieuw introduceert.

[GrooV]

Blokker_1999 schreef op vrijdag 4 oktober 2019 @ 08:01:
[...]

Is dat niet gevaarlijk voor wanneer men eens van ticketing systeem veranderd?

Hoe vaak verander je van ticket systeem zonder migratie of de oude in leven te houden?

[CurlyMo]

Enige waar je je wat mij betreft rekening mee moet houden is dat je teamleden sommige theorieën en concepten niet altijd kennen of paraat hebben. In mijn SQL gebruik ik regelmatig gaps & islands. Ik weet echter dat mijn collega's dat niet altijd direct zien of zonder uitleg begrijpen hoe dit dan in mijn grotere plaatje past. Dat leg ik dan uit in mijn script. Ik ga niet gaps & islands opnieuw uitleggen, maar wel dat een stukje SQL de toepassing van gaps & islands is.

[Olaf van der Spek]

GrooV schreef op vrijdag 4 oktober 2019 @ 09:04:
[...]

Hoe vaak verander je van ticket systeem zonder migratie of de oude in leven te houden?

Oude URLs werken vaak niet na een migratie..

[.oisyn]

boe2 schreef op vrijdag 4 oktober 2019 @ 08:59:
Dit soort comments vervuilen je code niet imo. Niet iedereen gaat een blame uitvoeren op code die weinig steek lijkt te houden.

Een timelapse view can de code spreekt anders vaak boekdelen, en dat is mijn eerste aanknopingspunt als ik een stuk code niet begrijp. Het vertelt hoe de code is geevolueerd, wat de commit message was van de betreffende change en wie hem heeft gemaakt. Ticketnummers wil ik daarentegen echt niet in de code zien; een goede comment omschrijft waarom de code is zoals hij is, een referentie naar een ander systeem om uitleg is alleen maar irritant.

[Blokker_1999]

GrooV schreef op vrijdag 4 oktober 2019 @ 09:04:
[...]

Hoe vaak verander je van ticket systeem zonder migratie of de oude in leven te houden?

Bij ons? Wij gaan nu naar het tweede systeem op 2 jaar tijd en daar het vorige niet gemigreerd werd betwijfel ik dat ze het deze keer wel gaan doen. En het vorige is uiteindelijk offline gehaald toen een blunder van een teamlead erin naar boven aan het komen was.

[DaCoTa]

.oisyn schreef op vrijdag 4 oktober 2019 @ 10:25:
[...]

Een timelapse view can de code spreekt anders vaak boekdelen, en dat is mijn eerste aanknopingspunt als ik een stuk code niet begrijp. Het vertelt hoe de code is geevolueerd, wat de commit message was van de betreffende change en wie hem heeft gemaakt. Ticketnummers wil ik daarentegen echt niet in de code zien; een goede comment omschrijft waarom de code is zoals hij is, een referentie naar een ander systeem om uitleg is alleen maar irritant.

Wij hebben aangeleerd om commit messages altijd met een ticketnummer en -naam te beginnen. Dan kan je in de revision annotations direct zien welke regels door welk ticket zijn aangeraakt. En van daaruit is eventueel verder te zoeken naar de achterliggende business case.

Timelapse view kende ik overigens nog niet, interessant systeem.

[downtime]

GrooV schreef op vrijdag 4 oktober 2019 @ 09:04:
[...]

Hoe vaak verander je van ticket systeem zonder migratie of de oude in leven te houden?

Vaak. Ik heb in 20 jaar IT al zoveel ticketsystemen zien komen en gaan. Migreren gebeurt nooit omdat migratie van data naar een ander pakket bijna altijd complex en duur uitpakt. En het oude systeem in leven houden betekent dat je het ook moet onderhouden. En dat kost geld. Zoveel geld is die oude data de organisatie vaak niet waard.

[ThomasG]

DaCoTa schreef op vrijdag 4 oktober 2019 @ 13:18:
[...]

Wij hebben aangeleerd om commit messages altijd met een ticketnummer en -naam te beginnen. Dan kan je in de revision annotations direct zien welke regels door welk ticket zijn aangeraakt. En van daaruit is eventueel verder te zoeken naar de achterliggende business case.

Timelapse view kende ik overigens nog niet, interessant systeem.

Het ticket nummer in de titel van de commit is alleen maar irritant, en verteld je helemaal niets aan je dat ziet en het ticket nummer niet uit je hoofd kent. Ik wil gewoon snel zien wat de commit oplost, dus een samenvatting van de commit in één zin. Vervolgens een uitgebreidere uitleg (indien nodig). Verwijzingen naar een ander systeem staan bij ons helemaal onder aan de commit message met iets als: Closes T123, T456 e.d. waardoor het automatisch opgepakt wordt. Als ik wil zien welke code is aangepast voor een bepaalde ticket, kijk ik daarvoor in het build systeem; niet in de git log.

[gekkie]

ThomasG schreef op vrijdag 4 oktober 2019 @ 20:15:
[...]
Het ticket nummer in de titel van de commit is alleen maar irritant, en verteld je helemaal niets aan je dat ziet en het ticket nummer niet uit je hoofd kent. Ik wil gewoon snel zien wat de commit oplost, dus een samenvatting van de commit in één zin. Vervolgens een uitgebreidere uitleg (indien nodig).

Ohhhww dat is zo gigantisch irritant. Merged pull requests met alleen een pull nummertje op github zijn dat ook.

[Bee.nl]

ThomasG schreef op vrijdag 4 oktober 2019 @ 20:15:
[...]
Het ticket nummer in de titel van de commit is alleen maar irritant, en verteld je helemaal niets aan je dat ziet en het ticket nummer niet uit je hoofd kent. Ik wil gewoon snel zien wat de commit oplost, dus een samenvatting van de commit in één zin. Vervolgens een uitgebreidere uitleg (indien nodig). Verwijzingen naar een ander systeem staan bij ons helemaal onder aan de commit message met iets als: Closes T123, T456 e.d. waardoor het automatisch opgepakt wordt. Als ik wil zien welke code is aangepast voor een bepaalde ticket, kijk ik daarvoor in het build systeem; niet in de git log.

Versiebeheer is imho juist bij uitstek de locatie om snel informatie te kunnen krijgen over het wie/wat/waarom/wanneer. Bij ons prefixen we de commit messages dus met ticket nummer gevolgd door een kleine uitleg. De relatie tussen een code change en het ticket in je build straat vind ik omslachtig. Aangezien het ticket nummer een vast format heeft vind ik het efficiënter om dat meteen aan het begin van de message terug te vinden. Bij logging zet je immers toch ook niet de datum achter de logmessage, maar ervoor.

[DaCoTa]

gekkie schreef op vrijdag 4 oktober 2019 @ 20:25:
[...]
Ohhhww dat is zo gigantisch irritant. Merged pull requests met alleen een pull nummertje op github zijn dat ook.

Ik zeg ook niet alleen een ticketnummer, dat is idd onhandig. Maar ticketnummer met titel van ticket of andere aanduiding, en daaronder nog evt. verdere uitleg. Maar die uitleg is specifiek over die commit, dat zou niet de uitleg van de commit moeten zijn, dat staat in het ticketsysteem.

[mcDavid]

Matis schreef op vrijdag 4 oktober 2019 @ 06:49:
Één van de weinige keren dat ik commentaar gebruik in een stuk code, is wanneer er een (onlogische) business keuze gemaakt is. Dan plemp ik daar de URL naar het ticket in, zodat een toekomstig ontwikkelaar terug kan lezen waarom er zulke gekke keuzes gemaakt worden.

Dat soort logica schrijf ik liever in een eigen method. Dat geeft je de gelegenheid om de functionaliteit te beschrijven in de method name, zodat in de flow van je code direct duidelijk is wat er gebeurt zonder überhaupt de inhoud van die method te hoeven lezen.

Eventueel kun je dan ook de docblock gebruiken voor extra uitleg of linkjes naar tickets of documentatie.

[DaCoTa]

Matis schreef op vrijdag 4 oktober 2019 @ 06:49:
Één van de weinige keren dat ik commentaar gebruik in een stuk code, is wanneer er een (onlogische) business keuze gemaakt is. Dan plemp ik daar de URL naar het ticket in, zodat een toekomstig ontwikkelaar terug kan lezen waarom er zulke gekke keuzes gemaakt worden.

Het lastige is dat commentaren niet altijd goed meeverhuizen met de code. Als dan de code veranderd, kan het zijn dat de originele business case niet meer valide is, maar het commentaar er nog wel staat.

Een mooiere variant vind ik zelf om de business case dan via test-cases af te vangen, waarmee je dus heel eenvoudig het onlogische aspect kunt verduidelijken.

[Mugwump]

DaCoTa schreef op zaterdag 5 oktober 2019 @ 10:29:
[...]

Het lastige is dat commentaren niet altijd goed meeverhuizen met de code. Als dan de code veranderd, kan het zijn dat de originele business case niet meer valide is, maar het commentaar er nog wel staat.

Een mooiere variant vind ik zelf om de business case dan via test-cases af te vangen, waarmee je dus heel eenvoudig het onlogische aspect kunt verduidelijken.

Ik vind testcases nog altijd de beste vorm van documentatie inderdaad. Daarin maak je heel expliciet dat je bepaald gedrag verwacht en als je ze een goede naam geeft ook gelijk waarom je dat verwacht.

[incaz]

Ik zou documentatie het liefst daar hebben waar je het tegenkomt. Dus zeker ook een comment in de code, dat is immers de plek waar het veranderd wordt. En het zinnetje 'voor meer informatie zie.'
Volgens mij zou je dus op elke plek op moeten nemen wat voor die plek relevant is. In de code is dat wel dat er een onlogische keuze gemaakt lijkt te zijn, maar niet de discussie die ertoe geleid heeft. In de commitmessage wel wat er veranderd is, maar niet de argumentatie. In het ticketsysteem wel de overwegingen, maar niet per se alle exacte details, en in de testcases wel allerlei randdetails om ze goed uit te werken.

Bv, ticketsysteem kan beginnen met 'naam met een cedille wordt niet goed weergegeven', idee kan zijn om alles naar UTF8 te doen, in het ticketsysteem komt aan de orde dat dat niet genoeg is en er vanwege wetgeving (en ik zou hier dus ook nog een verwijzing en link naar die specifieke wetgeving opnemen) maar een aantal speciale tekens zijn die zijn toegestaan en anderen volgens vaste manier moeten worden omgezet (ik weet niet meer zeker of dat precies klopt, er was iets mee maar het is lang geleden.) In de code kun je dan een commentaar krijgen met '//let op, extra omzettingsstap voor speciale tekens, UTF8 is niet genoeg, zie ticket [...]'
Commitmessage: 'ticket [...], extra omzettingsstap speciale tekens toegevoegd'
Testcases: de speciale tekens en hun verwachte resultaat, met ticketnummer.

En als er ergens urls breken, dan mis je een deel van de geschiedenis, maar heb je toch op alle plekken wel wat om mee te werken.

[Haan]

C#:

var olderThan18 = DateTime.Parse((currentYear - 19).ToString() + "-12-31");

[RobIII]

Haan schreef op vrijdag 8 november 2019 @ 11:28:

C#:

var olderThan18 = DateTime.Parse((currentYear - 19).ToString() + "-12-31");

Ik snap niet hoe iemand zoiets schrijft en dan voldaan achterover leunt en door gaat met meer code uitschijten. Er is toch geen enkel zichzelf respecterend programmeur die niet inziet wat hier mis mee is?

[CurlyMo]

RobIII schreef op vrijdag 8 november 2019 @ 11:46:
[...]

Ik snap niet hoe iemand zoiets schrijft en dan voldaan achterover leunt en door gaat met meer code uitschijten. Er is toch geen enkel zichzelf respecterend programmeur die niet inziet wat hier mis mee is?

Ik kan op zich nog best mild zijn naar hoe iemand iets implementeert, maar als het simpelweg niet klopt, niet.

[SpeedQber]

Wat geeft DateTime.Parse terug? Ik hoop een bool en geen string

[downtime]

SpeedQber schreef op vrijdag 8 november 2019 @ 12:02:
Wat geeft DateTime.Parse terug? Ik hoop een bool en geen string

Ik hoop dat ie een DateTime terug geeft.

Maarre... Ook als niet-programmeur krijg ik kriebel van zulke code. Het voelt niet goed en dan ga ik zoeken naar iets beters. Het is niet alsof je de eerste bent die het verschil twee datums moet berekenen en dan ga ik het wiel niet opnieuw (slecht) uitvinden als duizenden (betere) programmeurs dat probleem ook al bekeken hebben.

[Mugwump]

downtime schreef op vrijdag 8 november 2019 @ 12:33:
[...]

Ik hoop dat ie een DateTime terug geeft.

Maarre... Ook als niet-programmeur krijg ik kriebel van zulke code. Het voelt niet goed en dan ga ik zoeken naar iets beters. Het is niet alsof je de eerste bent die het verschil twee datums moet berekenen en dan ga ik het wiel niet opnieuw (slecht) uitvinden als duizenden (betere) programmeurs dat probleem ook al bekeken hebben.

In C# kun je dat heel eenvoudig als volgt kunt doen.

C#:

var olderThan18 = DateTime.Now.AddYears(-18);

[heuveltje]

Mugwump schreef op vrijdag 8 november 2019 @ 13:17:
[...]


In C# kun je dat heel eenvoudig als volgt kunt doen.

C#:

var olderThan18 = DateTime.Now.AddYears(-18);

Dan nog verwacht ik dat een var genaamd olderthan18 een Boolean zou moeten bevatten ipv een datum ?

[Mugwump]

heuveltje schreef op vrijdag 8 november 2019 @ 13:42:
[...]


Dan nog verwacht ik dat een var genaamd olderthan18 een Boolean zou moeten bevatten ipv een datum ?

Mja, kun je over twisten, qua conventie is een boolean weer eerder iets als isOlderThan18 vaak. Dat is een beetje een nitpick vergeleken met de functionele fail in die code.

[Matis]

Daarvoor zijn er unit testen, dan valt zo'n stuk code meteen door de mand.

[Mugwump]

Matis schreef op vrijdag 8 november 2019 @ 15:45:
Daarvoor zijn er unit testen, dan valt zo'n stuk code meteen door de mand.

Ligt eraan wie ze schrijft.

[.oisyn]

Mugwump schreef op vrijdag 8 november 2019 @ 13:51:
[...]


Mja, kun je over twisten, qua conventie is een boolean weer eerder iets als isOlderThan18 vaak. Dat is een beetje een nitpick vergeleken met de functionele fail in die code.

Maar dan redeneer je verkeerd om. Wat vind jij dat het datatype van een variabele "olderThan18" dan moet zijn?

Wat ze eigenlijk bedoelen is "18 years ago", dan zou een DateTime wel toepasselijk zijn.

[ Voor 10% gewijzigd door .oisyn op 08-11-2019 17:17 ]

[Mugwump]

.oisyn schreef op vrijdag 8 november 2019 @ 17:16:
[...]


Maar dan redeneer je verkeerd om. Wat vind jij dat het datatype van een variabele "olderThan18" dan moet zijn?

Wat ze eigenlijk bedoelen is "18 years ago", dan zou een DateTime wel toepasselijk zijn.

Klopt, maar de meeste talen doen een beetje moeilijk over variabelenamen die beginnen met een cijfer.
Ik vind olderThan18 vooral slecht gekozen. Het lijkt nog het meest op een predicate die je kunt gebruiken om true of false up te leveren.

[AW_Bos]

Holy hell, wat ik nu tegenkom, bij het helpen van iemand die iets aan zijn site wou toevoegen, slaat echt alles ...
Deze 'widget' die je als iframe kan invoegen op een site is wel erg... huge!
https://www.shoutcastwidg...ist.php?uid=452&frame=yes

Even ontleden:
- Meteen een hele bootstrap die inhoudelijk in het bestand staat, en dus niet even wordt ingeladen vanuit een een paas losse bestanden (cachen wordt al te-niet gedaan)
- Vervolgens worden de items geladen, niks mis mee, maar wacht.... elk item die geladen wordt laadt weer op zijn beurt https://unpkg.com/sweetalert/dist/sweetalert.min.js in
- En dan denk je dat we klaar zijn... nee hoor, want elk item heeft ook een eigen jQuery functie die steeds uitgevoerd wordt, en overschreven wordt.

Al met al wordt er op die manier enkele MB's ingeladen, en duurt het volledige renderen iets van 15 seconden.

En dan denk je: "Oh, maar dat fixxen ze wel?"
Welnee, op de chat is het advies om het aantal te verlagen
En uiteraard wordt het gesprek door hun beëindigd met: "Waar maak je je druk om?"
Bam, meteen een IP-ban en de chat sluit automatisch

Tja

[BertS]

Ik moest lachen om je sig onder deze post

[Antrax]

RobIII schreef op vrijdag 8 november 2019 @ 11:46:
[...]

Ik snap niet hoe iemand zoiets schrijft en dan voldaan achterover leunt en door gaat met meer code uitschijten. Er is toch geen enkel zichzelf respecterend programmeur die niet inziet wat hier mis mee is?

Dit kon wel eens mijn nieuwe signature worden

[MBV]

RobIII schreef op vrijdag 8 november 2019 @ 11:46:
[...]

Ik snap niet hoe iemand zoiets schrijft en dan voldaan achterover leunt en door gaat met meer code uitschijten. Er is toch geen enkel zichzelf respecterend programmeur die niet inziet wat hier mis mee is?

Helaas zijn er genoeg zichzelf respecterende programmeurs die ik hiertoe in staat zie, en die dus niet mijn respect verdienen

Matis schreef op vrijdag 8 november 2019 @ 15:45:
Daarvoor zijn er unit testen, dan valt zo'n stuk code meteen door de mand.

Hoe? Dit stuk broddelwerk levert toch een datetime op van 31 december 19 jaar geleden, dus afhankelijk van hoe het gebruikt wordt kan het het juiste resultaat opleveren (wanneer een datum wordt vergeleken met greater-than met deze datum, kan je zien of iemand dit jaar 18 wordt).
En trouwens: goede unittests schrijven is moeilijker dan goede code, dus als ik dit zie vertrouw ik er niet op

[Mugwump]

MBV schreef op woensdag 13 november 2019 @ 22:04:
En trouwens: goede unittests schrijven is moeilijker dan goede code, dus als ik dit zie vertrouw ik er niet op

Daar kun je dan weer mutation tests voor gebruiken.

[MBV]

Mugwump schreef op woensdag 13 november 2019 @ 22:38:
[...]


Daar kun je dan weer mutation tests voor gebruiken.

Unit-tests schrijven zou makkelijker moeten zijn, zeker met TDD, maar mijn ervaring met collega's is toch totaal anders. Om te beginnen moet je een goede test-infrastructuur opzetten, dan moet je 'alle' testcases uitschrijven, en je moet daarbij de neiging overwinnen om testen over te slaan 'want hij doet het toch?!?'.
Als ik iets nieuws opzet zorg ik als een van de eerste dingen voor een unit-test-infrastructuur, om op verder te bouwen. Maar als je ergens halverwege een project binnenkomt is dat lastig.

[JeroenE]

MBV schreef op woensdag 13 november 2019 @ 22:04:
Hoe? Dit stuk broddelwerk levert toch een datetime op van 31 december 19 jaar geleden, dus afhankelijk van hoe het gebruikt wordt kan het het juiste resultaat opleveren (wanneer een datum wordt vergeleken met greater-than met deze datum, kan je zien of iemand dit jaar 18 wordt).

Wellicht het vroege tijdstip, maar hoe zou je dat dan met deze datum kunnen checken?

Stel: iemand is geboren op 1 december 2002. Dan is die datum groter dan 31 december 2000. Maar die persoon wordt dit jaar niet 18 maar 17.

En een andere test (om te zien of iemand al 18 of ouder is) klopt ook niet. Want als je bent geboren op 1 januari 2001 dan ben je nu al 18 maar als je het vergelijkt met 31 december 2000 dan zegt het systeem dat je dat nog niet bent.

Nouja, misschien is er wel een specifiek domein waar zo'n test wel nodig is omdat iets pas mag vanaf het eerste nieuwe kalenderjaar nadat je 18 bent geworden. Dan maken wij het te ingewikkeld door te focussen op het gebruik van 31 december 19 jaar geleden in plaats van de onhandige manier om op die datum te komen.

Misschien gaat het niet eens om mensen maar denken wij dat alleen omdat de grens van 18 jaar bij veel mensen als grote mijlpaal wordt gezien vanwege allerlei wettelijke regels. Voor hetzelfde geld gaat het over het printen van een label voor whisky (of whiskey). Ik ken daar de exacte regels niet van.

[CurlyMo]

JeroenE schreef op donderdag 14 november 2019 @ 07:49:
[...]
Nouja, misschien is er wel een specifiek domein waar zo'n test wel nodig is omdat iets pas mag vanaf het eerste nieuwe kalenderjaar nadat je 18 bent geworden. Dan maken wij het te ingewikkeld door te focussen op het gebruik van 31 december 19 jaar geleden in plaats van de onhandige manier om op die datum te komen.

Dan is het aan je documentatie om uit te leggen waarom je afwijkt van wat gangbaar is.

[P-Storm]

JeroenE schreef op donderdag 14 november 2019 @ 07:49:
[...]
Wellicht het vroege tijdstip, maar hoe zou je dat dan met deze datum kunnen checken?

Stel: iemand is geboren op 1 december 2002. Dan is die datum groter dan 31 december 2000. Maar die persoon wordt dit jaar niet 18 maar 17.

En een andere test (om te zien of iemand al 18 of ouder is) klopt ook niet. Want als je bent geboren op 1 januari 2001 dan ben je nu al 18 maar als je het vergelijkt met 31 december 2000 dan zegt het systeem dat je dat nog niet bent.

Nouja, misschien is er wel een specifiek domein waar zo'n test wel nodig is omdat iets pas mag vanaf het eerste nieuwe kalenderjaar nadat je 18 bent geworden. Dan maken wij het te ingewikkeld door te focussen op het gebruik van 31 december 19 jaar geleden in plaats van de onhandige manier om op die datum te komen.

Misschien gaat het niet eens om mensen maar denken wij dat alleen omdat de grens van 18 jaar bij veel mensen als grote mijlpaal wordt gezien vanwege allerlei wettelijke regels. Voor hetzelfde geld gaat het over het printen van een label voor whisky (of whiskey). Ik ken daar de exacte regels niet van.

Op zich is het logisch, maar meer de WTF is dat niet de new DateTime(year, month, date) is gebruikt, maar voor een tekst oplossing is gegaan Deze heeft netjes integers als parameters.Vanuit daar kan dan teruggerekend worden in de jaren

Of ik lees je tekst verkeerd en doel je juist op het domein daarvoor, dan blame ik het ook op te vroeg

[ Voor 3% gewijzigd door P-Storm op 14-11-2019 09:02 ]

[Voutloos]

JeroenE schreef op donderdag 14 november 2019 @ 07:49:
[...]
Wellicht het vroege tijdstip, maar hoe zou je dat dan met deze datum kunnen checken?

Stel: iemand is geboren op 1 december 2002. Dan is die datum groter dan 31 december 2000. Maar die persoon wordt dit jaar niet 18 maar 17.

De code is zo slecht dat jij nu totaal verward bent, uiteraard check je op of datum voor ipv na deze peildatum ligt.

En een andere test (om te zien of iemand al 18 of ouder is) klopt ook niet. Want als je bent geboren op 1 januari 2001 dan ben je nu al 18 maar als je het vergelijkt met 31 december 2000 dan zegt het systeem dat je dat nog niet bent.

olderThan he, niet olderThanOrEqual, dus dit voorbeeld slaagt toevallig wel.

Dat gezegd hebbende, code is minstens erbarmelijk en als de 12-31 ook niet een domeinregel is zijn er geen woorden voor.

[BladeSlayer1000]

AW_Bos schreef op zondag 10 november 2019 @ 22:30:
Holy hell, wat ik nu tegenkom, bij het helpen van iemand die iets aan zijn site wou toevoegen, slaat echt alles ...
Deze 'widget' die je als iframe kan invoegen op een site is wel erg... huge!
https://www.shoutcastwidg...ist.php?uid=452&frame=yes

Even ontleden:
- Meteen een hele bootstrap die inhoudelijk in het bestand staat, en dus niet even wordt ingeladen vanuit een een paas losse bestanden (cachen wordt al te-niet gedaan)
- Vervolgens worden de items geladen, niks mis mee, maar wacht.... elk item die geladen wordt laadt weer op zijn beurt https://unpkg.com/sweetalert/dist/sweetalert.min.js in
- En dan denk je dat we klaar zijn... nee hoor, want elk item heeft ook een eigen jQuery functie die steeds uitgevoerd wordt, en overschreven wordt.

Al met al wordt er op die manier enkele MB's ingeladen, en duurt het volledige renderen iets van 15 seconden.

En dan denk je: "Oh, maar dat fixxen ze wel?"
Welnee, op de chat is het advies om het aantal te verlagen
En uiteraard wordt het gesprek door hun beëindigd met: "Waar maak je je druk om?"
Bam, meteen een IP-ban en de chat sluit automatisch

Tja

Wie heeft dit ooit bedacht, wat waardeloos...

[mcDavid]

Voutloos schreef op donderdag 14 november 2019 @ 10:07:
[...]
De code is zo slecht dat jij nu totaal verward bent, uiteraard check je op of datum voor ipv na deze peildatum ligt.
[...]
olderThan he, niet olderThanOrEqual, dus dit voorbeeld slaagt toevallig wel.

Dat gezegd hebbende, code is minstens erbarmelijk en als de 12-31 ook niet een domeinregel is zijn er geen woorden voor.

och heer, over <= vs < gesproken, ik had vorige week ook zo'n prachtige (treurigmakende) diff:

PHP:

    /**
     * Check when process can not run or fails it its
     * has to add new Process by retry count of process.
     *
     * @param  Process $process
     */
    public function checkIfNeedToRetry(Process $process)

        // Check if need for new Process
        if ($process->getRetryCount() <= $process->getMaxRetry()) {
            // some code that initiates a retry;
        }
    }

heb de comments die erbij stonden ook maar meegepost, dan is ten minste duidelijk wat deze method zou moeten doen Die heb ik uiteindelijk ook maar weggehaald en in plaats daarvan de method "retryIfNecessary" genoemd. Leek me duidelijker dan dit verhaal.

[Voutloos]

Dat docblock had ook echt een negatieve waarde. Dat de retry geïnitieerd wordt mist en dat is het %*{%* het belangrijkste aan het hele proces. En dan nog opgeschreven met Engels woorden, maar Engels grammatica (of desnoods Nederlandse woordvolgorde) is het in ieder geval niet.

Ik zou trouwens de conditie ín het object stoppen, dus enkel isRetryAllowed() ipv de count en de max allebei uit het object halen.

[ Voor 19% gewijzigd door Voutloos op 14-11-2019 10:59 ]

[JeroenE]

P-Storm schreef op donderdag 14 november 2019 @ 09:01:
Of ik lees je tekst verkeerd en doel je juist op het domein daarvoor, dan blame ik het ook op te vroeg

Wat ik bedoel is dat er misschien een hele goede reden is dat er getest wordt op jaartal plus 31 december. Misschien volgt dat wel uit regelgeving of een gebruikelijke manier in een branche of bedrijf waar ik geen weet van heb.

Om een voorbeeld te geven, in een bedrijf krijg je een extra vakantiedag als je 5 jaar of langer in dienst bent. Maar vakantiedagen worden toegekend op 1 januari. Dus als je op 1 februari 2014 in dienst bent gekomen krijg je die extra dag pas op 1 januari 2020. En als je op 31 december 2014 in dienst bent gekomen dan krijg je die extra dag ook op 1 januari 2020.

De regeling had net zo goed kunnen zijn dat je die extra dag al gelijk op de dag dat je 5 jaar in dienst bent krijgt. Maar ja, zo is die regeling niet dus in dat geval kan je een check maken die de datum in dienst controleert met het vaste gegeven van 1 januari en dan aangevuld met het huidige jaartal minus 5.

Voutloos schreef op donderdag 14 november 2019 @ 10:07:
De code is zo slecht dat jij nu totaal verward bent, uiteraard check je op of datum voor ipv na deze peildatum ligt.

Zo uiteraard vind ik dat niet. Je kan op twee manieren controleren:

1. ligt de geboortedatum voor de testdatum
2. ligt de testdatum achter de geboortedatum

Op beide manieren weet je of iemand ouder dan 18 jaar is.

olderThan he, niet olderThanOrEqual, dus dit voorbeeld slaagt toevallig wel.

Dat is wellicht weer mijn eigen interpretatie dat je checkt op 18 jaar en niet dat je checkt of iemand ouder dan 18 jaar (= groter of gelijk dan 19 jaar) is.

Als je het wel zo leest dat het groter of gelijk aan 19 moet zijn dan klopt die test weer niet met een datum als 1 december 2000. Want je bent dan nu nog geen 19 terwijl de test of je geboortedatum voor 31 december 2000 ligt wel waar is.

[.oisyn]

JeroenE schreef op donderdag 14 november 2019 @ 11:11:
[...]
Wat ik bedoel is dat er misschien een hele goede reden is dat er getest wordt op jaartal plus 31 december. Misschien volgt dat wel uit regelgeving of een gebruikelijke manier in een branche of bedrijf waar ik geen weet van heb.

Om een voorbeeld te geven, in een bedrijf krijg je een extra vakantiedag als je 5 jaar of langer in dienst bent. Maar vakantiedagen worden toegekend op 1 januari. Dus als je op 1 februari 2014 in dienst bent gekomen krijg je die extra dag pas op 1 januari 2020.

Dat volgt niet uit je eigen criteria. Iemand die 1 jan 2014 in dienst is, is op 1 jan 2019 al 5 jaar in dienst, dus die zou op die dag al een extra vakantiedag moeten krijgen.

De regeling had net zo goed kunnen zijn dat je die extra dag al gelijk op de dag dat je 5 jaar in dienst bent krijgt. Maar ja, zo is die regeling niet dus in dat geval kan je een check maken die de datum in dienst controleert met het vaste gegeven van 1 januari en dan aangevuld met het huidige jaartal minus 5.

Als je de uitkomst wil zoals je beschrijft zal ik niets met datumchecks doen maar gewoon kijken naar het jaartal. Dat is immers leidend; iemand die in jaar X in dienst is getreden, krijgt de extra dag pas in jaar X+6. Dat in een datum-type proberen te gieten geeft alleen maar een extra bron voor fouten.

[ Voor 6% gewijzigd door .oisyn op 14-11-2019 11:36 ]

[JeroenE]

.oisyn schreef op donderdag 14 november 2019 @ 11:34:
Dat volgt niet uit je eigen criteria. Iemand die 1 jan 2014 in dienst is, is op 1 jan 2019 al 5 jaar in dienst, dus die zou op die dag al een extra vakantiedag moeten krijgen.

Dat klopt, maar mijn voorbeeld was 1 februari 2014. En dan ben je op 1 januari 2019 dus nog maar 4 jaar en 11 maanden in dienst en krijg je geen extra dag.

Als je de uitkomst wil zoals je beschrijft zal ik niets met datumchecks doen maar gewoon kijken naar het jaartal. Dat is immers leidend; iemand die in jaar X in dienst is getreden, krijgt de extra dag pas in jaar X+6.

Behalve dus als je op 1 januari in dienst bent gekomen, dan is het wel X+5. Dan moet je daar weer een uitzondering voor gaan maken.

[.oisyn]

Oh jezus ik las echt "1 januari 2014" . Ja, als die ene dag uitmaakt dan moet je dat idd op die manier doen. Je kunt je afvragen of dat conceptueel handig is, maar dat ligt buiten de scope van de architect

[Haan]

Nog steeds discussie gaande over 'olderThan18' (mijn grootste WTF was inderdaad de creatieve manier waarop het DateTime object gemaakt werd, maar de naam van de variabele zelf is ook niet helemaal handig)

Ik heb zelf ook even goed moeten kijken hoe het nou precies zit, maar het wordt dus gebruikt in een stukje berekening mbt Kindgebonden budget, waarbij deze variabele als een ondergrens gebruikt wordt om kinderen weg te filteren die dus al ouder dan 18 zijn:

code:

if (geboortedatum kind > 'olderThan18') doe iets

[CurlyMo]

Haan schreef op donderdag 14 november 2019 @ 11:46:
Nog steeds discussie gaande over 'olderThan18' (mijn grootste WTF was inderdaad de creatieve manier waarop het DateTime object gemaakt werd, maar de naam van de variabele zelf is ook niet helemaal handig)

Ik heb zelf ook even goed moeten kijken hoe het nou precies zit, maar het wordt dus gebruikt in een stukje berekening mbt Kindgebonden budget, waarbij deze variabele als een ondergrens gebruikt wordt om kinderen weg te filteren die dus al ouder dan 18 zijn:

code:

if (geboortedatum kind > 'olderThan18') doe iets

Terwijl het stukje code daar dus niet voor geschikt is want het geeft uitgaande van je beschrijving verkeerde resultaten.

[Janoz]

CurlyMo schreef op donderdag 14 november 2019 @ 11:55:
[...]

Terwijl het stukje code daar dus niet voor geschikt is want het geeft uitgaande van je beschrijving verkeerde resultaten.

Mwah, lees het stukje van @JeroenE nog even een keertje .


Hier is nog een voorbeeld van zoiets:

Artikel 2; deelname
https://strongviking.com/nl/condities/

........ minimum leeftijd van 5 jaar (of wordt 5 jaar in dat kalenderjaar)......

[heuveltje]

Janoz schreef op donderdag 14 november 2019 @ 16:16:
[...]


Mwah, lees het stukje van @JeroenE nog even een keertje .


Hier is nog een voorbeeld van zoiets:

Artikel 2; deelname
https://strongviking.com/nl/condities/


[...]

niet zozeer codegerelateerd, maar wtf, je gaat je kind van 4 mee laten rennen in een 3km obstakel run.

[ Voor 3% gewijzigd door heuveltje op 14-11-2019 16:53 ]

[Janoz]

heuveltje schreef op donderdag 14 november 2019 @ 16:49:
[...]


niet zozeer codegerelateerd, maar wtf, je gaat je kind van 4 mee laten rennen in een 3km obstakel run.

Oh, ik wel hoor. Sowieso zijn de hindernissen natuurlijk ook op die leeftijd afgesteld, en daarnaast zijn kinderen over het algemeen een stuk behendiger in dergelijke dingen dan de gemiddelde volwassene.

[CurlyMo]

Janoz schreef op donderdag 14 november 2019 @ 16:16:
[...]
Mwah, lees het stukje van @JeroenE nog even een keertje .

Haan schreef op vrijdag 8 november 2019 @ 11:28:

C#:

var olderThan18 = DateTime.Parse((currentYear - 19).ToString() + "-12-31");

Voor het gemak alles even als integers. Stel is het nu 20190601 dan is de output van dit stukje 20001231. Iemand geboren op 20010101 is dan geen 18 op de huidige datum, maar dat is die wel volgens dit script.

Dit voorbeeld klopte inderdaad niet.

olderThan18 gaat qua variabele goed, mits je het niet omgekeerd gaat gebruiken om te checken of iemand jonger is dan 18.

[ Voor 3% gewijzigd door CurlyMo op 15-11-2019 13:09 ]

[Matis]

Ik zou gewoon de geboortedatum pakken, daar 18 jaar bij optellen en vergelijken met de (huidige) datum. Is die kleiner, dan is het antwoord nee.

Even kijken per programmeertaal hoe ze omgaat met schrikkeljaar en wat corner cases rond de jaarwisseling, maar verder zou ik het niet veel moeilijker maken.

[Janoz]

CurlyMo schreef op donderdag 14 november 2019 @ 19:11:
[...]


[...]

Voor het gemak alles even als integers. Stel is het nu 20190601 dan is de output van dit stukje 20001231. Iemand geboren op 20010101 is dan geen 18 op de huidige datum, maar dat is die wel volgens dit script.

Iemand die op 1 jan 2001 geboren is, wordt 18 op 1 jan 2019. Op 1 juni 2019 zal hij waarschijnlijk zijn rijbewijs al hebben

[Merethil]

Matis schreef op donderdag 14 november 2019 @ 21:24:
Ik zou gewoon de geboortedatum pakken, daar 18 jaar bij optellen en vergelijken met de (huidige) datum. Is die kleiner, dan is het antwoord nee.

Even kijken per programmeertaal hoe ze omgaat met schrikkeljaar en wat corner cases rond de jaarwisseling, maar verder zou ik het niet veel moeilijker maken.

Als je een goede datetimelibrary gebruikt dan houdt die daar rekening mee en heeft ie vaak een "addYears(int years)"-functie waar je er 18 bij op kan tellen. Dan zal het altijd moeten kloppen.

[Crazy D]

Janoz schreef op donderdag 14 november 2019 @ 16:16:
[...]


Mwah, lees het stukje van @JeroenE nog even een keertje .


Hier is nog een voorbeeld van zoiets:

Artikel 2; deelname
https://strongviking.com/nl/condities/


[...]

En zo zie je maar weer hoe belangrijk de specs zijn (even los van het feit of de code zo geweldig is en het juiste resultaat geeft) maar de subtiele aanvulling "of in het jaar waarin...." is wel een cruciaal stukje waar de gemiddelde programmeur wellicht erg makkelijk overheen kijkt

[downtime]

Matis schreef op donderdag 14 november 2019 @ 21:24:
Ik zou gewoon de geboortedatum pakken, daar 18 jaar bij optellen en vergelijken met de (huidige) datum. Is die kleiner, dan is het antwoord nee.

Even kijken per programmeertaal hoe ze omgaat met schrikkeljaar en wat corner cases rond de jaarwisseling, maar verder zou ik het niet veel moeilijker maken.

Juist niet naar schrikkeljaren en corner cases kijken. Die zijn immers irrelevant bij het vergelijken van dit soort datums. Iemand die op 1 maart is geboren in een gewoon jaar viert zijn verjaardag immers niet opeens op 29 februari in een schrikkeljaar. Je moet echt 18 jaar bij de geboortedatum optellen en niet 18 maal 365,25 dagen.

Ik realiseer me nu opeens dat ook dit een corner case kent. Iemand die op 29 februari is geboren wordt 18 jaar later meerderjarig op een datum die in dat jaar niet voorkomt. Dat zal zelden een probleem opleveren maar is niettemin een corner case.

[ Voor 14% gewijzigd door downtime op 15-11-2019 10:42 ]

[CurlyMo]

Janoz schreef op vrijdag 15 november 2019 @ 09:47:
[...]

Iemand die op 1 jan 2001 geboren is, wordt 18 op 1 jan 2019. Op 1 juni 2019 zal hij waarschijnlijk zijn rijbewijs al hebben

Je reageert niet op wat ik schrijf. Bepalen of iemand ouder is dan 18 gaat wel goed, alleen niet of iemand jonger is dan 18.

[Janoz]

Ik reageer wel degelijk op wat je schrijft . Jij zegt dat iemand die op 1 januari 2001 geboren is op 6 juli 2019 nog geen 18 is, maar die is dan al 18 jaar en een paar maanden oud. Je zult met een correct voorbeeld moeten komen om je punt te maken.

[Matis]

downtime schreef op vrijdag 15 november 2019 @ 10:29:
Juist niet naar schrikkeljaren en corner cases kijken. Die zijn immers irrelevant bij het vergelijken van dit soort datums. Iemand die op 1 maart is geboren in een gewoon jaar viert zijn verjaardag immers niet opeens op 29 februari in een schrikkeljaar. Je moet echt 18 jaar bij de geboortedatum optellen en niet 18 maal 365,25 dagen.

Nee, ik bedoelde juist personen die op 29 februari geboren zijn. Ik heb in PHP (ja ik weet het) af en toe iets moois, als je op 30 januari + 1 maand doet, dan komt ie ergens in de eerste dagen van maart uit (afhankelijk van wel of geen schrikkeljaar). Slaat ie mooi februari over.

Been there, done that

PHP:

$time = strtotime("2019-01-31");
$final = date("Y-m-d", strtotime("+1 month", $time));
var_dump($final);

string(10) "2019-03-03"

[ Voor 10% gewijzigd door Matis op 15-11-2019 10:52 ]

[Merethil]

Matis schreef op vrijdag 15 november 2019 @ 10:43:
[...]

Nee, ik bedoelde juist personen die op 29 februari geboren zijn. Ik heb in PHP (ja ik weet het) af en toe iets moois, als je op 30 januari + 1 maand doet, dan komt ie ergens in de eerste dagen van maart uit (afhankelijk van wel of geen schrikkeljaar). Slaat ie mooi februari over.

Been there, done that

PHP:

$time = strtotime("2019-01-31");
$final = date("Y-m-d", strtotime("+1 month", $time));
var_dump($final);

string(10) "2019-03-03"

Dat is toch gewoon verwacht gedrag? Je telt er een maand bij op, dus ga je naar 31-02-2019, wat niet bestaat en dus overflowt hij. Wat had je zelf verwacht dat 'ie zou doen dan?

Is trouwens ook zo in andere talen. Java doet hetzelfde vziw. En C# vast ook. En JavaScript, python en Ruby doen dat volgens mij ook.
Had ik het toch even fout zeg...

[ Voor 10% gewijzigd door Merethil op 15-11-2019 12:21 ]

[Janoz]

Merethil schreef op vrijdag 15 november 2019 @ 10:58:
[...]


Dat is toch gewoon verwacht gedrag? Je telt er een maand bij op, dus ga je naar 31-02-2019, wat niet bestaat en dus overflowt hij. Wat had je zelf verwacht dat 'ie zou doen dan?

Is trouwens ook zo in andere talen. Java doet hetzelfde vziw. En C# vast ook. En JavaScript, python en Ruby doen dat volgens mij ook.

Nope

Java:

    public void telMaandOp() {
        LocalDate datum = LocalDate.of(2020,1,31);
        System.out.println(datum.plusMonths(1));
    }

output:

2020-02-29


--edit--
Oh, en nu ik toch bezig ben, dit is dan wel weer grappig:

Java:

    @Test
    public void telMaandOp() {
        LocalDate datum = LocalDate.of(2020,1,31);
        System.out.println(datum.plusMonths(1));
        System.out.println(datum.plusMonths(2));
        System.out.println(datum.plusMonths(1).plusMonths(1));
    }

Output:
2020-02-29
2020-03-31
2020-03-29

[ Voor 24% gewijzigd door Janoz op 15-11-2019 11:12 ]

[Kanarie]

Ruby doet dit

code:

>> Date.new(2019, 1, 31) >> 1
=> #<Date: 2019-02-28 ((2458543j,0s,0n),+0s,2299161j)>

Of met ActiveSupport geladen

code:

>> Date.new(2019, 1, 31) + 1.month
=> Thu, 28 Feb 2019