funkcionális ügyek about code-review HOT 16 OPEN

Hm.. rájöttem, hogy ez az issú csak kis részben code review téma, nagyobb részben "ma is tanultam valamit" beszámoló. :)
De sztem az ilyesmi is hasznos lehet.

Az én ízlésemnek néhol kicsit nyakatekert. (pl. innentől lefelé.) Ez már funkcionálisprogramozás-alkoholizmus. :)

Persze, csak élveztem, hogy tényleg meg lehet csinálni kb. mindent ilyen módon
➡️ tényleg létezhetnek elvben funkcionális nyelvek 😃.

Kommentek nélkül nehezen érthető. (pl. itt még nem látszik, hogy hogy kerül a csízma az asztalra az EMPTYWORD (SPOILER: üres string) az iterátorba ami listákat ad vissza. Ami egyébként akár None is lehetne, mert az szokott az extremális elem lenni. De mindegy is lenne, ha paraméterezni lehetne a stream_to_words függvényt egy beszédes nevű parméterrel és nem lenne beleégetve a konstans.)

Ezt majd egyszer szóban, mert sztem bonyolultabb.

Ezeket a setup-okat el kellene dugni egy függvénybe:

for sentence in sentences:
     for center_elem, window in window_around_center_element(...):  # Itt lehetne center_index is vérmérséklettől függően mindkettő.
         pass  # Itt a lényeg.

Ezzel látszódna a lényeg, ami az adott logikai szinten érdekes, a paraméterek pedig paraméterek lennének tényleg.

Na ez tök jó tipp, köszi, ezt bele fogom tenni! 👍

from code-review.

@sassbalint

Az eredeti példánál maradva, az én javaslatom:

def stream_to_words(stream, sentence_sep=None):  # A None default érték vérmérséklettől függően elhagyható.
    """Process line: split by TAB + handle empty lines."""
    for line in stream:
        line = line.rstrip('\n')
        yield line.split('\t') if len(line) > 0 else sentence_sep

Ez van a "lib.py"-ban, amit hív a "main.py":

from more_itertools import split_at
from word import EMPTYWORD

words = stream_to_words(sys.stdin, sentence_sep=EMPTYWORD)
sentences = split_at(words, lambda x: x == EMPTYWORD)

Ebben a változatban nem kell nagyon utánanéznem, hogy jön a stream_to_words-höz az EMPTYWORD, mert a paraméter nevéből világos (sentence_sep). Egy függőség van a "lib.py"->"main.py" irányban. Az API garancia a def soron belül marad.

Amennyiben egy csillag alakú topológiát képzelünk el a különféle "lib.py"-ok és a "main.py"-ok között, nincs összefüggés az egyes "lib.py"-ok között és azok teljesen függetlenül cserélgethetőek, ha épp van jobb implementációnk.

Az a mögöttes érv, hogy a lokális kontextusból minden kiderül és még 3 sorral (=vagy nagy programnál sokkal) feljebb (esetleg más fájlban) se kell nézni.

A te változatodban:

EMPTYWORD = ''

# IDE KÉPZELJÜNK SOK SOR KÓDOT MÉG

def stream_to_words(stream):
    """Process line: split by TAB + handle empty lines."""
    for line in stream:
        line = line.rstrip('\n')
        yield line.split('\t') if line != EMPTYWORD else EMPTYWORD  # <- Ezzel külső függősége van a függvénynek, amit fejben kell tartani pl, ha nem standard vagy self-explanatory cucc.

A main.py-ben

from more_itertools import split_at
from word import EMPTYWORD

words = stream_to_words(sys.stdin)   # <- Itt titkon felhasználunk egy konstanst, ami nincs benne a függvény szignatúrájában.
sentences = split_at(words, lambda x: x == EMPTYWORD)

Az EMPTYWORD hogy kerül a stream_to_words kimenetébe? Ezt vagy kommenttel jelezzük vagy az embernek meg kell néznie a függvény definícióját. Az, hogy meg kell nézni valaminek a definícióját a legrosszabb, ami történhet, mert akkor a dokumentáció és az API nem elég a megértéshez.

Valamint két ponton függ össze a word.py és a connect_prev.py, aminél egyszer csak elkezd ez a két dolog külön életet élni és onnantól nagyon nehéz már az API kompatibilitást megőrizni.

Ez akkor lehet jó stratégia (Virág érve ez volt) ha előbb-utóbb konfigurációvá akarjuk alakítani a konstansokat és több helyen használni. Azt ilyenkor nehéz thread-safe módon (=ha több példány fut függetlenül) persze, de nem hülyeség alapvetően.
És ne feledjük a körkörös importokat. Ha nem jó az import hierarchia, akkor abba is bele lehet futni.

A lényeg, hogy nagyobb kódbázisnál és lassan változó API-knál jönnek ki ezek a problémák. Ha belső API van, akkor azt te látod át és saját kézzel tudod karbantartani, de külső API-nál ez nehezebb, mert akkor nem tudhatod, hogy használják.

Remélem érthető voltam. :)

from code-review.

@sassbalint

Az egyik a konstans a másik a függvény.

A külön élet úgy néz ki, hogy mondjuk az idő egy pontján úgy döntesz, hogy

1. konstans nélkül is megoldható a függvény funkciója. Pl. Iterátorok iterátorát adod vissza (=mondatok iterátora, ahol a mondatok maguk iterátorok.). Ekkor a konstans lehet, hogy okafogyott lesz, de ez nagyon nehezen fog kiderülni, mert semmi sem fogja aláhúzni, hogy unused variable. -> Ha a függvényen belül tartjuk, akkor ilyen nem lesz.
2. A Pi az ókori görögök óta 3.14... Valószínűleg az már úgy marad (bár az IEEE 754 szabványnak biztos lenne különféleménye). Ha viszont egy extremális elemet akarsz konstanssá emelni, akkor azt az extremális elemet máshol is akarhatod használni előbb utóbb vélhetően, aztán a formátum megváltozik, (mert a formátumok ilyenek) és akkor a konstanst meg kell változtatni néhány helyen, néhányon meg meg kell tartani, mert ott még jó a célra. -> Ha minden függvénynek saját privát élete van, akkor a fő programban, ami hívja őket egy helyen látszik a formátum és a generikus függvények nagyon jól paraméterezhetőek és újrahasználhatóaklesznek (=lásd az utalás a több különféle példány egyidejű futtatása).

A dolog véleményességét -- ismétlem -- az adja, hogy ha egy nagy programrendszer stabil standardokra épül, akkor persze logikusabb külön definiálni a konstansokat/defaultokat és a függvényeket. De ha még alakulóban van a dolog, akkor érdemesebb megtartani az elemek függetlenségét és a rugalmasságot amíg a tapasztalat nem mutatja, hogy abban az irányban nincs szükség a rugalmasságra és a konstansok tényleg konstansként viselkednek (és nem mint sokat változó paraméter). Egy ilyen átalakítást API törés nélkül legfeljebb minor verzióban be lehet vezetni utólag is. A visszavonás sokkal nagyobb törést eredményez. Kis programok esetén meg persze tökmindegy.

Pl. az Unix utilok esetén a rekord és a mezőelválasztó átírható (paraméter), de az esetek nagy részében biztonságos a defaultokra hagyatkozni (konstans). Ez elég rugalmas és időtálló rendszer, de a tervezéskor, kísérletezéskor, még nem biztos, hogy ez az architektúra a célszerű. ;)

Még annyi kiegészítés, hogy én szétválasztanám a random formátum beolvasást a logikai formátumba lépést a feldolgozási logikától.

1. A formátum változhat.
2. A beolvasás kódja jobban újrahasznosítható más feldolgozás esetén.

(Az xtsv-nél is a rendszer achilles sarka jelenleg a tsv formátumra való túlzott hagyatkozás.)

from code-review.

"Code volume is kept small by linking the tools together in a functional style which helps eliminate temporary variables." 😃

from code-review.

"Code volume is kept small by linking the tools together in a functional style which helps eliminate temporary variables."

Néha elgondolkozom, hogy hogy is lehetne tulajdonképpen máshogy értelmesen csinálni? :D

A toolbox filozófia és a funkcionális programozás kéz a kézben járnak. Az OOP meg hát... Ahhoz nem értek, kivéve a gyémánt alakú öröklődés felismerését. ;)

from code-review.

more-itertools/more-itertools#536 :)

from code-review.

Csináltam belőle PR-t more-itertools/more-itertools#542, és csodák csodája tetszett nekik és merge-ölték! 🎉

from code-review.

@sassbalint Gratulálok!

from code-review.

@dlazesz @sassbalint És ha már mappelés, akkor nem állom meg, hogy ne reklámozzam újra a régi contextfun csomagomat.

Téma: iterable filterezése és mappelése nem az egyes elemek tulajdonságai, hanem a környezetük tulajdonságai alapján.

Szívesen vennék bármilyen visszajelzést, kód review-t, feature request-et, bug-report-ot, README-fejlesztést, véleményt, stb.!

Iván, ezzel most 2 percet töltöttem el, de első ránézésre azt gondolom, hogy ez is mehet a more-itertools-ba, persze ha gondolod. :)

Megj: én map_if-nek egy olyan kiterjesztése dolgozom (khm..), hogy elemek adott n hosszúságú sorozatára (map_if esetén n=1, itt ez lenne általánosítva) ha teljesül egy feltétel, akkor mappel. Ezzel olyanokat lehet csinálni, hogy ha két szomszédos elem (n=2 eset) valamilyen, akkor pl. össze lehet vonni őket és egy elemet bocsátani ki helyettük.

from code-review.

Az én ízlésemnek néhol kicsit nyakatekert. (pl. innentől lefelé.) Ez már funkcionálisprogramozás-alkoholizmus. :)

Kommentek nélkül nehezen érthető. (pl. itt még nem látszik, hogy hogy kerül a csízma az asztalra az EMPTYWORD (SPOILER: üres string) az iterátorba ami listákat ad vissza. Ami egyébként akár None is lehetne, mert az szokott az extremális elem lenni. De mindegy is lenne, ha paraméterezni lehetne a stream_to_words függvényt egy beszédes nevű parméterrel és nem lenne beleégetve a konstans.)

Ezeket a setup-okat el kellene dugni egy függvénybe:

for sentence in sentences:
     for center_elem, window in window_around_center_element(...):  # Itt lehetne center_index is vérmérséklettől függően mindkettő.
         pass  # Itt a lényeg.

Ezzel látszódna a lényeg, ami az adott logikai szinten érdekes, a paraméterek pedig paraméterek lennének tényleg.

from code-review.

@dlazesz

Balázs, ez az EMPTYWORD-ös téma talán pont az, amivel Virággal küzdöttél:
sok helyen használom, ezért egy helyen definiálom, és mindenhol importálom.
Asszem ugyanezt a hozzáállást képviselte Virág.
És te mit is ajánlottál helyette?

from code-review.

@sassbalint

Balázs, ez az EMPTYWORD-ös téma talán pont az, amivel Virággal küzdöttél.

Én az importálás helyett a paraméterként átadást pereferálom, mert egyrészt loosely coupled-dé teszi a kódot (csak stdlib-et importál a lib fájl és a fő fájl a lib fájlt), másrészt a paraméter integránsabban a függvény része (=funkcionális, tehát nincsenek side-effectek) mint a konstansok. Ezért a refaktorálás is egyszerűbb. (A függvény self-contained.)

Ezzel lehet vitatkozni. Virág meg is tette. Abban maradtunk, hogy legyen az ő változata, aztán majd meglátjuk, hogy fog jobban kinézni. :)

from code-review.

@dlazesz

Szóval stimmel: ugyanaz a probléma! :)
Tudnál esetleg mutatni/írni egy minimál kódot két verzióban, ami bemutatja az egyik illetve másik hozzáállást?
Az tök jó lenne. 👍

from code-review.

@dlazesz

Köszi, Balázs, a példát és a részletes magyarázatot.

Érthető voltál! 😃

Asszem egy kérdésem maradt:

Valamint két ponton függ össze a word.py és a connect_prev.py, aminél egyszer csak elkezd ez a két dolog külön életet élni...

Pontosan mi ez a két pont és hogy tud külön életet élni?

from code-review.

@dlazesz @sassbalint És ha már mappelés, akkor nem állom meg, hogy ne reklámozzam újra a régi contextfun csomagomat.

Téma: iterable filterezése és mappelése nem az egyes elemek tulajdonságai, hanem a környezetük tulajdonságai alapján.

Szívesen vennék bármilyen visszajelzést, kód review-t, feature request-et, bug-report-ot, README-fejlesztést, véleményt, stb.!

from code-review.

@mittelholcz

Szívesen vennék bármilyen visszajelzést, kód review-t, feature request-et, bug-report-ot, README-fejlesztést, véleményt, stb.!

Ránéztem és van pár kérdésem, észrevételem:

1. Itt nekem a PyCharm jelez egy soremelé hiányt a fájl végén (fura, mert a github nem mutatja): https://github.com/mittelholcz/contextfun/blob/master/contextfun/__init__.py#L8
2. A get_distribution-t elmondhatnád, hogy mit csinál és miért jó mondjuk a sima stringhez képest?
3. Itt miért lista, miért nem queue legfőképpen miért nem tee és zip vagy more-itertools chunked iterator?
4. Jó lenne egy ilyen frameiter-hez hasonló jól olvasható, gyors megoldás ide PR wellcome :)
5. Én nem nevezném i-nek az iterátor elemét ha nem szám, inkább item. (Ez véleményes.)
6. Itt kiszűrjük a fake-ket. Akkor mi értelmük volt? Mármint valami placeholder kell, hogy a kontextus a helyén legyen? Pl van egy 3 hosszú iterátorom 5 ös ablakkal, akkor kap egy 3 hosszú kontextust a 4 hosszú helyett, amit várok a középső 5. elemhez és máris meghalt a quantifier, nem? (Olyan quantifiert nem lehet írni ami a kontextus elemeit a helyi érték szerint várja?) Magyarul: Miért generátor a quantifier, miért nem fix hosszú tuple?
7. A predicate() és quantifier() elvárt szignatúráit a docstringbe be kellene írni.
8. Itt nem castolsz int-re, itt meg igen. Válasz egy megoldást és elimináld a kód duplikációt (valójában a map és a filter csak ebben különbözik: a és b)
9. Az i[before][0] kiszedhető az if elé, mert ez is duplikáció az if-en belül.
10. [Ez])(https://github.com/mittelholcz/contextfun/blob/master/test/test_contextual_filter.py#L18) PEP8 as hiba. Kellene egy space az == után.
11. Túl sok újsor itt
12. Itt és még a többi copy-paste helyen a kettőskereszt előtt kellene még egy space (PEP8)
13. A test/test_contextual_filter.py fájl végén egy újsort hiányol a PyCharm amúgy rengeteg kód duplikáció van a fájlban, kevesebb kóddal kellene ugyanezt megoldani.
14. A test/test_contextual_filter.py fájlra ugyanezek a meglátásaim.
15. Itt fölösleges a lista a tuple-ben, mert generátort is eszik. Itt ugyanez.
16. Itt egy PEP8
17. Itt túl sok space és kód duplikáció. Legyen segédfüggvény.
18. A test/test_contextual_frameiter.py fájl végén is egy újsort reklamál a PyCharm
19. Itt az i helyett _ -et rakj, mert nem használod úgyse.
20. Itt nem kell az object-ből örököltetni Pyhton3-ban a 2 meg a múlté.
21. Amúgy a performance test-be nem ártana pár komment. Hogy teszteli ez a gyorsaságot? Miért nem timeit?
22. if we want replace numbers -> want to és applied for items -> applied to items (A saját szövegeimben nem látom meg ugyanezeket a hibákat. Szóval no para! ;) )
23. in the list before prime numbers én a before-t emph-elném. A másik helyen az after-t szintén.
24. Az egy hosszú kontextus esetén az any és az all ugyanazt tudja. Úgyhogy hosszabb kontextusra is kellenének példák SZVSZ.

Iván, ezzel most 2 percet töltöttem el, de első ránézésre azt gondolom, hogy ez is mehet a more-itertools-ba, persze ha gondolod. :)

Ezzel maximálisan egyetértek.

@sassbalint

Megj: én map_if-nek egy olyan kiterjesztése dolgozom (khm..), hogy elemek adott n hosszúságú sorozatára (map_if esetén n=1, itt ez lenne általánosítva) ha teljesül egy feltétel, akkor mappel. Ezzel olyanokat lehet csinálni, hogy ha két szomszédos elem (n=2 eset) valamilyen, akkor pl. össze lehet vonni őket és egy elemet bocsátani ki helyettük.

Ez nem a map_if + n-gramok a bemenetek, amin vizsgálja a mappelést?

Ha nincs meg az elemszám, az nem map!
A probléma ilyenkor szétbontható egy map-ra (ami fake elemekkel tölti fel az iterátort( és egy filterre (ami kiszűri a fake elemeket).

from code-review.