cotestatnt
Commenti al codice a ruota libera
Come da accordi apro questa "Issue" senza pretese, solo per condividere opinioni sulla realizzazione del progetto. Partirei usando l'italiano, se necessario commutiamo in inglese. I commenti sono al solo scopo di chiarire alcuni aspetti ed eventualmente generare spunti di riflessione. NON è mia intenzione criticare l'ottimo lavoro svolto.
Base64
- I sorgenti in Base64 poco si integrano con il resto del codice C++, sembrano qualcosa di separato scritto in C. L'uso di classe C++ mi sembrerebbe più coerente.
GoogleFileList
Mi aspettavo più commenti, almeno nel .h. È una cosa che si aspetta l'utente della libreria.
La dipendenza da <Arduino.h>, almeno per l'header, è forse eccessiva. Sposterei l'include nel file .cpp.
I membri name ed id di GoogleFile potrebbero essere string, viceversa una inizializzazione a nullptr a mio avviso andrebbe comunque resa esplicita anche per loro e per isFolder.
_filesCount andrebbe esplicitamente inizializzata a 0, almeno per chiarezza.
La funzione clearList() non distrugge gli oggetti che compongono la lista linkata, ma si limita a cancellarne i riferimenti. Confesso di conoscere un C++ abbastanza datato, ma non ho trovato informazioni riguardo ad un eventuale garbage collector anche per C++, mi aspetto dunque che ogni oggetto vada distrutto prima di eliminarne i riferimenti.
Modificherei il prototipo di void addFile(GoogleFile gFile); in void addFile(const GoogleFile& gFile);. Inutile passare l'intero oggetto .
In void GoogleFilelist::addFile(const char* _name, const char* _id, bool _isFolder) i nomi preceduti da underscore sono fuorvianti (sono usati altrove per i membri della classe). In linea generale, in C sicuramente, tutti i nomi che iniziano per underscore sono da evitare perché riservati al sistema.
La costante 30 dovrebbe avere un nome parlante, che spieghi perché proprio 30.
L'allocazione fatta con new non ha il corrispettivo delete, ma questo è stato già detto prima.
Le due istruzioni strdup() allocano anche loro memoria mai rilasciata.
Doppio ;; alla riga #34.
Guardando le varie funzioni getFileXXX() viene da chiedersi se l'uso della lista sia la soluzione più comoda. Visto che non vengono mai tolti elementi non sarebbe più semplice l'implementazione tramite un array di puntatori a GoogleFile?
Non vedo applicazioni per la funzione getList() che restituisce l'oggetto in se.
Quando le classi contengono puntatori è bene controllarne la copia e l'assegnazione. Volendo impedire l'uso di copy constructor ed assignment si possono dichiarare le funzioni come private senza implementarle: GoogleFilelist(cont GoogleFilelist &obj); e GoogleFilelist& operator=(const GoogleFilelist& obj);. Questo evita "l'uso improprio del prodotto", anche non intenzionale.
Ciao Carlo! L'italiano va benissimo anche per me. Intanto grazie per i tuoi commenti che sono sempre molto apprezzati :) GoogleFileList credo che sia la classe messa peggio... sono rimasto combattuto a lungo se implementarla. L'idea di base era avere a portata di mano gli id di tutti i file accessibili all'ESP senza dover ogni volta inviare una richiesta al server che comunque ruba tempo.
Ho corretto le sviste per quanto riguarda le allocazioni e accolto i suggerimenti sul naming. Sto cercando anche di semplificare alcuni metodi e rivedere alcuni meccanismi di base... appena raggiungo una versione "stabile" faccio un nuovo upload.
Ciao, l'idea era quella di continuare con gli altri file, ma penso che bene o male l'idea di fondo sia passata. Io sono dell'avviso che avere un codice "curato" e consistente renda giustizia a chi lo scrive e ne trasmette la sua identità.
Se lo stai modificando è inutile andare avanti a commentare codice ormai obsoleto. La GoogleFileList (sono partito da quella per caso) secondo me ha il suo perché, fa cache dei dati ricevuti senza dover interrogare ogni volta il servizio. Nei sistemi alimentati a batteria il guadagno potrebbe essere significativo. L'implementazione è probabilmente migliorabile riducendo l'eredità dal C e pensandola più in ottica C++ (es: strutture come GoogleFile potrebbero diventare tipi veri e propri con una loro dignità, anche se in questo caso specifico è usata solo internamente a GoogleFileList).
Ho notato che non ci sono distruttori di classe. È una cosa voluta per qualche ragione tecnica dell'hardware che utilizzi?
@carlo-banfi ho appena fatto un cospicuo update! Se ti va di riprendere con i commenti a ruota libera procedi pure :-) Diciamo che l'attuale versione può essere considerata un buon punto di partenza (almeno credo)
In merito ai distruttori di classe, ritieni sia necessario implementarli?
@cotestatnt Ciao, oggi sono un po' lesso per cui mi riprometto di fare meglio nei prossimi giorni.
Ho iniziato sempre dalla classe GoogleFilelist (non so perché, ma inizio sempre da lì).
In questo caso il distruttore è necessario perché la memoria usata per contenere la lista dei file, allocata da new in GoogleFilelist.cpp:21, non è mai liberata.
Mi viene in mente una cosa del genere, dove ogni elemento è distrutto dopo averne recuperato il puntatore al successivo:
GoogleFilelist::~GoogleFilelist()
{
GoogleFile_t * p_file = m_firstFile;
while( p_file != nullptr )
{
/* Get the next file pointer before destroying the obj */
GoogleFile_t * p_next_file = p_file->nextFile;
/* Delete the obj */
delete p_file;
/* Move to next obj */
p_file = p_next_file;
}
}
Un'altra cosa che saltata all'occhio è la mancata dichiarazione const ai membri della classe. Si dichiarano const i membri che non modificano la classe, in modo da poterli invocare anche da oggetti costanti (magari non è questo il caso, ma primo o poi capita).
Intendo una dichiarazione di classe di questo tipo:
class GoogleFilelist {
public:
~GoogleFilelist();
void clearList();
unsigned int size() const;
void addFile(const char* name, const char* id, bool isFolder);
void addFile(const String& name, const String& id_t, bool isFolder);
void addFile(const GoogleFile_t& gFile);
const char* getFileName(int index) const;
const char* getFileId(int index) const;
const char* getFileId(const char* name) const;
const char* getFileId(const String& name) const;
bool isFolder(int index) const;
bool isFolder(const char* name) const;
bool isFolder(const String& name) const;
bool isInList(const char* id) const;
GoogleFilelist* getList() const;
void printList() const;
private:
unsigned int m_filesCount = 0;
GoogleFile_t *m_firstFile = nullptr;
GoogleFile_t *m_lastFile = nullptr;
};
In cui sono stati aggiunti i const alle funzioni, per dichiarare che non modificano l'oggetto corrente e ai parametri passati per riferimento, per dichiarare che non verranno modificati dalla funzione, cosa che invece potrebbe avvenire senza const.
Dimenticavo: mancano (o non ho trovato) i sorgenti libb64/cdecode.h e libb64/cencode.h. Sono parte dell'installazione standard?
Ciao Carlo, scusa per la risposta un po' tardiva, ma sono fuori città per un breve weekend di relax 😋
Giusto, il distruttore almeno in GoogleFilelist è necessario e in fondo immagino sia meglio comunque esplicitarlo in tutte le classi. Per quanto riguarda il const per le funzioni, ammetto di averlo sempre sottovalutato... oltre a dichiarare che il metodo non modifica nulla, comporta anche benefici in termini di compilato?
I file cencode.h e cdecode.h sono inclusi nel core sia per ESP32 che per ESP8266 ed in realtà il sorgente di Base64.h è preso a mani basse direttamente dal file base64.h anch'esso incluso nel core, solo che stranamente non implementa il metodo per la decodifica che ho quindi aggiunto (pensavo di fare anche una pull request).
In effetti il metodo aggiunto è talmente semplice che forse conviene usare direttamente la funzione presa da cdecode.h? Anche se in realtà mi piacerebbe esplorare la possibilità di usare la libreria anche con altre MCU.
Ma ci manca solo, goditi il relax!
Sul fatto che il const applicato ai metodi porti benefici al compilato non ho una risposta, non l'ho mai visto in questi termini. L'ho sempre percepito come un metodo per meglio definire i vincoli del contratto e, di conseguenza, aumentare la possibilità di identificare gli errori a compile-time piuttosto che a run-time. Usandolo, diverse volte sono incappato in errori di compilazione precoci che mi hanno permesso di sistemare le cose prima di proseguire con lo sviluppo.
Una delle filosofie che mi hanno insegnato è: "dichiara tutto const, poi toglilo dove veramente non serve". È forse un po' eccessivo, ma rende l'idea.
Il fatto di incapsulare una chiamata ad una funzione di libreria tramite una tua classe secondo me ha il suo perché, dato che astrae la dipendenza dall'ambiente. Quando cambierai MCU dovrai riadattare/riscrivere solo quella funzione, mentre il resto resterà tale e quale. Come esperienza personale posso aggiungere che ho visto diverse volte nel firmware lo strato OSAL (Operative System Abstraction Layer), che non faceva altro che ridefinire le funzioni dell'RTOS di turno. Pensavo fosse un'accortezza inutile fino a che non c'è stato da migrare un progetto da FreeRTOS a Micriµm OS. Senza quello strato "inutile" saremmo morti.
Questa sera ho iniziato a guardare gli altri file, ma non ho capito la gerarchia delle classi, probabilmente perché mi mancano le basi delle API di Google.
L'eredità diretta si usa per una relazione "è un", dunque guardandolo capisco che GoogleDriveAPI è un GoogleOAuth2, così come lo è GoogleGmailAPI. La cosa che non capisco è che GoogleSheetAPI è un GoogleDriveAPI.
Mi sarei aspettato che Drive "usi" OAuth2 e che Sheet sia ospitato in una cartella di Drive.
Per come è fatta un oggetto di tipo GoogleSheetAPI dovrebbe avere, ad esempio, anche un metodo searchFile() che sembra strano.
Spesso mi dicono che scrivendo mi esprimo male, sono riuscito a spiegare i miei dubbi?
Ho iniziato a sviluppare questa libreria usando il metodo di autenticazione per dispositivi con input limitato che impone il limite di poter lavorare esclusivamente sui file creati direttamente dall'app autorizzata a prescindere dallo scope che l'utente concede.
Inoltre è possibile creare un nuovo spreadsheet sia con le API di Google Sheets che con le con le API di Google Drive e usando Drive è possibile contestualmente impostare il parent del nuovo foglio, mentre con Sheet non ci sono riuscito e dovevo fare una doppia chiamata HTTPS raddoppiando il tempo per la creazione di un nuovo file.
Ho quindi ereditato GoogleSheetAPI da GoogleDriveAPI perché mi sembrava la cosa più semplice per creare un nuovo spreadsheet qualora non fosse presente in Drive, ma comprendo le tue perplessità perché ovviamente la cosa dal punto di vista "formale" non quadra.
Forse potrei ereditare da GoogleOAuth2 usando i metodi di GoogleDriveAPI come una friend class magari rinominandoli in modo più coerente? Ad esempio searchFile() potrebbe diventare un:
const char* searchSpreadsheet(const char *fileName, const char* parentId) {
return GoogleDriveAPI::searchFile(fileName, parentId);
}
Ottimo, ero sicuro che una spiegazione c'era.
Ti scrivo dove vorrei arrivare, come me lo immagino io. Non so bene ancora il come, ma questo è un dettaglio implementativo… :-).
Uno spreadsheet è uno spreadsheet: oggi è GoogleSheet, domani un CSV. Per cui mi aspetto viva di vita propria ed abbia i metodi di un foglio di calcolo: addRow, addSheet, ecc.
Lo spreadsheet è un file (e spero anche in futuro), per cui deriverà da una classe file con i metodi relativi (open, close, ecc.)
Un file è contenuto in una lista di file (drive e/o direcotory), per cui una classe che gestisce la navigazione.
Tutta la parte Google* poi è un caso particolare di quanto sopra, per cui GoogleSheet sarà derivata da SpreadSheet, GoogleFile da File, ecc.
Google introduce l'autenticazione, che ha un suo processo e che mi aspetto sia unica per più operazioni, per cui la farei vivere come entità a parte e passerei un riferimento agli oggetti Google* durante la loro creazione.
Questo un po' lo scenario operativo immaginato:
// Start of Google* dependency
GoogleOAuth2 myAuth( "user", "password", "URL" );
GenericDrive* myDrive = new GoogleDrive ( myAuth );
GenericSheet* mySheet = new GoogleSheet( myDrive );
// End of Google* dependency
mySheet->Open( "mySheetName" );
mySheet->AddRow( ... );
mySheet->Close();
myDrive->Close();
delete mySheet;
delete myDrive;
Se (quando) il mondo intorno cambierà saranno da cambiare solo le prime righe.
Cosa ne pensi? Dal tuo punto di vista la cosa può avere un senso?
Ciao @carlo-banfi Scusa la latitanza, ma in questo periodo ho tutta una serie di impegni e come avrai capito queste cose le faccio nei ritagli di tempo.
Tornando a bomba, mi piace l'idea di passare il riferimento a GoogleOAuth2 per le diverse classi. Rende sicuramente tutto molto più chiaro e credo proprio che modificherò in tal senso.
Non sono invece del tutto convinto sul discorso del GoogleFile perché il concetto di file in realtà è piuttosto diverso da come siamo abituati su un PC: giusto per fare un esempio, in Drive posso avere due file che hanno lo stesso identico nome nella stessa cartella perché l'unica cosa che lo identifica in modo univoco è l'id; un file CSV ad esempio per Google Sheet non ha molto significato e lo puoi aprire direttamente sia con Sheet che con un editor di testo qualsiasi (aggiunto come estensione); quello che conta alla fine (credo) sono i metadata associati al file. Ci devo ragionare su con calma.
In un ottica di maggior semplicità di utilizzo, vorrei evitare ad esempio di creare un'istanza GoogleDrive se devo usare solo un GoogleSheet. In fondo l'API di Google Drive viene usata solo nel setup e solo nel caso in cui lo spreadsheet non sia stato già creato. Cosa ne pensi dell'idea della friend class per chiamare il metodo necessario (uno solo) all'occorrenza senza derivare tutta la classe da GoogleDrive?
Nessun problema per la latenza, queste sono cose che si fanno, appunto, nei ritagli di tempo. Non ho ben capito come intendi usare la classe friend (che mi è capitato di usare pochissime volte) potresti fare un veloce esempio chiarificatore senza tutta l'implementazione?
Ho aggiunto il supporto per alcune API di Calendar ed implementato il suggerimento in merito alla classe GoogleOAuth2
Per quanto riguarda il discorso della friend class, ho lasciato perdere perché si rendeva necessario rendere static i metodi di GoogleDrive` da usare complicando un po' tutto. Per ora sono ritornato all'ereditarietà della classe... ci devo riflettere meglio e fare ulteriori test, soprattutto sul come implementare.
Vorrei fermarmi a commenti costruttivi senza derivare in assillamento e rotture, per cui se passo il segno fermami pure.
Mi fa un po' paura la modalità di passaggio di auth alla classe GoogleDriveAPI. È passato un puntatore a un oggetto GoogleOAuth2 e GoogleDriveAPI se ne tiene una copia locale (m_auth), ma poi il distruttore della classe effettua un delete su quella copia, di fatto distruggendo l'oggetto originale all'insaputa di tutti. Questo viola la regola pratica secondo cui le cose si distruggono allo stesso livello in cui sono state create.
Implementandola così come riportato parzialmente di seguito dovresti essere protetto da eventuali invocazioni "errate", però non conoscendo il contesto non riesco a valutarne gli eventuali effetti collaterali.
class GoogleDriveAPI
{
public:
GoogleDriveAPI( const GoogleOAuth2& auth, …) : m_auth(auth) {}
~GoogleDriveAPI();
private:
GoogleOAuth2 & m_auth;
}
Può essere una cosa da tenere in considerazione?
@carlo-banfi i tuoi commenti sono sempre costruttivi anche e soprattutto quando rimarchi gli aspetti più formali che per me sono abbastanza sconosciuti ad essere sincero. Io non sono un programmatore (e per fortuna faccio altro di mestiere :) ), quindi affronto queste cose con un po' di impreparazione di base, ma tanta voglia di approfondire.
Ad esempio mi è sempre sfuggita nel dettaglio, la differenza nell'inizializzare le proprietà della classe come nel tuo esempio oppure fare tutto nell'implementazione del costruttore.
Per quanto riguarda il delete non ci avevo pensato proprio... in effetti io do per scontato che l'utente userà una classe per volta dichiarandola globale, un po' come in tutti gli esempi alla Arduino maniera, ma ovviamente la cosa non è assolutamente scontata ed è sicuramente plausibile che uno voglia usare due o più API contemporaneamente...
@cotestatnt bene, visto che non ho ancora passato il limite ne approfitto e mi dilungo un pochino su quanto già scritto.
Si parlava di passare un oggetto GoogleOAuth2 ad un oggetto GoogleDriveAPI durante la costruzione di quest'ultimo.
Dato che GoogleDriveAPI non esiste senza autorizzazione si potrebbe passare GoogleOAuth2 come riferimento direttamente durante la costruzione (const o meno dipende poi dall'implementazione, per ora facciamo finta che sia const).
La classe GoogleDriveAPI avrà dunque un costruttore che accetta un riferimento a GoogleOAuth2 e che memorizzerà internamente un una variabile membro.
Riscrivo il codice sopra aggiungendo un po' di commenti.
class GoogleDriveAPI
{
public:
/* Constructor that receive const reference to `GoogleOAuth2` object */
GoogleDriveAPI( const GoogleOAuth2& auth, …);
/* Destructor, does nothing */
~GoogleDriveAPI();
private:
/* Referene to authorization object */
const GoogleOAuth2 & m_auth;
/* Default constructor is declared but NOT implemented */
GoogleDriveAPI();
}
GoogleDriveAPI::GoogleDriveAPI( const GoogleOAuth2& auth ) : m_auth( auth )
{
/* m_auth is a reference, so there is no way to leave it unassigned */
}
GoogleDriveAPI::~GoogleDriveAPI()
{
}
Per come è stata scritta la classe qui sopra sarà possibile creare un oggetto GoogleDriveAPI solo passando un oggetto GoogleOAuth2 all'atto della sua costruzione.
Il fatto di dichiarare il default constructor senza implementarlo impedisce al compilatore di crearlo (e usarlo) in modo nascosto. Questo evita, ad esempio, di scrivere questo:
GoogleDriveAPI bad_initialized_object;
che genera errore in compilazione e dunque obbliga a specificare il parametro:
GoogleDriveAPI initialized_object( auth, … );
Per quanto riguarda invece la classe friend non è detto che sia necessario rendere i membri static, dato che ogni funzione friend può lavorare su riferimenti o puntatori all'oggetto desiderato, come nel seguente pessimo esempio:
/* Class with a friend function */
class MyClass
{
public:
/* This global function will be class-friend */
friend void FriendOfMyClass( MyClass& myObject );
private:
int m_private_member;
};
/* Friend function implementation */
void FriendOfMyClass( MyClass& myObject )
{
/* Here we can access to private members of MyClass*/
int test = myObject.m_private_member;
}