[Javascript] Code leesbaarder maken (loop in loop in functie

[JT]

Ik ben hobby-scripter/wannabe-developer, keep that in mind Dat gezegd hebbende vind ik letterlijk alles waarover je feedback wil geven prima hoor, geen probleem. Zelfs als je met een enkele regel komt die alles doet wat ik heb gemaakt, dan kun je dat zeggen Leer ik alleen maar van. Goed, het gaat om het volgende. Ik heb een stukje code geschreven met daarin een functie. In die functie zit een loop met daarin weer een loop. Volgens mij prima want ik wil voor elke arraywaarde (object) (moederloop) meerdere mogelijkheden uitrekenen, wat ik dus met de tweede loop doe (kindloop ). Het werkt en geeft de gewenste output. Alleen....ik vind dat de leesbaarheid te wensen over laat. Wellicht door de vele losse comments (juist een poging tot documenteren) maar ik heb het gevoel dat een loop in een loop in een functie ook niet helpt. Daarom vroeg ik mij af, is het voor de leesbaarheid handiger dat ik de diepste loop eruit haal en in een eigen functie stop? Ik heb de zoekmachine geraadpleegd voor meningen en advies op internet maar niks gevonden. Het maken is niet het probleem, ik ben meer op zoek naar feedback van mensen met ervaring.

Let op: dit is alleen die functie. hoursInBlock is een integer die aangeeft hoeveel uur in een aaneengesloten blok moeten zitten.

code:

function findCheapestHours(hoursInBlock) {
    let calculatedPrice
    let consecutiveHourCount
    let bestPricedBlockStart
    let arrayHour
    let nextHour
    let epochSecondsToAdd = 0
    let sumHourPrices = 0

    //For every hour in array
    for (
        var arrayCount = 0;
        arrayCount <= price_array.length - hoursInBlock;
        arrayCount++
    ) {
        sumHourPrices = 0
        arrayHour = price_array[arrayCount]     
        //Calculate average price for coming <hoursInBlock> hours
        for (
            consecutiveHourCount = 0;
            consecutiveHourCount < hoursInBlock;
            consecutiveHourCount++
        ) {
            //Add 3600 seconds for every extra consecutive hour
            epochSecondsToAdd = consecutiveHourCount * 3600
            //Find next hour in price array based on extra added 3600 seconds
            nextHour = price_array.findIndex(
                (object) =>
                    new Date(object.startsAt).getTime() == new Date(arrayHour.startsAt).getTime() + (epochSecondsToAdd * 1000)
            )
            sumHourPrices += price_array[nextHour].total
        }
        //Calculate average price by dividing sum of prices by amount of hours
        calculatedPrice = sumHourPrices / hoursInBlock
        //If calculated average price is lower than already stored calculated price
        if (
            bestPricedBlockStart == null || calculatedPrice < bestPricedBlockStart.total

        ) {
            //Store current hour as best priced to start action
            bestPricedBlockStart = arrayHour
        }
    }
    console.log("Definitieve beste start: " + bestPricedBlockStart.startsAt)
    return bestPricedBlockStart
}

[crisp]

Ik zou dit niet met een geneste loop doen, maar eerst een loop die price_array omzet naar een array met gemiddelde (of totaal) prijzen per 'block' van uren, en daarna een loop om daaruit het goedkoopste blok te pakken.

Zelf vind ik het gebruik van array methods met callbacks (forEach, map, reduce etc) ook fijner en leesbaarder dan for-of while-loops

[Soultaker]

Het afsplitsen van code naar een aparte functie is vooral nuttig wanneer die functie op een kleinere state opereert dan de originele functie. Dan kun je helderheid scheppen door de code die minder context nodig heeft naar een aparte functie te verplaatsen. Dat helpt omdat hoe eenvoudig een functie te begrijpen is vooral afhangt van de complexiteit van de data waarop die functie opereert, en in veel mindere mate de lengte van de code zelf.

In dit geval lijkt het afsplitsen me niet bijzonder nuttig: alle parameters van findCheapestHours() zijn ook relevant in de binnenste lus. Je kunt de code dus niet veel versimpelen. Hooguit kun je de buitenste lus eraf halen (en dan arrayCount als parameter aan je nieuwe functie toevoegen) maar dat levert qua leesbaarheid niet veel op.

Ik zou liever de functie zoals die is proberen leesbaarder te maken. Om te beginnen kun je de declaraties van variabelen naar de scope waarin ze relevant zijn verplaatsen. calculatedPrice, consecutiveHourCount, arrayHour, nextHour, epochSecondsToAdd, sumHourPrices hoeven geen van allen op functie-niveau gedefinieerd te worden.

Als algemene regel kun je aanhouden: plaats declaraties in de meest nauwe scope mogelijk, en initialiseer variabelen in de declaratie.

Je kunt ook de naamgeving verbeteren; `arrayCount` is eigenlijk een index in price_array bijvoorbeeld. De naam `arrayCount` is dan eerder verwarrend.

Verder kun je de logica mogelijk versimpelen: de manier waarop de code geschreven is lijkt te impliceren dat de elementen in price_array al gesorteerd zijn als opeenvolgende oplopende uren. Als dat zo is, waarom dan zo moelijk doen met findIndexAt()? (Waarbij het predicaat ook nog eens vrij inefficiënt geïmplementeerd is.)

[JT]

Leuk, input om over na te denken

crisp schreef op vrijdag 23 mei 2025 @ 14:13:
Ik zou dit niet met een geneste loop doen, maar eerst een loop die price_array omzet naar een array met gemiddelde (of totaal) prijzen per 'block' van uren, en daarna een loop om daaruit het goedkoopste blok te pakken.

De price_array is al een kopie van 2 arrays met prijzen van 1 dag per array waar de uren uit zijn die al gepasseerd zijn én waar de uren uit zijn die niet meetellen. De functionaliteit is namelijk dat je aangeeft vóór welk dagdeel (ochtend, middag, avond) op welke dag (vandaag of morgen) je iets wil doen. Price_array is dus
1. Een selectie van de uren die relevant zijn, die dus voor elke aanroep anders kunnen zijn
2. De uren van vandaag en morgen samengevoegd.

Het aantal uren kan daarbij ook bij elke aanroep anders zijn, in de praktijk zal het een integer zijn tussen 2 en 8. Omdat de prijzen elke dag rond 13:00 opgehaald kunnen worden, zullen net na dat moment er nog 35 te verstrijken uren zijn. Dan zijn er dus 34 combinaties te berekenen zijn voor 2 uur, t/m 8 uur zijn dat 33 + 32 + 31 + 30 + 29 + 28 = 217 combinaties.

Dat maakt eveneens (34*1) + (33*2) + (32*3) + (31*4) + (30*5) + (29*6) + (28*7) = 840 loops om alles op voorhand te berekenen, los van de rest van de code. Is dat dan niet veel overhead om het allemaal voorhanden te hebben?

Zelf vind ik het gebruik van array methods met callbacks (forEach, map, reduce etc) ook fijner en leesbaarder dan for-of while-loops

Sterker nog, nadat ik er niet kwam met for (let x in array) loops (ik kreeg in de loop niet bij elke regel code het voor elkaar om property's tevoorschijn te halen) ging ik hier op over. Tot ik in mijn zoektocht op internet vond dat deze erg inefficient zijn. Kan zelfs een factor 20 schelen! Toen besloot ik weer terug te krijgen op een for loop maar dan deze variant en daarmee kreeg ik het ook vrij snel werkend.

Soultaker schreef op vrijdag 23 mei 2025 @ 14:59:
Het afsplitsen van code naar een aparte functie is vooral nuttig wanneer die functie op een kleinere state opereert dan de originele functie. Dan kun je helderheid scheppen door de code die minder context nodig heeft naar een aparte functie te verplaatsen. Dat helpt omdat hoe eenvoudig een functie te begrijpen is vooral afhangt van de complexiteit van de data waarop die functie opereert, en in veel mindere mate de lengte van de code zelf.

In dit geval lijkt het afsplitsen me niet bijzonder nuttig: alle parameters van findCheapestHours() zijn ook relevant in de binnenste lus. Je kunt de code dus niet veel versimpelen. Hooguit kun je de buitenste lus eraf halen (en dan arrayCount als parameter aan je nieuwe functie toevoegen) maar dat levert qua leesbaarheid niet veel op.

Ik zou liever de functie zoals die is proberen leesbaarder te maken. Om te beginnen kun je de declaraties van variabelen naar de scope waarin ze relevant zijn verplaatsen. calculatedPrice, consecutiveHourCount, arrayHour, nextHour, epochSecondsToAdd, sumHourPrices hoeven geen van allen op functie-niveau gedefinieerd te worden.

Als algemene regel kun je aanhouden: plaats declaraties in de meest nauwe scope mogelijk, en initialiseer variabelen in de declaratie.

Daar kan ik nog eens extra naar kijken. Ik loop nog wel eens te klooien met de beschikbaarheid van waardes in specifieke scopes; misschien moet ik er eens goed voor gaan zitten om te doen wat jij zegt en het werkend te houden. Zou wel een mooi leermoment weer zijn.

Je kunt ook de naamgeving verbeteren; `arrayCount` is eigenlijk een index in price_array bijvoorbeeld. De naam `arrayCount` is dan eerder verwarrend.

Grappig, ik heb het count genoemd omdat ik het in eerste plaats ook gebruik als count voor de loop, jouw perspectief had ik nog niet zo gezien.

Verder kun je de logica mogelijk versimpelen: de manier waarop de code geschreven is lijkt te impliceren dat de elementen in price_array al gesorteerd zijn als opeenvolgende oplopende uren. Als dat zo is, waarom dan zo moelijk doen met findIndexAt()? (Waarbij het predicaat ook nog eens vrij inefficiënt geïmplementeerd is.)

In reactie op crisp beschreef ik zojuist hoe deze array wordt opgebouwd. De volgorde wordt dus overgenomen uit de twee eerder genoemde arrays. Deze haal ik op bij een API. Nu lijkt het me logisch dat de volgorde hetzelfde blijft, maar als dit een keer niet gebeurt dan werkt mijn code niet meer. Nu is het aan de andere kant ook zo dat ik perfectionistisch kan zijn binnen mijn eigen kunnen (je zult nu vast denken, perfectionisme met jouw vaardigheden heeft weinig zin ) dus is mijn vraag, ga ik dan te ver met deze zekerheid inbouwen met als gevolg wat extra regels code? En wat bedoel je met de implementatie? Dat ik de code beter anders had kunnen schrijven of dat ik een ongelukkige methode gebruik ofzo?

[crisp]

JT schreef op vrijdag 23 mei 2025 @ 21:10:
Leuk, input om over na te denken


[...]

De price_array is al een kopie van 2 arrays met prijzen van 1 dag per array waar de uren uit zijn die al gepasseerd zijn én waar de uren uit zijn die niet meetellen. De functionaliteit is namelijk dat je aangeeft vóór welk dagdeel (ochtend, middag, avond) op welke dag (vandaag of morgen) je iets wil doen. Price_array is dus
1. Een selectie van de uren die relevant zijn, die dus voor elke aanroep anders kunnen zijn
2. De uren van vandaag en morgen samengevoegd.

Heb je daar een concreet voorbeeld van?

Het aantal uren kan daarbij ook bij elke aanroep anders zijn, in de praktijk zal het een integer zijn tussen 2 en 8. Omdat de prijzen elke dag rond 13:00 opgehaald kunnen worden, zullen net na dat moment er nog 35 te verstrijken uren zijn. Dan zijn er dus 34 combinaties te berekenen zijn voor 2 uur, t/m 8 uur zijn dat 33 + 32 + 31 + 30 + 29 + 28 = 217 combinaties.

Dat maakt eveneens (34*1) + (33*2) + (32*3) + (31*4) + (30*5) + (29*6) + (28*7) = 840 loops om alles op voorhand te berekenen, los van de rest van de code. Is dat dan niet veel overhead om het allemaal voorhanden te hebben?

Hoeveel loop-iteraties maak je nu dan?

[...]

Sterker nog, nadat ik er niet kwam met for (let x in array) loops (ik kreeg in de loop niet bij elke regel code het voor elkaar om property's tevoorschijn te halen) ging ik hier op over. Tot ik in mijn zoektocht op internet vond dat deze erg inefficient zijn. Kan zelfs een factor 20 schelen! Toen besloot ik weer terug te krijgen op een for loop maar dan deze variant en daarmee kreeg ik het ook vrij snel werkend.

Let wel dat dat stack-overflow topic al 8 jaar oud is. Er is sindsdien veel geoptimaliseerd in JS-engines.

[martin_v_z]

Als je voor leesbaarheid gaat. Gooi die nested loop dan gewoon in een aparte functie. Verder is arrayCount een rare variabel naam voor wat je als index gebruikt.

Niks mis trouwens met een for loop. Ik vind het zelf er niet minder leesbaar op worden.

Comments zoals deze, zijn onnodig, je zegt namelijk exact hetzelfde als de regel er onder:
//Add 3600 seconds for every extra consecutive hour
epochSecondsToAdd = consecutiveHourCount * 3600

[CH4OS]

Als je goede leesbaarheid wilt, zou je eventueel een linter daarover los kunnen laten, die geef je dan een set regels mee waar het op moet letten. Zo kun je het dus forceren. Zelf vind ik het ook wel wat raar lezen om de drie parameters van de for loops op een aparte regel te zetten. Maar dat zal denk ik een voorkeur voor een ieder zijn, vrees ik.

[JT]

crisp schreef op vrijdag 23 mei 2025 @ 22:04:
[...]

Heb je daar een concreet voorbeeld van?

Waarvan exact, de bron en het eindresultaat?

[...]

Hoeveel loop-iteraties maak je nu dan?

Nu doe ik het dus voor 1 specifiek blok aan uren. De loops gaan over de uren die komen ná het eerste uur. Want ik loop gewoon over elk uur van de price_array en bereken het gemiddelde van dat uur + het gespecificeerde aantal uren. Dus in het meest gunstige geval 34*1 = 34 loops, in het minst gunstige geval 28*7 = 196.

Een andere overweging zou kunnen zijn om wél elke dag de goedkoopste uurblokken te berekenen maar dan moet ik uitzoeken hoe dat in Home Assistant kan en werkt met niet het risico maar feit van scope creep Terwijl ik nog veel te doen heb.

[...]

Let wel dat dat stack-overflow topic al 8 jaar oud is. Er is sindsdien veel geoptimaliseerd in JS-engines.

Goed punt! Ik had het nog op een aantal andere plekken gelezen, die zijn wel iets recenter. Bijvoorbeeld deze uit 2023. Daar is het verschil wel kleiner maar imho nog significant. Bij deze uit 2025 zie ik bijna een factor 3 verschil.

[JT]

martin_v_z schreef op vrijdag 23 mei 2025 @ 22:14:
Als je voor leesbaarheid gaat. Gooi die nested loop dan gewoon in een aparte functie. Verder is arrayCount een rare variabel naam voor wat je als index gebruikt.

Niks mis trouwens met een for loop. Ik vind het zelf er niet minder leesbaar op worden.

Comments zoals deze, zijn onnodig, je zegt namelijk exact hetzelfde als de regel er onder:
//Add 3600 seconds for every extra consecutive hour
epochSecondsToAdd = consecutiveHourCount * 3600

Dank je, dit helpt mij wat kritischer te kijken.

[JT]

CH4OS schreef op vrijdag 23 mei 2025 @ 22:31:
Als je goede leesbaarheid wilt, zou je eventueel een linter daarover los kunnen laten, die geef je dan een set regels mee waar het op moet letten. Zo kun je het dus forceren. Zelf vind ik het ook wel wat raar lezen om de drie parameters van de for loops op een aparte regel te zetten. Maar dat zal denk ik een voorkeur voor een ieder zijn, vrees ik.

De drie parameters op een aparte regel is gedaan door een beautifier. Ik had er ook al mijn twijfels over. Met alles wat ik hier lees ga ik een dezer dagen nog wat sleutelen. Dit weekend ga ik eens zoeken wat een linter is.

[ Voor 3% gewijzigd door JT op 23-05-2025 23:07 ]

[Soultaker]

JT schreef op vrijdag 23 mei 2025 @ 21:10:
In reactie op crisp beschreef ik zojuist hoe deze array wordt opgebouwd. De volgorde wordt dus overgenomen uit de twee eerder genoemde arrays. Deze haal ik op bij een API. Nu lijkt het me logisch dat de volgorde hetzelfde blijft, maar als dit een keer niet gebeurt dan werkt mijn code niet meer.

Mijn punt is dat het er op lijkt dat je huidige code ook niet werkt als de uren niet aansluitend en gesorteerd zijn (dus uur 1, uur 2, uur 3, .. uur N, in die volgorde, zonder ontbrekende uren).

Dat leidt ik uit twee dingen af. Ten eerste deze lus hier:

JavaScript:

    for (
        var arrayCount = 0;
        arrayCount <= price_array.length - hoursInBlock;
        arrayCount++
    ) {

Zoals gezegd, arrayCount is eigenlijk je start-index. Hier neem je alle indices van 0 tot price_array.length - hoursInBlock (inclusief) in beschouwing. Dat is alleen correct als die laatste hoursInBlock - 1 elementen ook de laatste uren bevatten.

Het tweede is dit:

JavaScript:

            nextHour = price_array.findIndex(
                (object) =>
                    new Date(object.startsAt).getTime() == new Date(arrayHour.startsAt).getTime() + (epochSecondsToAdd * 1000)

Hier vind je een element als het bestaat. Maar als er een uur mist in je array, dan is nextHour undefined, en krijg je een exception op de volgende regel.

Volgens mij gaat je code dus de mist in als je b.v. de elementen random shufflet of van op aflopende tijd sorteert.

En wat bedoel je met de implementatie? Dat ik de code beter anders had kunnen schrijven of dat ik een ongelukkige methode gebruik ofzo?

Bijvoorbeeld dat het predicaat van findIndex() élke keer twee nieuwe Date objecten aanmaakt puur om de timestamps te kunnen vergelijken. Heel efficiënt is dat niet. Simpeler zou zijn om van te voren een index van start times te maken:

JavaScript:

const start_times = price_array.map(object => Date(object.startsAt).getTime());

en dan in de loop:

JavaScript:

nextHour = start_times.indexOf(start_times[arrayHour] + (epochSecondsToAdd * 1000));

... scheelt een heleboel constructies van Date objecten.

Verder is je algoritme momenteel O(n³) terwijl O(n) mogelijk is (of O(n log n) als je de array eerst nog moet sorteren). Daar valt veel meer te halen qua performance dan het micro-optimaliseren van for-loops vs forEach().

[JT]

Soultaker schreef op vrijdag 23 mei 2025 @ 23:07:
[...]

Mijn punt is dat het er op lijkt dat je huidige code ook niet werkt als de uren niet aansluitend en gesorteerd zijn (dus uur 1, uur 2, uur 3, .. uur N, in die volgorde, zonder ontbrekende uren).

Dat leidt ik uit twee dingen af. Ten eerste deze lus hier:

JavaScript:

    for (
        var arrayCount = 0;
        arrayCount <= price_array.length - hoursInBlock;
        arrayCount++
    ) {

Zoals gezegd, arrayCount is eigenlijk je start-index. Hier neem je alle indices van 0 tot price_array.length - hoursInBlock (inclusief) in beschouwing. Dat is alleen correct als die laatste hoursInBlock - 1 elementen ook de laatste uren bevatten.

Het tweede is dit:

JavaScript:

            nextHour = price_array.findIndex(
                (object) =>
                    new Date(object.startsAt).getTime() == new Date(arrayHour.startsAt).getTime() + (epochSecondsToAdd * 1000)

Hier vind je een element als het bestaat. Maar als er een uur mist in je array, dan is nextHour undefined, en krijg je een exception op de volgende regel.

Volgens mij gaat je code dus de mist in als je b.v. de elementen random shufflet of van op aflopende tijd sorteert.


[...]

Bijvoorbeeld dat het predicaat van findIndex() élke keer twee nieuwe Date objecten aanmaakt puur om de timestamps te kunnen vergelijken. Heel efficiënt is dat niet. Simpeler zou zijn om van te voren een index van start times te maken:

JavaScript:

const start_times = price_array.map(object => Date(object.startsAt).getTime());

en dan in de loop:

JavaScript:

nextHour = start_times.indexOf(start_times[arrayHour] + (epochSecondsToAdd * 1000));

... scheelt een heleboel constructies van Date objecten.

Verder is je algoritme momenteel O(n³) terwijl O(n) mogelijk is (of O(n log n) als je de array eerst nog moet sorteren). Daar valt veel meer te halen qua performance dan het micro-optimaliseren van for-loops vs forEach().

Interessant. Oogjes worden zwaar, ik ga er morgen eens goed naar kijken. Ik had niet verwacht dat de feedback op zo een klein stukje code zo leerzaam kon zijn (geldt voor alle comments!)

[JT]

Eindelijk even tijd gevonden om er goed voor te gaan zitten.

Soultaker schreef op vrijdag 23 mei 2025 @ 23:07:
[...]

Mijn punt is dat het er op lijkt dat je huidige code ook niet werkt als de uren niet aansluitend en gesorteerd zijn (dus uur 1, uur 2, uur 3, .. uur N, in die volgorde, zonder ontbrekende uren).

Dat leidt ik uit twee dingen af. Ten eerste deze lus hier:

JavaScript:

    for (
        var arrayCount = 0;
        arrayCount <= price_array.length - hoursInBlock;
        arrayCount++
    ) {

Zoals gezegd, arrayCount is eigenlijk je start-index. Hier neem je alle indices van 0 tot price_array.length - hoursInBlock (inclusief) in beschouwing. Dat is alleen correct als die laatste hoursInBlock - 1 elementen ook de laatste uren bevatten.

Het tweede is dit:

JavaScript:

            nextHour = price_array.findIndex(
                (object) =>
                    new Date(object.startsAt).getTime() == new Date(arrayHour.startsAt).getTime() + (epochSecondsToAdd * 1000)

Hier vind je een element als het bestaat. Maar als er een uur mist in je array, dan is nextHour undefined, en krijg je een exception op de volgende regel.

Volgens mij gaat je code dus de mist in als je b.v. de elementen random shufflet of van op aflopende tijd sorteert.

Ah ja, ik zie wat je bedoelt. Dus eigenlijk zeg je, maak een keuze: doe makkelijk en kies op basis van index in de aanname dat het op volgorde staat, waarmee ik de code zou kunnen versimpelen _of_ zorg dat je overal dit probleem voorkomt. Dat zou dan misschien kunnen (mijn invulling) door als findIndex niks vindt niks toe te voegen aan sumHourPrice en niet te delen door hoursInBlock maar door aantal daadwerkelijk gevonden uren. Alternatief om als niet hoursInBlock uren gevonden kunnen worden om niks terug te geven want het blok is niet op te bouwen. Dat is misschien nog wel beter.

De for-loop zou ik zou kunnen aanpassen dat alleen wordt berekend vanaf alle uren _voor_ 23 - hoursInBlock in plaats van vaste indices. Of logica om price_array oplopend op datetime te sorteren en dan kan ik de rest van de logica versimpelen. Daar neig ik dan nog het meest naar denk ik, aangezien price_array behoorlijk bepalend is in hoeveel en wat er gebeurt.

[...]

Bijvoorbeeld dat het predicaat van findIndex() élke keer twee nieuwe Date objecten aanmaakt puur om de timestamps te kunnen vergelijken. Heel efficiënt is dat niet. Simpeler zou zijn om van te voren een index van start times te maken:

JavaScript:

const start_times = price_array.map(object => Date(object.startsAt).getTime());

en dan in de loop:

JavaScript:

nextHour = start_times.indexOf(start_times[arrayHour] + (epochSecondsToAdd * 1000));

... scheelt een heleboel constructies van Date objecten.

Die start_times maak je dan aan per uur waar je de waardes voor berekent? En dat je dan een dergelijk array maakt met x waardes kost minder resources dan x keer de Date constructor gebruiken? Zoals ik het begrijp declareer je start_times dan bij

code:

 //For every hour in array
    for (
        var arrayCount = 0;
        arrayCount <= price_array.length - hoursInBlock;
        arrayCount++
    ) {
        sumHourPrices = 0
        arrayHour = price_array[arrayCount]

Ik zou het ook kunnen doen bij het stuk waar ik price_array maak, dan maak ik het een property van het object. Hoe kijk jij daar naar?

Verder is je algoritme momenteel O(n³) terwijl O(n) mogelijk is (of O(n log n) als je de array eerst nog moet sorteren). Daar valt veel meer te halen qua performance dan het micro-optimaliseren van for-loops vs forEach().

Ik moet m'n wiskunde-inzichten nog even afstoffen om dit te begrijpen, kom ik nog op terug.

Edit: oh ja, logaritme was nou niet perse heel moeilijk. Maar wat bedoel je met O, is dat elk uur in price_array? @Soultaker

[ Voor 3% gewijzigd door JT op 27-05-2025 16:45 ]

[Soultaker]

JT schreef op dinsdag 27 mei 2025 @ 13:51:
Ah ja, ik zie wat je bedoelt. Dus eigenlijk zeg je, maak een keuze: doe makkelijk en kies op basis van index in de aanname dat het op volgorde staat, waarmee ik de code zou kunnen versimpelen _of_ zorg dat je overal dit probleem voorkomt.

Ja inderdaad, maar ook: je moet weten of je invoer gestorteerd is of niet, en of je algoritme dat nodig heeft of niet. Nu zit je er een beetje tussenin: je code suggereert dat 'ie flexibel genoeg is om met een niet-gesorteerde array te werken, maar in werkelijkheid werkt het vermoedelijk niet.

Als je algoritme geen gesorteerde invoer vereist, en je invoer is wel gesorteerd, is er geen probleem.

Als je algoritme geen gesorteerde invoer vereist, en je invoer is niet gesorteerd, is er geen probleem.

Als je algoritme gesorteerde invoer vereist, en je invoer is wel gesorteerd, is er geen probleem.

Als je algoritme gesorteerde invoer vereist, en je invoer is niet (per se) gesorteerd, dan heb je een probleem. Volgens mij ben je nu hier Dan kun je het probleem op twee manieren oplossen: ofwel je verandert je algoritme zodat het geen gesorteerde invoer nodig heeft, ofwel je sorteert je invoer. Het tweede is eenvoudiger.

Dat zou dan misschien kunnen (mijn invulling) door als findIndex niks vindt niks toe te voegen aan sumHourPrice en niet te delen door hoursInBlock maar door aantal daadwerkelijk gevonden uren. Alternatief om als niet hoursInBlock uren gevonden kunnen worden om niks terug te geven want het blok is niet op te bouwen. Dat is misschien nog wel beter.

Dat is een business logic probleem, geen programmeerprobleem, dus daar kan ik je niet bij helpen

Of logica om price_array oplopend op datetime te sorteren en dan kan ik de rest van de logica versimpelen. Daar neig ik dan nog het meest naar denk ik, aangezien price_array behoorlijk bepalend is in hoeveel en wat er gebeurt.

Dat lijkt me sowieso een goed idee.

Edit: oh ja, logaritme was nou niet perse heel moeilijk. Maar wat bedoel je met O, is dat elk uur in price_array

Als ik het goed begrepen heb probeer je gegeven N prijzen per uur een blok van M aansluitende uren te vinden zodat de gemiddelde uurprijs minimaal is (wat natuurlijk hetzelfde is als: de totaalprijs voor de M uren in het blok is minimaal, immers het gemiddelde is gewoon het totaal gedeeld door M).

Als N de lengte van price_array is en M = hoursInBlock, dan is het aantal keer dat de binneste lus van je code uitgevoerd wordt proportioneel aan N² × M. Heel grofweg: het is een O(N²M) algoritme.

Terwijl het ook in O(N) tijd kan. Op deze manier bijvoorbeeld:

JavaScript:

function vind_beste_periode(uurprijzen, aantal_uren) {
  if (aantal_uren > uurprijzen.length) {
    throw new Error('Meer uren gevraagd dan beschikbaar!');
  }
  // Invariant: `totaal` is de som van de elementen in uurprijzen
  // van `begin` (inclusief) tot `einde` (exclusief).
  let begin  = 0;
  let einde  = 0;
  let totaal = 0;
  while (einde < aantal_uren) totaal += uurprijzen[einde++];
  let beste_begin  = begin;
  let beste_einde  = einde;
  let beste_totaal = totaal;
  while (einde < uurprijzen.length) {
    totaal = totaal - uurprijzen[begin++] + uurprijzen[einde++];
    if (totaal < beste_totaal) {
      beste_begin  = begin;
      beste_einde  = einde;
      beste_totaal = totaal;
    }
  }
  return [beste_begin, beste_einde, beste_totaal];
}

// Voorbeeld:
//                0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19
let uurprijzen = [3, 1, 5, 9, 2, 6, 5, 3, 5, 8, 9, 7, 9, 3, 2, 3, 8, 4, 6, 2];
let aantal_uren = 4;
let [van, tot, totaal] = vind_beste_periode(uurprijzen, aantal_uren);
console.log(`beste periode is van ${van} tot ${tot} (prijzen: ${uurprijzen.slice(van, tot)}) \
totaalprijs ${totaal} (gemiddelde: ${totaal / aantal_uren})`);

Met dit soort optimalisaties kun je vaak meer winnen dan met micro-optimalisaties zoals een forEach() lus vervangen door een for-lus.

[JT]

Soultaker schreef op dinsdag 27 mei 2025 @ 22:21:
[...]


Ja inderdaad, maar ook: je moet weten of je invoer gestorteerd is of niet, en of je algoritme dat nodig heeft of niet. Nu zit je er een beetje tussenin: je code suggereert dat 'ie flexibel genoeg is om met een niet-gesorteerde array te werken, maar in werkelijkheid werkt het vermoedelijk niet.

Als je algoritme geen gesorteerde invoer vereist, en je invoer is wel gesorteerd, is er geen probleem.

Als je algoritme geen gesorteerde invoer vereist, en je invoer is niet gesorteerd, is er geen probleem.

Als je algoritme gesorteerde invoer vereist, en je invoer is wel gesorteerd, is er geen probleem.

Als je algoritme gesorteerde invoer vereist, en je invoer is niet (per se) gesorteerd, dan heb je een probleem. Volgens mij ben je nu hier Dan kun je het probleem op twee manieren oplossen: ofwel je verandert je algoritme zodat het geen gesorteerde invoer nodig heeft, ofwel je sorteert je invoer. Het tweede is eenvoudiger.

Ik was er al mee begonnen

[...]

Dat is een business logic probleem, geen programmeerprobleem, dus daar kan ik je niet bij helpen

True. Ik retourneer dan gewoon niks.

[...]


Als ik het goed begrepen heb probeer je gegeven N prijzen per uur een blok van M aansluitende uren te vinden zodat de gemiddelde uurprijs minimaal is (wat natuurlijk hetzelfde is als: de totaalprijs voor de M uren in het blok is minimaal, immers het gemiddelde is gewoon het totaal gedeeld door M).

Als N de lengte van price_array is en M = hoursInBlock, dan is het aantal keer dat de binneste lus van je code uitgevoerd wordt proportioneel aan N² × M. Heel grofweg: het is een O(N²M) algoritme.

Terwijl het ook in O(N) tijd kan. Op deze manier bijvoorbeeld:

JavaScript:

function vind_beste_periode(uurprijzen, aantal_uren) {
  if (aantal_uren > uurprijzen.length) {
    throw new Error('Meer uren gevraagd dan beschikbaar!');
  }
  // Invariant: `totaal` is de som van de elementen in uurprijzen
  // van `begin` (inclusief) tot `einde` (exclusief).
  let begin  = 0;
  let einde  = 0;
  let totaal = 0;
  while (einde < aantal_uren) totaal += uurprijzen[einde++];
  let beste_begin  = begin;
  let beste_einde  = einde;
  let beste_totaal = totaal;
  while (einde < uurprijzen.length) {
    totaal = totaal - uurprijzen[begin++] + uurprijzen[einde++];
    if (totaal < beste_totaal) {
      beste_begin  = begin;
      beste_einde  = einde;
      beste_totaal = totaal;
    }
  }
  return [beste_begin, beste_einde, beste_totaal];
}

// Voorbeeld:
//                0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19
let uurprijzen = [3, 1, 5, 9, 2, 6, 5, 3, 5, 8, 9, 7, 9, 3, 2, 3, 8, 4, 6, 2];
let aantal_uren = 4;
let [van, tot, totaal] = vind_beste_periode(uurprijzen, aantal_uren);
console.log(`beste periode is van ${van} tot ${tot} (prijzen: ${uurprijzen.slice(van, tot)}) \
totaalprijs ${totaal} (gemiddelde: ${totaal / aantal_uren})`);

Met dit soort optimalisaties kun je vaak meer winnen dan met micro-optimalisaties zoals een forEach() lus vervangen door een for-lus.

Ik ben ook maar een beginner maar ik krijg die eerste while niet ontcijferd, ik heb het idee dat er daar wellicht iets niet helemaal goed is gegaan met typen? Moet het zijn

code:

function vind_beste_periode(uurprijzen, aantal_uren) {
  if (aantal_uren > uurprijzen.length) {
    throw new Error('Meer uren gevraagd dan beschikbaar!');
  }
  // Invariant: `totaal` is de som van de elementen in uurprijzen
  // van `begin` (inclusief) tot `einde` (exclusief).
  let begin  = 0;
  let einde  = 0;
  let totaal = 0;
  while (einde < aantal_uren) {
    totaal += uurprijzen[einde++];
    let beste_begin  = begin;
    let beste_einde  = einde;
    let beste_totaal = totaal;
  while (einde < uurprijzen.length) {
    totaal = totaal - uurprijzen[begin++] + uurprijzen[einde++];
    if (totaal < beste_totaal) {
      beste_begin  = begin;
      beste_einde  = einde;
      beste_totaal = totaal;
    }
  }
  return [beste_begin, beste_einde, beste_totaal];
}

[ Voor 10% gewijzigd door JT op 28-05-2025 21:35 ]

[Ed Vertijsment]

Ten eerste top dat je het probleem identificeert en wil oplossen.

Ik wou hier inhoudelijk op reageren met de editor en wat refactors maar raak in de war vanwege missende context (price_array) . Dus ik doe het anders:

JT schreef op vrijdag 23 mei 2025 @ 13:44:
Wellicht door de vele losse comments (juist een poging tot documenteren)

In JavaScript is er een conventie genaamd JSDoc, veel editors zullen dit ook kunnen generen. Het toevoegen van docstrings, zelfs al is dit zonder nuttige inhoud kan de leesbaarheid al verbeteren:

2 plaatjes:

Geen docstrings	Met docstrings

Dit zijn abstracties van dezelfde code, een met, een zonder JSDoc strings. De linker is korter, en je zou kunnen zeggen daarom beter maar het is rechts veel makkelijker individuele delen te identificeren door de syntax highlighting van de docstrings. Dit heet "chunking", en maakt vaak code beter werkbaar.

Verder kan je in je docstrings duidelijk beschrijven wat een functie écht moet doen, en kan het je zo helpen om de scope te bepalen (en waar je wilt splitten naar een nieuwe functie).

Inline comments zijn verder prima, maar ik zie vooral waarde in goede docstrings.

JT schreef op vrijdag 23 mei 2025 @ 13:44:
Is het voor de leesbaarheid handiger dat ik de diepste loop eruit haal en in een eigen functie stop?

Ja, de nested loop is vaak geindent en indents zijn lastig te lezen, aldus ESLint.

Many developers consider code difficult to read if blocks are nested beyond a certain depth. - ESLint

Verder zijn de losse loops waarschijnlijk complexiteit die een andere vraag beantwoorden, ik denk altijd zo.

code:

function vraagDieIkKanStellen() {
  return antwoordOpVraag;
}
const antwoordOpVraag = vraagDieIkKanStellen();

Ding is hier dat ik functie beschouw als vragen die te stellen zijn en dat de return value een antwoord moet zijn op die vraag. Als je verschillende deelvragen kan identificieren kan je ook deelfuncties identificeren.

Verder raad ik je aan om te kijken aar Array methods (filter, find, map, reduce), minder performant dan ruwe foor loops (echter verschillen zeer klein) maar vaak ook leesbaarder. Ook kun je volgens mij bepaalde constructies herschrijven in een [or of loop

[Soultaker]

JT schreef op woensdag 28 mei 2025 @ 20:44:
Ik ben ook maar een beginner maar ik krijg die eerste while niet ontcijferd, ik heb het idee dat er daar wellicht iets niet helemaal goed is gegaan met typen? Moet het zijn [..]

Mijn aanname was dat je op zoek bent naar een blok van precies aantal_uren uren, en niet minder dan dat.

De eerste while-lus berekent de som van de eerste aantal_uren uren, en de tweede while lus schuift begin/eind steeds 1 keer op.

Voor de volledigheid een voorbeeld met extra debug-informatie:

JavaScript:

function vind_beste_periode(uurprijzen, aantal_uren) {
  if (aantal_uren > uurprijzen.length) {
    throw new Error('Meer uren gevraagd dan beschikbaar!');
  }
  // Invariant: `totaal` is de som van de elementen in uurprijzen
  // van `begin` (inclusief) tot `einde` (exclusief).
  let begin = 0;
  let einde = 0;
  let totaal = 0;
  while (einde < aantal_uren) {
    totaal += uurprijzen[einde++];
    DEBUG_PRINT();
  }
  let beste_begin = begin;
  let beste_einde = einde;
  let beste_totaal = totaal;
  while (einde < uurprijzen.length) {
    totaal = totaal - uurprijzen[begin++] + uurprijzen[einde++];
    DEBUG_PRINT();
    if (totaal < beste_totaal) {
      beste_totaal = totaal;
      beste_begin = begin;
      beste_einde = einde;
    }
  }
  return [beste_begin, beste_einde, beste_totaal];

  function DEBUG_PRINT() {
    console.log(
        ...uurprijzen.map((p, i) => `${i == begin ? '[' : ' '}${p}${i + 1 == einde ? ']' : ' '}`),
        `begin=${begin} einde=${einde} lengte=${einde - begin} totaal=${totaal}`);
  }
}

// Voorbeeld:
//                0  1  2  3  4  5  6  7  8  9
let uurprijzen = [3, 1, 5, 9, 2, 6, 5, 3, 5, 8];
let aantal_uren = 4;
let [van, tot, totaal] = vind_beste_periode(uurprijzen, aantal_uren);
console.log(`beste periode is van ${van} tot ${tot} (prijzen: ${uurprijzen.slice(van, tot)}) \
totaalprijs ${totaal} (gemiddelde: ${totaal / aantal_uren})`);

Uitvoer:

[3]  1   5   9   2   6   5   3   5   8  begin=0 einde=1 lengte=1 totaal=3
[3   1]  5   9   2   6   5   3   5   8  begin=0 einde=2 lengte=2 totaal=4
[3   1   5]  9   2   6   5   3   5   8  begin=0 einde=3 lengte=3 totaal=9
[3   1   5   9]  2   6   5   3   5   8  begin=0 einde=4 lengte=4 totaal=18
 3  [1   5   9   2]  6   5   3   5   8  begin=1 einde=5 lengte=4 totaal=17
 3   1  [5   9   2   6]  5   3   5   8  begin=2 einde=6 lengte=4 totaal=22
 3   1   5  [9   2   6   5]  3   5   8  begin=3 einde=7 lengte=4 totaal=22
 3   1   5   9  [2   6   5   3]  5   8  begin=4 einde=8 lengte=4 totaal=16
 3   1   5   9   2  [6   5   3   5]  8  begin=5 einde=9 lengte=4 totaal=19
 3   1   5   9   2   6  [5   3   5   8] begin=6 einde=10 lengte=4 totaal=21
beste periode is van 4 tot 8 (prijzen: 2,6,5,3) totaalprijs 16 (gemiddelde: 4)

[didrix]

Weet je zeker dat de code doet wat je denkt/wil dat het doet? Volgens mij wil je dit.
Als:

JavaScript:

price_array = [
    { startsAt: "2025-05-31 00:00:00.000", total: 80 },
    { startsAt: "2025-05-31 01:00:00.000", total: 80 },
    { startsAt: "2025-05-31 02:00:00.000", total: 10 },
    { startsAt: "2025-05-31 03:00:00.000", total: 80 },
    { startsAt: "2025-05-31 04:00:00.000", total: 80 },
]

Verwacht je

JavaScript:

> findCheapestHours(1) 
{ startsAt: "2025-05-31 02:00:00.000", total: 10 }

Dit kan je automatiseren. Dat wil zeggen; tests schrijven.

JavaScript:

import { expect, test } from "vitest";
import HourOptimizer from "./houroptimizer.js";

test("can identify cheapest hour", () => {
  const optimizer = new HourOptimizer([
    { startsAt: "2025-05-31 00:00:00.000", total: 80 },
    { startsAt: "2025-05-31 01:00:00.000", total: 80 },
    { startsAt: "2025-05-31 02:00:00.000", total: 10 },
    { startsAt: "2025-05-31 03:00:00.000", total: 80 },
    { startsAt: "2025-05-31 04:00:00.000", total: 80 },
  ]);
  expect(optimizer.findCheapestHours(1)).toStrictEqual({
    startsAt: "2025-05-31 02:00:00.000",
    total: 10,
  });
});

test("can identify cheapest block at beginning", () => {
  const optimizer = new HourOptimizer([
    { startsAt: "2025-05-31 00:00:00.000", total: 10 },
    { startsAt: "2025-05-31 01:00:00.000", total: 80 },
    { startsAt: "2025-05-31 02:00:00.000", total: 80 },
    { startsAt: "2025-05-31 03:00:00.000", total: 80 },
    { startsAt: "2025-05-31 04:00:00.000", total: 80 },
  ]);
  expect(optimizer.findCheapestHours(4)).toStrictEqual({
    startsAt: "2025-05-31 00:00:00.000",
    total: 10,
  });
});

test("can identify cheapest block at end", () => {
  const optimizer = new HourOptimizer([
    { startsAt: "2025-05-31 00:00:00.000", total: 80 },
    { startsAt: "2025-05-31 01:00:00.000", total: 80 },
    { startsAt: "2025-05-31 02:00:00.000", total: 80 },
    { startsAt: "2025-05-31 03:00:00.000", total: 80 },
    { startsAt: "2025-05-31 04:00:00.000", total: 10 },
  ]);
  expect(optimizer.findCheapestHours(4)).toStrictEqual({
    startsAt: "2025-05-31 01:00:00.000",
    total: 80,
  });
});

test("can identify cheapest block is not cheapest hour", () => {
  const optimizer = new HourOptimizer([
    { startsAt: "2025-05-31 00:00:00.000", total: 10 },
    { startsAt: "2025-05-31 01:00:00.000", total: 80 },
    { startsAt: "2025-05-31 02:00:00.000", total: 80 },
    { startsAt: "2025-05-31 03:00:00.000", total: 20 },
    { startsAt: "2025-05-31 04:00:00.000", total: 20 },
  ]);
  expect(optimizer.findCheapestHours(3)).toStrictEqual({
    startsAt: "2025-05-31 02:00:00.000",
    total: 80,
  });
});

Om je code te testen heb ik je functie in een class gewrapped, omdat je een globale variabele (price_array) gebruikt en dat maakt de code ontestbaar. Maar als ik je code uitvoer gaat de laatste test fout. Die geeft terug;

JavaScript:

{ startsAt: "2025-05-31 00:00:00.000", total: 10 }

Hier lijkt de bug te zitten;

JavaScript:

if (
  bestPricedBlockStart == null || calculatedPrice < bestPricedBlockStart.total
) {
  //Store current hour as best priced to start action
  bestPricedBlockStart = arrayHour
}

Je vergelijk het berekende gemiddelde prijs met de prijs van het eerste uur van het tot dan toe "goedkoopste" blok. Je moet hier vergelijken met de gemiddelde prijs van het goedkoopste blok, niet de prijs van het eerste uur.

Maar met tests kun je nu eenvoudig fixen. En je kan direct verder met het leesbaar maken van de code, zonder bang te zijn het kapot te maken. Dit doe je stapgewijs. Dit proces heet "refactoring".

Na een snelle refactoring krijg ik zoiets;

JavaScript:

class HourOptimizer {
  constructor(priceArray) {
    this.priceArray = priceArray;
  }

  findCheapestHours(hoursInBlock) {
    const startingHours =
      hoursInBlock == 1
        ? this.priceArray
        : this.priceArray.slice(0, -(hoursInBlock - 1));

    const possibleBlocks = startingHours.map((startingHour, startingIndex) => ({
      startingHour,
      blockTotal: this.calculateBlockPrice(startingIndex, hoursInBlock),
    }));

    const lowestCostBlock = possibleBlocks.reduce((currentCheapest, block) =>
      block.blockTotal < currentCheapest.blockTotal ? block : currentCheapest,
    );
    return lowestCostBlock.startingHour;
  }

  calculateBlockPrice(fromIndex, hoursInBlock) {
    const currentHourPrice = this.priceArray[fromIndex].total;

    return hoursInBlock == 1
      ? currentHourPrice
      : currentHourPrice +
          this.calculateBlockPrice(fromIndex + 1, hoursInBlock - 1);
  }
}

export default HourOptimizer;

Verder zijn er nog een paar andere verbeterpunten die meer te maken hebben met de interface dan de implementatie;

• De gekozen functienaam findCheapestHours representeert niet het teruggegeven resultaat. De naam doet vermoeden dat je de x goedkoopste uren terugkrijgt, maar je krijgt alleen het eerste uur terug van een blok van x aaneengesloten uren die het goedkoopste is. Misschien is een betere naam findCheapestStartingHour().
• de price_array wordt geacht een array te zijn van aaneengesloten hele uren. Als een uur niet gedefinieerd is gaat je code stuk. Mijn refactoring zou hier niet stuk gaan, maar vereist dan weer dat de price_array gesorteerd is. Er is nog ruimte voor verbetering om de price_array datastructuur beter te definiëren.
• Wellicht wil je valideren dat de hoursInBlock parameter een integer is van tenminste 1.

[JT]

Allen, wederom bedankt. Sommige zaken moet ik echt even mijn hoofd flink voor laten werken. Dat vind ik leuk, maar kost tijd. Ik heb nu ook weer alleen al een uur uitgetrokken om de posts allemaal 100% te snappen, te doorgronden en te antwoorden, al dan niet nadat ik er iets mee heb gedaan. Voor mij is bijvoorbeeld ook nieuw dat je de increment operator kunt gebruiken op een variabele die je tegelijkertijd als index in een array gebruikt

Ed Vertijsment schreef op woensdag 28 mei 2025 @ 22:36:
Ten eerste top dat je het probleem identificeert en wil oplossen.

Ik wou hier inhoudelijk op reageren met de editor en wat refactors maar raak in de war vanwege missende context (price_array) .

Ik haal data op van Tibber. De objecten (uren) van today en tomorrow voeg ik samen in price_array afhankelijk van de wensen (resulteert in maximale eindtijd) en reeds gepasseerde uren komen er ook niet in (dat geeft de begintijd).

Dus ik doe het anders:


[...]


In JavaScript is er een conventie genaamd JSDoc, veel editors zullen dit ook kunnen generen. Het toevoegen van docstrings, zelfs al is dit zonder nuttige inhoud kan de leesbaarheid al verbeteren:

2 plaatjes:

Geen docstrings	Met docstrings
[Afbeelding]	[Afbeelding]

Dit zijn abstracties van dezelfde code, een met, een zonder JSDoc strings. De linker is korter, en je zou kunnen zeggen daarom beter maar het is rechts veel makkelijker individuele delen te identificeren door de syntax highlighting van de docstrings. Dit heet "chunking", en maakt vaak code beter werkbaar.

Verder kan je in je docstrings duidelijk beschrijven wat een functie écht moet doen, en kan het je zo helpen om de scope te bepalen (en waar je wilt splitten naar een nieuwe functie).

Inline comments zijn verder prima, maar ik zie vooral waarde in goede docstrings.

Als ik dat plaatje zo zie dan denk ik dat ik beter minder inline comments kan toevoegen en meer docstrings om zo de toelichting te concentreren en zoals jij beschrijft duidelijk chunks te krijgen, duidelijke secties.

[...]


Ja, de nested loop is vaak geindent en indents zijn lastig te lezen, aldus ESLint.

Many developers consider code difficult to read if blocks are nested beyond a certain depth. - ESLint

Ik ga eerst eens refactoren op basis van alles wat ik leer van deze mooie comments en dan ga ik eens "linten" zoals @CH4OS suggereerde

Verder zijn de losse loops waarschijnlijk complexiteit die een andere vraag beantwoorden, ik denk altijd zo.

code:

function vraagDieIkKanStellen() {
  return antwoordOpVraag;
}
const antwoordOpVraag = vraagDieIkKanStellen();

Ding is hier dat ik functie beschouw als vragen die te stellen zijn en dat de return value een antwoord moet zijn op die vraag. Als je verschillende deelvragen kan identificieren kan je ook deelfuncties identificeren.

Ook daar ga ik naar kijken, of het niet te diep wordt.

Verder raad ik je aan om te kijken aar Array methods (filter, find, map, reduce), minder performant dan ruwe foor loops (echter verschillen zeer klein) maar vaak ook leesbaarder. Ook kun je volgens mij bepaalde constructies herschrijven in een [or of loop

Ik ga eens kijken of ik die methodes kan gebruiken, ook bij het opbouwen van price_array vanuit 2 arrays.

Soultaker schreef op donderdag 29 mei 2025 @ 16:54:
[...]

Mijn aanname was dat je op zoek bent naar een blok van precies aantal_uren uren, en niet minder dan dat.

De eerste while-lus berekent de som van de eerste aantal_uren uren, en de tweede while lus schuift begin/eind steeds 1 keer op.

Voor de volledigheid een voorbeeld met extra debug-informatie:

JavaScript:

function vind_beste_periode(uurprijzen, aantal_uren) {
  if (aantal_uren > uurprijzen.length) {
    throw new Error('Meer uren gevraagd dan beschikbaar!');
  }
  // Invariant: `totaal` is de som van de elementen in uurprijzen
  // van `begin` (inclusief) tot `einde` (exclusief).
  let begin = 0;
  let einde = 0;
  let totaal = 0;
  while (einde < aantal_uren) {
    totaal += uurprijzen[einde++];
    DEBUG_PRINT();
  }
  let beste_begin = begin;
  let beste_einde = einde;
  let beste_totaal = totaal;
  while (einde < uurprijzen.length) {
    totaal = totaal - uurprijzen[begin++] + uurprijzen[einde++];
    DEBUG_PRINT();
    if (totaal < beste_totaal) {
      beste_totaal = totaal;
      beste_begin = begin;
      beste_einde = einde;
    }
  }
  return [beste_begin, beste_einde, beste_totaal];

  function DEBUG_PRINT() {
    console.log(
        ...uurprijzen.map((p, i) => `${i == begin ? '[' : ' '}${p}${i + 1 == einde ? ']' : ' '}`),
        `begin=${begin} einde=${einde} lengte=${einde - begin} totaal=${totaal}`);
  }
}

// Voorbeeld:
//                0  1  2  3  4  5  6  7  8  9
let uurprijzen = [3, 1, 5, 9, 2, 6, 5, 3, 5, 8];
let aantal_uren = 4;
let [van, tot, totaal] = vind_beste_periode(uurprijzen, aantal_uren);
console.log(`beste periode is van ${van} tot ${tot} (prijzen: ${uurprijzen.slice(van, tot)}) \
totaalprijs ${totaal} (gemiddelde: ${totaal / aantal_uren})`);

Uitvoer:

[3]  1   5   9   2   6   5   3   5   8  begin=0 einde=1 lengte=1 totaal=3
[3   1]  5   9   2   6   5   3   5   8  begin=0 einde=2 lengte=2 totaal=4
[3   1   5]  9   2   6   5   3   5   8  begin=0 einde=3 lengte=3 totaal=9
[3   1   5   9]  2   6   5   3   5   8  begin=0 einde=4 lengte=4 totaal=18
 3  [1   5   9   2]  6   5   3   5   8  begin=1 einde=5 lengte=4 totaal=17
 3   1  [5   9   2   6]  5   3   5   8  begin=2 einde=6 lengte=4 totaal=22
 3   1   5  [9   2   6   5]  3   5   8  begin=3 einde=7 lengte=4 totaal=22
 3   1   5   9  [2   6   5   3]  5   8  begin=4 einde=8 lengte=4 totaal=16
 3   1   5   9   2  [6   5   3   5]  8  begin=5 einde=9 lengte=4 totaal=19
 3   1   5   9   2   6  [5   3   5   8] begin=6 einde=10 lengte=4 totaal=21
beste periode is van 4 tot 8 (prijzen: 2,6,5,3) totaalprijs 16 (gemiddelde: 4)

Ah ja, nu snap ik het. Ik meende al te zien dat op regel 10 van je vorige stukje code wat mist maar ik twijfelde ook of ik nu iets ongelooflijk over het hoofd zag. Met jouw bevestiging door dit nieuwe voorbeeld en het output-voorbeeld snap ik het nu. De eerst while berekent eigenlijk effectief de blokken minder groot dan aantal_uren. Maar laat ik dat net niet nodig hebben. Dan zou ik uit kunnen komen op:

JavaScript:

function vind_beste_periode(uurprijzen, aantal_uren) {
  if (aantal_uren > uurprijzen.length) {
    throw new Error('Meer uren gevraagd dan beschikbaar!');
  }
  // Invariant: `totaal` is de som van de elementen in uurprijzen
  // van `begin` (inclusief) tot `einde` (exclusief).
  let beste_begin = 0;
  let beste_einde = aantal_uren;
  let beste_totaal;
  while (einde < uurprijzen.length && einde >= aantal_uren ) {
    totaal = totaal - uurprijzen[begin++] + uurprijzen[einde++];
    DEBUG_PRINT();
    if (totaal < beste_totaal) {
      beste_totaal = totaal;
      beste_begin = begin;
      beste_einde = einde;
    }
  }
  return [beste_begin, beste_einde, beste_totaal];

  function DEBUG_PRINT() {
    console.log(
        ...uurprijzen.map((p, i) => `${i == begin ? '[' : ' '}${p}${i + 1 == einde ? ']' : ' '}`),
        `begin=${begin} einde=${einde} lengte=${einde - begin} totaal=${totaal}`);
  }
}

// Voorbeeld:
//                0  1  2  3  4  5  6  7  8  9
let uurprijzen = [3, 1, 5, 9, 2, 6, 5, 3, 5, 8];
let aantal_uren = 4;
let [van, tot, totaal] = vind_beste_periode(uurprijzen, aantal_uren);
console.log(`beste periode is van ${van} tot ${tot} (prijzen: ${uurprijzen.slice(van, tot)}) \
totaalprijs ${totaal} (gemiddelde: ${totaal / aantal_uren})`);

Of zie ik iets over het hoofd?

didrix schreef op zaterdag 31 mei 2025 @ 14:29:
Weet je zeker dat de code doet wat je denkt/wil dat het doet? Volgens mij wil je dit.
Als:

JavaScript:

price_array = [
    { startsAt: "2025-05-31 00:00:00.000", total: 80 },
    { startsAt: "2025-05-31 01:00:00.000", total: 80 },
    { startsAt: "2025-05-31 02:00:00.000", total: 10 },
    { startsAt: "2025-05-31 03:00:00.000", total: 80 },
    { startsAt: "2025-05-31 04:00:00.000", total: 80 },
]

Verwacht je

JavaScript:

> findCheapestHours(1) 
{ startsAt: "2025-05-31 02:00:00.000", total: 10 }

Dit kan je automatiseren. Dat wil zeggen; tests schrijven.

JavaScript:

import { expect, test } from "vitest";
import HourOptimizer from "./houroptimizer.js";

test("can identify cheapest hour", () => {
  const optimizer = new HourOptimizer([
    { startsAt: "2025-05-31 00:00:00.000", total: 80 },
    { startsAt: "2025-05-31 01:00:00.000", total: 80 },
    { startsAt: "2025-05-31 02:00:00.000", total: 10 },
    { startsAt: "2025-05-31 03:00:00.000", total: 80 },
    { startsAt: "2025-05-31 04:00:00.000", total: 80 },
  ]);
  expect(optimizer.findCheapestHours(1)).toStrictEqual({
    startsAt: "2025-05-31 02:00:00.000",
    total: 10,
  });
});

test("can identify cheapest block at beginning", () => {
  const optimizer = new HourOptimizer([
    { startsAt: "2025-05-31 00:00:00.000", total: 10 },
    { startsAt: "2025-05-31 01:00:00.000", total: 80 },
    { startsAt: "2025-05-31 02:00:00.000", total: 80 },
    { startsAt: "2025-05-31 03:00:00.000", total: 80 },
    { startsAt: "2025-05-31 04:00:00.000", total: 80 },
  ]);
  expect(optimizer.findCheapestHours(4)).toStrictEqual({
    startsAt: "2025-05-31 00:00:00.000",
    total: 10,
  });
});

test("can identify cheapest block at end", () => {
  const optimizer = new HourOptimizer([
    { startsAt: "2025-05-31 00:00:00.000", total: 80 },
    { startsAt: "2025-05-31 01:00:00.000", total: 80 },
    { startsAt: "2025-05-31 02:00:00.000", total: 80 },
    { startsAt: "2025-05-31 03:00:00.000", total: 80 },
    { startsAt: "2025-05-31 04:00:00.000", total: 10 },
  ]);
  expect(optimizer.findCheapestHours(4)).toStrictEqual({
    startsAt: "2025-05-31 01:00:00.000",
    total: 80,
  });
});

test("can identify cheapest block is not cheapest hour", () => {
  const optimizer = new HourOptimizer([
    { startsAt: "2025-05-31 00:00:00.000", total: 10 },
    { startsAt: "2025-05-31 01:00:00.000", total: 80 },
    { startsAt: "2025-05-31 02:00:00.000", total: 80 },
    { startsAt: "2025-05-31 03:00:00.000", total: 20 },
    { startsAt: "2025-05-31 04:00:00.000", total: 20 },
  ]);
  expect(optimizer.findCheapestHours(3)).toStrictEqual({
    startsAt: "2025-05-31 02:00:00.000",
    total: 80,
  });
});

Om je code te testen heb ik je functie in een class gewrapped, omdat je een globale variabele (price_array) gebruikt en dat maakt de code ontestbaar. Maar als ik je code uitvoer gaat de laatste test fout. Die geeft terug;

JavaScript:

{ startsAt: "2025-05-31 00:00:00.000", total: 10 }

Hier lijkt de bug te zitten;

JavaScript:

if (
  bestPricedBlockStart == null || calculatedPrice < bestPricedBlockStart.total
) {
  //Store current hour as best priced to start action
  bestPricedBlockStart = arrayHour
}

Je vergelijk het berekende gemiddelde prijs met de prijs van het eerste uur van het tot dan toe "goedkoopste" blok. Je moet hier vergelijken met de gemiddelde prijs van het goedkoopste blok, niet de prijs van het eerste uur.

Maar met tests kun je nu eenvoudig fixen. En je kan direct verder met het leesbaar maken van de code, zonder bang te zijn het kapot te maken.

Wat tof! Ik heb nooit in m'n werk veel meegekregen van unit testing (wel het schrijven van code zelf), erg interessant om dit nu eens te zien. En ik zie inderdaad de fout.

Dit doe je stapgewijs. Dit proces heet "refactoring".

Na een snelle refactoring krijg ik zoiets;

JavaScript:

class HourOptimizer {
  constructor(priceArray) {
    this.priceArray = priceArray;
  }

  findCheapestHours(hoursInBlock) {
    const startingHours =
      hoursInBlock == 1
        ? this.priceArray
        : this.priceArray.slice(0, -(hoursInBlock - 1));

    const possibleBlocks = startingHours.map((startingHour, startingIndex) => ({
      startingHour,
      blockTotal: this.calculateBlockPrice(startingIndex, hoursInBlock),
    }));

    const lowestCostBlock = possibleBlocks.reduce((currentCheapest, block) =>
      block.blockTotal < currentCheapest.blockTotal ? block : currentCheapest,
    );
    return lowestCostBlock.startingHour;
  }

  calculateBlockPrice(fromIndex, hoursInBlock) {
    const currentHourPrice = this.priceArray[fromIndex].total;

    return hoursInBlock == 1
      ? currentHourPrice
      : currentHourPrice +
          this.calculateBlockPrice(fromIndex + 1, hoursInBlock - 1);
  }
}

export default HourOptimizer;

Deze ga ik eens ontleden aan de hand van de array methodes. En dan naast alle andere feedback leggen, bedankt.

Verder zijn er nog een paar andere verbeterpunten die meer te maken hebben met de interface dan de implementatie;

• De gekozen functienaam findCheapestHours representeert niet het teruggegeven resultaat. De naam doet vermoeden dat je de x goedkoopste uren terugkrijgt, maar je krijgt alleen het eerste uur terug van een blok van x aaneengesloten uren die het goedkoopste is. Misschien is een betere naam findCheapestStartingHour().
• de price_array wordt geacht een array te zijn van aaneengesloten hele uren. Als een uur niet gedefinieerd is gaat je code stuk. Mijn refactoring zou hier niet stuk gaan, maar vereist dan weer dat de price_array gesorteerd is. Er is nog ruimte voor verbetering om de price_array datastructuur beter te definiëren.
• Wellicht wil je valideren dat de hoursInBlock parameter een integer is van tenminste 1.

Nuttige punten, ik neem ze mee. Die middelste heb ik al opgelost door de array te sorteren met sort().

[Soultaker]

JT schreef op woensdag 4 juni 2025 @ 12:38:
De eerst while berekent eigenlijk effectief de blokken minder groot dan aantal_uren. Maar laat ik dat net niet nodig hebben. Dan zou ik uit kunnen komen op:

JavaScript:

[..]

Of zie ik iets over het hoofd?

Volgens mij heb je iets te veel weggehaald, maar daar kom je zelf wel achter als je het uitprobeert.

Ik denk dat je eerst zelf met je code aan de slag moet. Als je nu eerst een paar unit tests schrijft, kun je de meeste problemen hopelijk zelf oplossen. Als je er dan nog niet uitkomt kun je weer om hulp vragen, maar dan is het in ieder geval duidelijk wat de functie zou moeten doen.