Code Reviews

[simone989]

Ciao Ragazzi, il Dottor Borzì mi ha incaricato di effettuare la Code Review del vostro progetto.

Tra oggi e domani vi segnalerò in questa issue i miei feedback.

Qualora ci fossero dubbi o incomprensioni su alcuni miei feedback siete liberi di commentare.

Vi Auguro una buona giornata

[simone989]

 __pycache__

Vi consiglio di definire le due folder e i rispettivi file al suo interno dentro il file .gitignore.

E' buona norma non committare tali tipi di file per diverse ragioni:

• Aumento dello spazio usato dal progetto git. Nel tempo quei file potrebbero pesare diversi Mb/Gb rallentando così il download della repo da una pipeline/Github Action esempio.
• Sicurezza, dentro le cache potrebbero finire informazioni sensibili come il token del bot nel vostro caso
• Le cache potrebbero non essere compatibili tra le diverse architetture quindi potrebbero causare problemi in fase di esecuzione dello script.

[simone989]

Name Convention Test Files

Piuttosto che utilizzare le iniziali del file che si vuole testare, vi consiglio di chiamare il files di test nel medesimo modo del file che si vuole testare e aggiungendo il suffisso _test. Esempio: ep_test.py --> element_processing_test.py

Un altro approccio valido è quello di aggiungere .test, sempre come suffisso.

Così da rendere unico il file e rendere più leggibile il progetto ad un utente esterno.

[simone989]

__init__

Vi consiglio di spostare il contenuto del file principale del progetto, coreRadioBot.py, nel file __init__.py che si trova nella folder principale del progetto.

[simone989]

String Interpolation / Constant

Provate ad usare f-string per unire più stringhe:
Esempio:

f"Hello {user}, I'm xCoreBot!\nI'm a bot that provides information about the latest releases in the Metal music genre. Use the command '/help' to discover the commands you can use!"

Per quanto riguarda invece il parametro text che passata a send_message per i comandi: help, wrong_question usate delle costanti dove definire le stringhe di testo. Meglio evitare il più possibile stringhe/numeri hardcoded.

[simone989]

Magic Number

Nella funzione n_songs_checker viene ritornato 20 nel caso in cui il primo if non venga soddisfatto, definire tale valore in una variabile.
Esempio: DEFAULT_NUMBER_OF_SONG_PAGE_SIZE = 20

---

Stessa cosa nella funzione calling_parallel con il numero 32 e con il parametro max_worker=16

---

In altre funzione sto notando il medesimo "problema", non ve li elenco tutti ma vi consiglio di definire per ognuno dei casi una variabile specifica.

[simone989]

Perchè il metodo si chiama arguments_checker se poi il controllo dei return value viene fatto dalla funzione che chiama quel metodo ?

Vi consiglierei di fare un raise di una exceptions qualore i parametri non fossero validi.

[simone989]

site_requests_maker

Potreste aggiungere dei commenti a questa funzione così da rendere più chiaro a cosa vi serve la variabile subtoken ?

[simone989]

Single Responsibility

In diverse occasioni ho notato che tale principio non viene rispettato, esempio:
La funzione site_requests_maker eseguire diverse operazioni al suo interno:

• controlla che tipo di comando l'utente ha lanciato
• Parsa il cotenuto html
• Estrapola le informazioni delle canzoni
• Eseguire la funzione che filtra i brani

Con questo non voglio dirvi di spittare in più funzioni ma tenetelo in considerazione nei futuri progetti.

[simone989]

Update Function Params

Nella funzione process_elements_list andate a modificare direttamente il parametro page_list, è buona norma creare una copia del parametro, modificarlo e ritornare la copia modificata lasciando intatto il parametro.

[antof27]

Buonasera Simone, ti chiedo scusa in anticipo se rispondo solo adesso, purtroppo sono giorni un pò impegnativi. Ti ringrazio/ringraziamo per la revisione, non appena abbiamo la possibilità, provvediamo a sistemare tutto ciò che non va. Un buon proseguimento di serata, a presto!

…

________________________________ Da: Simone Di mauro ***@***.***> Inviato: lunedì 19 febbraio 2024 18:03 A: antof27/xCoreBot ***@***.***> Cc: Subscribed ***@***.***> Oggetto: Re: [antof27/xCoreBot] Code Reviews (Issue #10) Update Function Params Nella funzione process_elements_list andate a modificare direttamente il parametro page_list, è buona norma creare una copia del parametro, modificarlo e ritornare la copia modificata lasciando intatto il parametro. — Reply to this email directly, view it on GitHub<#10 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AURRZTGKSGGPOIPU6AJXBITYUOAVNAVCNFSM6AAAAABDPEGDDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJSHA4DSMBQGM>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[antof27]

Buonasera Simone,
abbiamo apportato tutte le modifiche da te suggerite.
Ti ringraziamo nuovamente per il tempo dedicatoci, rimaniamo a disposizione se dovessero essere necessari ulteriori cambiamenti. Ti auguro una buona serata.