Amélioration possible code C with classes

L’auteur de ce sujet a trouvé une solution à son problème.
Auteur du sujet

Bonjour ,

Voilà je viens de terminer un gros bout de code pour un projet de réveil matin .

Je suis en phase d’apprentissage, donc j’aimerais remettre sur le métier mon ouvrage. Je pense qu’il y a sans doutes des façons plus propres/élégantes/stables/rapides de faire ce code. J’aimerais avoir un regard critique la dessus.

Impératif : il ne faut pas que le code soit bloquant ( boucle ) ou alors sur un temps très bref, c’est pourquoi je n’ai pas utilisé de boucle.

je mets le lien git pour éviter de surcharger la page du post avec les trois fichiers : ino .h .cpp

Aussi, j’ai créé pour l’écran LCD une méthode _scroll_one_line.

Elle permet de faire défiler un texte ( peu importe la taille )sur une seule ligne de l’écran. Je n’est pas vu, dans aucun tuto sur la lib. LiquidCrystal une fonction similaire.

J’aimerais rendre cette fonction portable, mais je suis bloquer par ma variable globale qui prend la taille de l’écran et je ne peu pas récupéré les attributs de la lib. car il sont privé. je suis preneur de solution comme cela humblement je la réécrirai pour que quiconque puisse l’utilisé.

Le lien git du projet: Reveil_matin

Merci

Un bon travail se voit, un excellent est invisible.

+0 -0

Plusieurs questions:

  • Pourquoi Screen::Setup ? À quoi sert-elle ?
  • À quoi sert Screen::state_alarm ? Pourquoi volatile ?

Et sinon, ça manque de const:

  • display_home devrait prendre une référence constante, pas un pointeur.
  • text_to_scroll devrait être un pointeur constant.
  • La plupart des fonctions membres devraient être const void display_home(DateTime const& date) const

Pour l’implémentation des fonctions:

  • Je ne vois pas l’intérêt d’utiliser calloc pour des tableaux de petite taille statiques. Retourner une structure contenant un tableau de la dite taille serait à mon sens beaucoup mieux. Une sorte de std::array<char, N>, mais cela n’est pas dispo sur arduino… Le mieux serait quand même de faire une petite classe représentant une chaîne de caractère qui connaît sa taille et virer tout les strlen. Je suis assez surpris qu’il n’en existe pas sur arduino.
  • char* scroll_end = strchr(text_to_scroll, text_to_scroll[0]) -> char* scroll_end = text_to_scroll ?
  • Les fonctions de format me semblent bien compliquées pour quelque chose qui pourrait se faire en quelques lignes:
sprintf(buf, "%02d:%02d:%02d", hh, mm, ss);

// et
char const* days_of_the_week[] {"Lundi", etc, "Err: Jour"}; // on pourrait se passer de la valeur 
char const* months[] {....};
sprintf(buf, "%s %d %s %d", days_of_the_week[max(dayOfTheWeek, 7)], day, months[max(month, 12)], month);

Concernant les globales, c’est simple, il suffit de les mettre en paramètre de fonction ou de faire une classe qui contient des variables membres et expose scroll_one_line.

+0 -0
Auteur du sujet

@jo_link_noir, merci d’avoir pris le temps de lire le code.

Pour répondre a tes questions :

  • Pourquoi Screen::Setup ? À quoi sert-elle ?
  • À quoi sert Screen::state_alarm ? Pourquoi volatile ?

Setup ,Oui je pense quelle ne va pas servir à grand chose, elle est voué à initialiser les variables de ma classe. ( au cas ou il y aurais des ajouts plus tard, module internet ect…)

State_alarm récupère l’état d’un interrupteur et en fonction devras faire une petit icone sur l’écran home d’ou le volatile. ( je n’ai pas implémenté la fonction encore )

Et sinon, ça manque de const:

  • display_home devrait prendre une référence constante, pas un pointeur.
  • text_to_scroll devrait être un pointeur constant.
  • La plupart des fonctions membres devraient être const void display_home(DateTime const& date) const

Je vais approfondir ce passage la… Pourquoi un const est important, au delà qu’il n’est pas modifiable? J’ai l’impression de répondre à la question en la posant, mais je cherche surtout à savoir quel serais le problème s’il on ne l’utilisais pas.

Pour l’implémentation des fonctions:

  • Je ne vois pas l’intérêt d’utiliser calloc pour des tableaux de petite taille statiques. Retourner une structure contenant un tableau de la dite taille serait à mon sens beaucoup mieux. Une sorte de std::array<char, N>, mais cela n’est pas dispo sur arduino… Le mieux serait quand même de faire une petite classe représentant une chaîne de caractère qui connaît sa taille et virer tout les strlen. Je suis assez surpris qu’il n’en existe pas sur arduino.

Oui l’heure effectivement est statique mais pas la date, l’utilisation de calloc viens du fait que j’ai eu beaucoup de mal a faire retourner un char par ma fonction. J’ai vu cette solution sur un forum.

Classe pour virer les strlen pourquoi ? j’ai déjà lu qu’il valais mieux éviter de les mettre dans les conditions il y à un problème avec eux ?

  • char* scroll_end = strchr(text_to_scroll, text_to_scroll[0]) -> char* scroll_end = text_to_scroll ?

LOool le saint graal de mon programme tout ma fierté ( soit gentil avec cette fonction elle ma empêcher de dormir ) :D

plus sérieusement c’est la seule solution simple que j’ai trouvé pour que mon texte trop long ~50 caractères disparaisse vert la gauche sans calcul trop compliqué. J’avoue ma variable est mal nommé et c’est pas très propre mais pas eu de meilleures idées. je pense que je me suis un peu trop pris la tête mais ta solution est évidente. :magicien:

  • Les fonctions de format me semblent bien compliquées pour quelque chose qui pourrait se faire en quelques lignes:
sprintf(buf, "%02d:%02d:%02d", hh, mm, ss);

// et
char const* days_of_the_week[] {"Lundi", etc, "Err: Jour"}; // on pourrait se passer de la valeur 
char const* months[] {....};
sprintf(buf, "%s %d %s %d", days_of_the_week[max(dayOfTheWeek, 7)], day, months[max(month, 12)], month);

Mais carrément j’avais oublié ça !!! :-°

Concernant les globales, c’est simple, il suffit de les mettre en paramètre de fonction ou de faire une classe qui contient des variables membres et expose scroll_one_line.

ok je vais tester.

Source:jo_link_noir

Un bon travail se voit, un excellent est invisible.

+0 -0

Cette réponse a aidé l’auteur du sujet

State_alarm récupère l’état d’un interrupteur et en fonction devras faire une petit icone sur l’écran home d’où le volatile. ( je n’ai pas implémenté la fonction encore )

Je doute quand même que volatile soit justifié, mais à voir.

Je vais approfondir ce passage la… Pourquoi un const est important, au delà qu’il n’est pas modifiable? J’ai l’impression de répondre à la question en la posant, mais je cherche surtout à savoir quel serais le problème s’il on ne l’utilisais pas.

Il n’y a pas de réel problème, mais un const offre plus de garantie pour celui qui implémente la fonction: il ne peut pas modifier l’état des variables constante, et pour celui qui l’utilise: il sait qu’il n’y aura pas de modification des états. Dans le cas contraire, il y aura une erreur à la compilation. Pour cette même raison, seules les fonctions membres const sont accessibles depuis une variable const. Si const n’est pas respecté de bout en bout, il va encore une fois y avoir une erreur de compilation.

D’ailleurs, une chaîne de caractère de la forme "bla bla" est un tableau constant. Le passer à une fonction qui prend un char* devrait au minimum retourner un avertissement: cela n’est pas autorisé en C++, mais pour des raisons historiques lié au C, les compilateurs l’autorisent encore.

Oui l’heure effectivement est statique mais pas la date, l’utilisation de calloc viens du fait que j’ai eu beaucoup de mal a faire retourner un char par ma fonction. J’ai vu cette solution sur un forum.

Pas un char, mais un tableau. Les tableaux du C sont une plaie et les retourner dans une fonction revient à retourner un pointeur et non pas le tableau complet. C’est pour cela qu’il existe dans le standard un std::array. Grosso modo, ce n’est qu’une structure qui contient un tableau. Mais contrairement au tableau, une structure peut être copiée.

struct SecretNumber
{
  char data[5];
};

SecretNumber secret()
{
  SecretNumber secret_number;
  // des trucs
  return secret_number; // pas possible si on utilise directement un tableau C
}

Avec des templates, on peut spécifier la taille du tableau interne

template<unsigned N>
struct Array
{
  char data[N];
};

using TimeString = Array<16>;
using DateString = Array<50>;


TimeString Screen::_format_time(int hh, int mm, int ss)
{
  TimeString buf;
  sprintf(buf.data, "%02d:%02d:%02d", hh, mm, ss);
  return buf;
}

Classe pour virer les strlen pourquoi ? (1) j’ai déjà lu qu’il valais mieux éviter de les mettre dans les conditions il y à un problème avec eux ?

  1. C’est plutôt éviter de les faire plusieurs fois alors qu’on peut conserver le résultat dans une variable et la réutiliser.

Mais à mon sens, le vrai problème est de calculer la taille de la chaîne alors que cette taille est presque systématiquement connue. Par exemple, la famille des printf retourne le nombre de caractère écrit, cela correspond à la taille (sauf erreur si la taille du buffer est insuffisant, ce qui n’est pas le cas ici). Ta fonction display_home fait un strlen de today_date, appel _scroll_one_line qui fait également un strlen de today_date. Il y a 2 appels à strlen, alors que la taille aurait pu être conservée dès le début. Même si le compilateur va probablement virer le second appel et peut-être même le premier, il ne pourra pas toujours faire ce genre d’optimisation ce qui ferra tourner le programme pour rien.

Quelque chose de vraiment basique ressemblerait à

template<unsigned N>
struct StaticString
{
  char data[N];
  unsigned size;
};

using TimeString = StaticString<16>;


TimeString Screen::_format_time(int hh, int mm, int ss)
{
  TimeString buf;
  buf.size = sprintf(buf.data, "%02d:%02d:%02d", hh, mm, ss);
  return buf;
}
+1 -0
Auteur du sujet

Merci ! J’ai commencer a faire les modif et mon code c’est amaigri a vu oeil! ;)

Maintenant je vais essayer d’intégrer les struct et template mais avant je doit faire un peu de lecture ce sont des notions que je ne maîtrise pas.

Un bon travail se voit, un excellent est invisible.

+0 -0
Auteur du sujet

Bonjour ,

Non , mais je suis pas bien remis du réveillon j’ai déclarer la structure dans la classe :euh:

Avec des templates, on peut spécifier la taille du tableau interne

template<unsigned N>
struct Array
{
  char data[N];
};

using TimeString = Array<16>;
using DateString = Array<50>;


TimeString Screen::_format_time(int hh, int mm, int ss)
{
  TimeString buf;
  sprintf(buf.data, "%02d:%02d:%02d", hh, mm, ss);
  return buf;
}

Source:jo_link_noir

J’essaie en vain d’utiliser ta technique mais je mon ide se plaint voici comment je l’utilise:

Dans le .h

class Screen : public LiquidCrystal_I2C
{
  private:

      template<unsigned N>
      struct _Array
      {
          char data[N];

      };
      using _TimeString = _Array<20>;
      using _DateString = _Array<50>;

      _TimeString _format_time(int const hh, int const mm, int const ss);
      char* _format_date(int const dayOfTheWeek, int const day, int const month, int const year);
      void _scroll_one_line(char* text_to_scroll, int line);
      
  public:
    Screen(uint8_t lcd_Addr, uint8_t lcd_cols, uint8_t lcd_rows);
    void Setup(bool aState_alarm);
    void display_home(DateTime* date);
    
    //void display_test(char *message);
    volatile bool state_alarm;
};
#endif

Tous se passe relativement bien Mais dans le .cpp il n’est pas content

#include "display_lcd.h"


_TimeString Screen:: _format_time(int hh, int mm, int ss)
{
    /*
    On formate l'heure s'il elle arrive sous la forme de = 12: 4: 5
    la fonction affiche l'heure sous ce format = 12:04:05
        
    */

    _TimeString buff;
    sprintf(buff.data, "%2d:%02d:%02d", hh, mm, ss);
    return buff;
}

voici ce qu’il me dit : Gravité Code Description Projet Fichier Ligne État de la suppression Erreur (active) E0020 identificateur "_TimeString" non défini firmware_reveil C:\Users\plop\OneDrive\Documents\Arduino\Reveil_matin\firmware_reveil\display_lcd.cpp 5 Erreur (active) E0147 déclaration incompatible avec "Screen::_TimeString Screen::_format_time(int hh, int mm, int ss)" (déclaré à la ligne 27 de "C:\Users\plop\OneDrive\Documents\Arduino\Reveil_matin\firmware_reveil\display_lcd.h") firmware_reveil C:\Users\plop\OneDrive\Documents\Arduino\Reveil_matin\firmware_reveil\display_lcd.cpp 5 Erreur (active) E0304 aucune instance de fonction surchargée "Screen::print" ne correspond à la liste d'arguments firmware_reveil C:\Users\plop\OneDrive\Documents\Arduino\Reveil_matin\firmware_reveil\display_lcd.cpp 169

Édité par Marycha

Un bon travail se voit, un excellent est invisible.

+0 -0
Vous devez être connecté pour pouvoir poster un message.
Connexion

Pas encore inscrit ?

Créez un compte en une minute pour profiter pleinement de toutes les fonctionnalités de Zeste de Savoir. Ici, tout est gratuit et sans publicité.
Créer un compte