stejsle1 / wator

Simulation of Wa-Tor the sea world
Creative Commons Zero v1.0 Universal
0 stars 0 forks source link

Feedback ke stylu #2

Open hroncok opened 7 years ago

hroncok commented 7 years ago

V zásadě to máš dobře, zadání to splňuje a testy prochází. Mám ale trochu feedbacku, na základě toho, jak vypadá kód. Přijde mi velmi špatně čitelný a rozhodně bych ho nenazval pythonistickým. Několik tipů:

Rozděl metody __init__ (45 řádek) a tick (153 řádek) na podmetody. Vhodným pojmenováním metod způsobíš, že čtenář metody bude vědět, co se má stát, i když neuvidí celou implementaci na jednom místě. Vhodné metody pro __init__ by mohly být třeba: _prepare_animals (a _random_animals), _prepare_energies. Pro tick pak třeba _move_fish, _move_sharks, _starve_sharks. Některé z těchto metod by mohly znovuvyužívat kus kódu, který řeší pohyby do 4 stran, a tento kód by nemusel být zopakován 3krát.

Máš velmi dlouhé řádky kódu, kde je toho hodně na pochopení. Zkus si vytvořit pomocné funkce nebo ještě lépe properties.

Srovnej čitelnost:

if nfish == None or nsharks == None or nfish > creatures.shape[0]*creatures.shape[1] or nsharks > creatures.shape[0]*creatures.shape[1]:  

s

if nfish == None or nsharks == None or nfish > self.size or nsharks > self.size:  

Podobně:

if ran == 2 and creatures2[(i-1+creatures.shape[0])%creatures.shape[0],j] == 0:

s

if ran == 2 and creatures2[(i - 1 + self.height) % self.height, j] == 0:

:bulb: Mimochodem to přičítání self.height dokonce ani není potřeba:

>>> -1 % 5
4
>>> (-1 + 5) % 5
4

Tvůj kód je taky celkem kondenzovaný a hutný. Podívej se na :8ball: PEP 8 a zkus se ho trochu dodržovat. Existují automatické kontroly (pycodestyle, flake8) i částečné převaděče (autopep8).

Existují i další lintery, které ti řeknou, co by mohlo být problematické. Třeba pylint, ale já ho považuju za příliš striktní (je dobré si ho ale pustit a nad jednotlivými výtkami se alespoň zamyslet).

V přípapdě dotazů se neboj na mě nebo @MarekSuchanek obrátit.

stejsle1 commented 7 years ago

Kod jsem poupravila, v upravach budu pokracovat. Diky za feedback.

Chtela bych se zeptat k ukolu na Cython: kdyz definuji promenne tridy

cdef class WaTor:
   cdef numpy.int64_t[:] energies
   cdef numpy.int64_t[:] creatures
   cdef int age_fish
   cdef int age_shark
   cdef int eat

a zaroven v puvodnim kodu pouzivam pro zjisteni poctu zvirat boolarr = self.creatures < 0, python setup.py develop hlasi chybu, ze nezna funkci < pro dane datove typy (Invalid types for '<' (int64_t[:], long). Mam promenne zadefinovat jinak, nebo ve funkcich count_fish/count_sharks lokalne predefinovat promenne?

hroncok commented 7 years ago

Pomůže definovat numpy matice jako matice a ne jako pole intů?

cdef numpy.ndarray[numpy.int64_t, ndim=2] energies

BTW Dělat celou třídu WaTor v Cythonu je overkill. Doporučuju spíš vyhledat kritická místa a na ty napsat a použít funkci v Cythonu.