Feedback på kod i kmom02, 4.2 i lab2 och A5 i marvin

1 kmom02 4.2 lab2 och A5 marvin.md

En till student har kontaktat mig om feedback och gav mig tillåtelse att dela med mig av kod och kommentar. Det blev mycket feedback på labbuppgiften 4.2. Koden såg OK ut och studenten använda alla konstruktioner korrekt. Men koden kunder göras om för att bli kortare och på så sätt även exekvera snabbare. Jag baserar feedbacken utifrån hur snabbt koden exekverar, det är egentligen inget vi fokuserar på i kursen men det är samtidigt väldigt intressant och lärorikt.

Uppgift 4.2

Swedish numberplates consist of three letters and three numbers. The numbers range from 001 to 999. Using one of the four common rules of arithmetics (+, -, *, /), on how many of the numberplates can the two first numbers give the third number?

Examples: 'ABC123' can be combined to 1 + 2 = 3. So this numberplate is good. 'ABC129' 1 and 2 cannot be combined to give 9 using the four rules of arithmetics, so this does not work.

Order matters, so only use 1 +-/ 2 = 3 and not 2 +-/ 1 = 3.

Do not count multiple times if more than one rule of arithmetics work.

Answer with the number of numberplates.

Marvin 1 A5

Räkna poäng, gör ett menyval där Marvin tar emot en input som ska vara en lång sträng med där varannan karaktär är en bokstav och varannan är en siffra, ex "a2b4A5s3B1". En bokstav representerar en spelare och efterföljande siffra är dess poäng. Om bokstaven är liten, får spelaren poäng, om det är en stor bokstav förlorar spelaren poäng. Räkna ut och skriv ut varje spelares totala poäng.

2 Studentens kod 4.2.py

counter = 0
c = 0

for x in range(1, 1000):

    if x < 10:
        a = 0
        b = 0
        c = x
    elif x < 100:
        a = 0
        b = int(str(x)[0])
        c = int(str(x)[1])
    else:
        a = int(str(x)[0])
        b = int(str(x)[1])
        c = int(str(x)[2])

    if a + b == c:
        counter = counter + 1
    elif a - b == c:
        counter = counter + 1
    elif a * b == c:
        counter = counter + 1
    else:
        if (a >= 1) and (b >= 1):
            if a / b == c:
                counter = counter + 1

3 Feedback 4.2.md

4.2 i lab2

Vi börjar med 4.2 i lab2. Koden är inte dålig men jag reagerade på en sak, hur du producerar dina nummerskyltar. Du loopar från 1 till 1000, vilket känns logiskt i och med att vi ska producerar alla tal mellan 001 och 999. Men du ska inte hantera 001 som ett tal utan som tre individuella tal. Vilket göra att du har lagt in if-case för att plocka ut de tre siffrorna och dessutom behöver du typkonvertera till sträng och tillbaka till heltal. Det känns rimligt men lite omständligt.

Jag har löste uppgiften med 3 nästlade for-loopar istället, där varje loop går från 0-10. På så sätt produceras tre individuella siffror direkt och vi slipper if-satser och typkonvertering.

for i in range(10):
    for j in range(10):
        for k in range(10):
            if i == 0 and j == 0 and k == 0:
                continue
            if i + j == k:
                count += 1
            elif i - j == k:
                count += 1
            elif i * j == k:
                count += 1
            elif j > 0 and i / j == k:
                count += 1

Själva uträkningarna gör vi typ likadant. Så den stora skillnaden mellan våra koder är hur vi producerar siffrorna att räkna på. Jag blev nyfiken på tidsskillnaden på din lösning och min. Jag har kört koderna med modulen timeit för att få fram hur långtid koderna tar att exekvera.

timeit

Med timeit kan man välja hur många gånger man vill exekvera koden och se hur lång tid det tar.

Hela koden

If-satser, loopar och typkonvertering är "dyrt" när man kollar på tid. Det är sånt som tar lång tid och därför vill man minska mängden sånt i sin kod. Vikollar på hur lång tid det tar att exekvera hela koden och längre ner försöker jag bryta ned vad i din kod det är som skiljer från min och hur det påverkar tidsexekveringen.

100 gånger

Din kod exekverad 100 gånger i rad: 0.15331500000320375 sekunder
Min kod exekverad 100 gånger i rad: 0.026871000009123236 sekunder

Din kod kör ~6 gånger långsammare.

1000 gånger

Din kod: 1.512726000015391 sekunder
Min kod: 0.2731109999876935 sekunder

Din kod kör ~5.5 gånger långsammare.

Tiden ökar linjärt med antalet gånger koden exekveras. Så jag kommer bara köra koden 1000 gånger från och med nu.

Bara for-loopen

Här jämför vi med bara din for-loop mot mina tre loopar, för att se vilken av dem som är snabbast.

for x in range(1, 1000):
    pass

Mot

for i in range(10):
    for j in range(10):
        for k in range(10):
            pass

1000 gånger

Din kod: 0.015759000001708046
Min kod: 0.0401879999844823

Här är min kod långsammast, din kod är ~2.5 gånger snabbare. Så fler loopar är långsammare än en stor loop.

Men nu testar vi att lägga till din kod för att separera siffrorna.

Producera de tre siffrorna

Min kod ser likadan ut men för dig lägger vi till den första if-sats sektionen.

for x in range(1, 1000):
    if x < 10:
        a = 0
        b = 0
        c = x
    elif x < 100:
        a = 0
        b = int(str(x)[0])
        c = int(str(x)[1])
    else:
        a = int(str(x)[0])
        b = int(str(x)[1])
        c = int(str(x)[2])

1000 gånger

Din kod: 1.3197869999858085
Din kod: 0.04421900000306778

Din kod blir ~30 gånger långsammare för att producera samma resultat när vi lägger på if-satser och typkonvertering.

Slutsatsen vi drar från detta är, om vi kan vill vi undvika if-satser och typkonvertering.

Uträkningarna

För skojs skull jämför jag också if-satsen för uträkningarna:

counter = 0

for a in range(10):
    for b in range(10):
        for c in range(10):
            if a + b == c:
                counter = counter + 1
            elif a - b == c:
                counter = counter + 1
            elif a * b == c:
                counter = counter + 1
            else:
                if (a >= 1) and (b >= 1):
                    if a / b == c:
                        counter = counter + 1

Mot

count = 0
for i in range(10):
    for j in range(10):
        for k in range(10):
            if i == 0 and j == 0 and k == 0:
                continue
            if i + j == k:
                count += 1
            elif i - j == k:
                count += 1
            elif i * j == k:
                count += 1
            elif j > 0 and i / j == k:
                count += 1

Kod är nästan likadan, det som skiljer är att jag börjar min if-sats med att kolla om alla är 0. Medan du har två if-satser för att kolla divisionen. Men, med "din" kod räknar den med 000 nummerplåtn vilket inte ska vara med egentligen. Så den koden ger 153 som svar istället för 152.

1000 gånger

Din kod: 0.31176400001277216
Min kod: 0.3515369999804534

Det är minimal skillnad på de två men din blir lite snabbare. Det ska noteras att jag körde skriptet flera gånger och ibland blev min kod lite snabbare, men oftast vann din.

Varför är din snabbare även om du har fler if-satser?

Troligen för att även om du har fler if-satser så exekveras inte alla dina if-satser lika ofta som mina. Jag börjar varje iteration i looparna med att kolla om siffrorna är 0 även om det bara kommer inträffa 1 gång. Det är onödigt, att kolla den 999 gånger när jag vet att det bara kommer inträffa en gång, första iterationen.

Medan i din kod så exekveras bara den extra if-satsen om koden har kommit förbi de tidigare elif. Vilket inte kommer inträffa alls lika ofta som min extra if-sats exekveras.

Det här blev väldigt lång, men lärorikt, för att visa upp hur kodens utformad kan påverka hur lång tid det tar för den att exekvera. Nu är inte detta något vi kommer märka av i kursen, vi gör inte så avancerade program eller jobbar med så mycket data för att vi ska märka någon skillnad. Det är därför jag kör koden 1000 gånger, för att det ska ta lite tid.

4 Marvin A5 kod.py

alphabet_lower = "abcdefghijklmnopqrstuvwxyz"
alphabet_upper = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
points = ""
counter = 0

string = input("Enter a string for evaluation: ")

length = len(string)

for char_lower in alphabet_lower:

    char_upper = alphabet_upper[counter]
    plus_p = 0
    minus_p = 0

    if string.find(char_lower) >= 0 or string.find(char_upper) >= 0:

        if string.find(char_lower) >= 0:
            for i in range(length):
                if string[i] == char_lower:
                    plus_p = plus_p + int(string[i+1])

        if string.find(char_upper) >= 0:
            for j in range(length):
                if string[j] == char_upper:
                    minus_p = minus_p + int(string[j+1])

        points = points + char_lower + " " + str(minus_p*-1 + plus_p) + ", "

    counter = counter + 1

print(points[:(len(points)-2)])

5 A5 feedback.md

Här har vi en hel del vi kan ändra på. Om vi bara kollar på din kod som den är skriven nu, kan du lägga till enumerate på for char_lower in alphabet_lower: för att slippa ha counter = 0 variabeln. I och med att du bara använder counter för att räkna med varje iteration i loopen.

Du loopar igenom hela alfabetet och kollar om varje bokstav finns i input strängen. Det är onödigt, för vi gör inget med kunskapen om en bokstav inte finns i strängen.

När du väl har hittat en bokstav som finns i strängen då kollar du igenom om det är liten bokstav eller stor bokstav som finns.

if string.find(char_lower) >= 0 or string.find(char_upper) >= 0:

    if string.find(char_lower) >= 0:
        ...
    if string.find(char_upper) >= 0:
    

Här gör du samma sak två gånger och du borde kunna ta bort den yttre if-satsen helt.

Du har två väldigt lika kod snuttar.

if string.find(char_lower) >= 0:
    for i in range(length):
        if string[i] == char_lower:
            plus_p = plus_p + int(string[i+1])

if string.find(char_upper) >= 0:
    for j in range(length):
        if string[j] == char_upper:
            minus_p = minus_p + int(string[j+1])

När man har kod som är så här lika då kan man oftast skriva om den till en kod snutt. Här vill vi få bort en av for-looparna så vi inte ska behöva loopa igenom två gånger.

Försök hitta gemensamma delar vi kan använda för både stor och små bokstäver och sen hur vi kan specialisera mot om det är stora eller små. Försök skriva om till en for-loop istället för två.
Du kan använda funktionen isupper() på en sträng för att kolla om det är stor eller liten bokstav.

Lösning

for l in string:
    if char_lower == l.lower():
        if l.isupper():
            counter -= 1
        else:
            counter += 1

Nu hittar vi rätt bokstav i samma loop och kör plus eller minus beroende på om det är stor eller liten bokstav.

Det som är kvar nu är att byta ut att loopa igenom hela alfabetet och istället bara loopa över strängens bokstäver. Det gör du lättast genom att göra om hela strängen till lower och bara loopa igenom den. Jag skrev lite om det i feedbacken till den andra studenten, https://gist.github.com/AndreasArne/f43c0fff0f814527f5324f102908cc48#file-min-feedback.