Correction de TP

a marqué ce sujet comme résolu.
Auteur du sujet

Salut,

Quand on debute dans un langage ca serait sympa qu’on puisse avoir des corrections, remarques, conseils… sur le resultat de son travail pratique, histoire de progresser, et d’ajuster ses mauvaises pratiques… je n’ai vu nulle part où partager le résultat de son tp, est-ce que j’ai mal regardé?

Édité par Taurre

+0 -0

Tu peux utiliser le forum ^^

En demandant des conseils et des avis.

ache.one                 🦹         👾                                🦊

+2 -0
Auteur du sujet

Alors voila ma mouture du puissance 4 (sans l’IA), si vous voyez des énormités n’hésitez pas :)

Aussi beaucoup disent que les variables globales c’est mal, mais en C ce sont des "file scope variables", un peu come des "module scope variables" en python, et en python on encourage justement à utiliser des variables de modules plutôt que de tout mettre dans des classes ("classitis"). J’utilise beaucoup de "file scope variables", vous en pensez quoi ?

#include <stdio.h>
#include <stdlib.h>
#include <memory.h>
#include <ctype.h>

#define LARGEUR_DAMIER 7
#define HAUTEUR_DAMIER 6

#define NOMBRE_JOUEURS 2

#define CASE_VIDE ' '
#define CASE_JOUEUR_1 'X'
#define CASE_JOUEUR_2 '0'

#define N 0
#define NE 1
#define E 2
#define ES 3
#define S 4
#define SO 5
#define O 6
#define ON 7

struct case_damier {
    char contenu;
    long index;
    long ligne;
    long colonne;
};

struct curseur {
    struct case_damier * soi;
    struct case_damier * haut;
    struct case_damier * haut_droit;
    struct case_damier * droit;
    struct case_damier * droit_bas;
    struct case_damier * bas;
    struct case_damier * bas_gauche;
    struct case_damier * gauche;
    struct case_damier * gauche_haut;
} curseur;

struct case_damier damier[LARGEUR_DAMIER * HAUTEUR_DAMIER];

struct joueur {
    char humain;
    char symbole;
};

struct joueur joueurs[NOMBRE_JOUEURS];

int nombre_joueurs = sizeof(joueurs) / sizeof(joueurs[0]);
int longueur_damier = sizeof(damier) / sizeof(damier[0]);

int numero_gagnant = 0;


void assigne_curseur(struct case_damier * soi) {

    curseur.haut = NULL;
    curseur.haut_droit = NULL;
    curseur.droit = NULL;
    curseur.droit_bas = NULL;
    curseur.bas = NULL;
    curseur.bas_gauche = NULL;
    curseur.gauche = NULL;
    curseur.gauche_haut = NULL;


    curseur.soi = soi;

    if(soi) {
        if (soi->ligne > 0)
            curseur.haut = &damier[soi->index - LARGEUR_DAMIER];

        if (soi->ligne > 0 && soi->colonne < LARGEUR_DAMIER - 1)
            curseur.haut_droit = &damier[soi->index - LARGEUR_DAMIER + 1];

        if (soi->colonne < LARGEUR_DAMIER - 1)
            curseur.droit = &damier[soi->index + 1];

        if (soi->ligne < HAUTEUR_DAMIER - 1 && soi->colonne < LARGEUR_DAMIER - 1)
            curseur.droit_bas = &damier[soi->index + LARGEUR_DAMIER + 1];

        if (soi->ligne < HAUTEUR_DAMIER - 1)
            curseur.bas = &damier[soi->index + LARGEUR_DAMIER];

        if (soi->ligne < HAUTEUR_DAMIER - 1 && soi->colonne > 0)
            curseur.bas_gauche = &damier[soi->index + LARGEUR_DAMIER - 1];

        if (soi->colonne > 0)
            curseur.gauche = &damier[soi->index - 1];

        if (soi->colonne > 0 && soi->ligne > 0)
            curseur.gauche_haut = &damier[soi->index - LARGEUR_DAMIER - 1];
    }
}

int gagnant() {
    char symbole = '\0';
    for (int i = 0; i < longueur_damier ; ++i) {
        if (damier[i].contenu == CASE_VIDE) continue;

        symbole = damier[i].contenu;

        for (int direction = 0 ; direction < 8 ; direction ++) {
            assigne_curseur(&damier[i]);
            int longueur = 1;
            while(curseur.soi->contenu == symbole) {
                switch (direction) {
                    case N:
                        assigne_curseur(curseur.haut);
                        break;
                    case NE:
                        assigne_curseur(curseur.haut_droit);
                        break;
                    case E:
                        assigne_curseur(curseur.droit);
                        break;
                    case ES:
                        assigne_curseur(curseur.droit_bas);
                        break;
                    case S:
                        assigne_curseur(curseur.bas);
                        break;
                    case SO:
                        assigne_curseur(curseur.bas_gauche);
                        break;
                    case O:
                        assigne_curseur(curseur.gauche);
                        break;
                    case ON:
                        assigne_curseur(curseur.gauche_haut);
                        break;
                    default:
                        assigne_curseur(curseur.haut);
                        break;
                }

                if(curseur.soi == NULL || curseur.soi->contenu != symbole)
                    break;

                if(++longueur == 4) {
                    if(symbole == CASE_JOUEUR_1) return 1;
                    if(symbole == CASE_JOUEUR_2) return 2;
                }
            }
        }
    }

    return 0;
}


void afficher_numeros_ligne(void) {
    for (int j = 1 ; j <= LARGEUR_DAMIER ; ++j) {
        printf("  %d ", j);
    }
    putchar('\n');
}

void afficher_separation_horizontale(void) {
    for (int j = 1 ; j <= LARGEUR_DAMIER ; ++j) {
        printf("+---");
    }
    puts("+");
}

void afficher_damier() {

    int ligne = 1;

    for (typeof(longueur_damier) i = 0 ; i < longueur_damier ; i++) {

        if (i == 0) {
            afficher_numeros_ligne();
            afficher_separation_horizontale();
        }

        printf("| %c ", damier[i].contenu);

        if (((i + 1) % LARGEUR_DAMIER) == 0) {
            puts("|");
            afficher_separation_horizontale();
            ++ligne;
        }
    }
    afficher_numeros_ligne();
}

int choix_ia(struct joueur pJoueur) {
    return 1;
}

int main() {

    char char_rec = '\0';

    joueurs[0].symbole = CASE_JOUEUR_1;
    joueurs[1].symbole = CASE_JOUEUR_2;

    for (typeof(longueur_damier) i = 0 ; i < longueur_damier ; i++) {
        damier[i].contenu = CASE_VIDE;
        damier[i].index = i;
        damier[i].colonne = i % LARGEUR_DAMIER;
        damier[i].ligne = i / LARGEUR_DAMIER;
    }

    afficher_damier();

    for (int i = 0 ; i < nombre_joueurs ; ++i) {

        printf("Joueur %d humain ou ordinateur ? (H/O)\n> ", i + 1);

        scanf(" %c", &char_rec);

        if (tolower(char_rec) == 'h') {
            joueurs[i].humain = '\1';
        }
    }

    struct joueur *joueur_actif = &joueurs[1];
    long colonne_jouee = -1;

    while(!(numero_gagnant = gagnant())) {

        colonne_jouee = -1;
        joueur_actif = (joueur_actif == &joueurs[0]) ? &joueurs[1] : &joueurs[0];

        if (joueur_actif->humain) {
            printf(
                    "Au joueur %ld (%c) de jouer (un chiffre entre 1 et 7)\n> ",
                   ((joueur_actif - &joueurs[0]) + 1),
                   joueur_actif->symbole
            );

            while (1) {
                scanf(" %c", &char_rec);

                if (char_rec >= '1' && char_rec <= '7') {
                    colonne_jouee = strtol(&char_rec, NULL, 10);
                    if(damier[colonne_jouee -1].contenu == CASE_VIDE)
                        break;
                    else {
                        puts("Colonne remplie, veuillez en choisir une autre.");
                    }
                } else {
                    puts("Chiffre invalide.");
                }
            }
        } else {
            colonne_jouee = choix_ia(*joueur_actif);
        }

        assigne_curseur(&damier[colonne_jouee -1]);

        while(curseur.bas != NULL && curseur.bas->contenu == CASE_VIDE)
            assigne_curseur(curseur.bas);

        curseur.soi->contenu = joueur_actif->symbole;

        afficher_damier();

    }

    printf("Le joueur %d (%c) a gagné", numero_gagnant, joueur_actif->symbole);

    return EXIT_SUCCESS;
}

Édité par Meithal

+0 -0

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

Salut,

Aussi beaucoup disent que les variables globales c’est mal, mais en C ce sont des "file scope variables", un peu come des "module scope variables" en python, et en python on encourage justement à utiliser des variables de modules plutôt que de tout mettre dans des classes ("classitis"). J’utilise beaucoup de "file scope variables", vous en pensez quoi ?

Meithal

De manière générale, ce sont surtout les variables partagées entre différents modules (comme errno) qui peuvent poser problèmes, parce qu’il est alors plus difficile de relire le code et qu’il y a risque de collisions (plusieurs variables partagées avec le même nom).

Les variables locales ne posent pas ce problème puisque leur définition est présente dans le fichier source et que leur portée (et liaison, voir ce tuto pour plus de détails) est limitée au fichier où elles sont définies.

C’est une pratique courante et à mon sens louable puisqu’elle permet de simplifier l’écriture du code dans le cas où des données doivent être partagées entre différentes fonctions. Tu remarqueras d’ailleurs que les corrections proposées en utilise une. ^^

Donc non, ce n’est sûrement pas une mauvais pratique, elle doit juste être employée quand cela s’avère bénéfique, c’est tout. Dans ton cas, je dirais que cela n’est pas nécessaire pour nombre_joueurs et longueur_damier, qui sont finalement des contantes entières, ainsi que pour numero_gagnant qui a priori n’est pas utilisée par suffisamment de fonctions pour justifier d’augmenter sa portée.

Alors voila ma mouture du puissance 4 (sans l’IA), si vous voyez des énormités n’hésitez pas :)

Meithal

Tout d’abord et de manière générale, ton code est clair et concis. Tu fais un bon usage des fonctions en séparant ton code en petites parties réutilisables et tu as pris soin de nommer adéquatement tes variables et fonctions. On voit que tu as pris le temps de réfléchir et de travailler à ton code, c’est une excellente habitude. :)

Sinon, après une première lecture, je peux formuler deux remarqes (en plus de celle concernant les variables locales).

  1. Tu ne vides pas le tampon du flux d’entrée lorsque tu récupères des données auprès de l’utilisateur (ligne 215 et 238) et, plus important, tu ne vérifies pas si celles-ci sont valides. Entre un chiffre ou une lettre autre que 'H', 'h', 'o' ou 'O' et tu verras qu’il y a un petit soucis. :p
  2. Tu inclus l’en-tête <memory.h>, qui est un en-tête non standard et a priori non nécessaire.

#JeSuisArius

+0 -0
Auteur du sujet

D’accord, j’ai pris tes remarques en compte et j’ai enlevé quelques variables en trop, et vérifié que je regardais bien de vider la memoire de mon buffer (si j’ai bien compris), j’ai aussi utilisé quelques astuces trouvés dans l’article de ache sur le C, notamment l’opérateur de virgule

J’ai rajouté une IA, elle marche pas tous les coups, mais est capable de trouver un coup gagnant o_O

Source sur gist

Édité par Meithal

+0 -0

Hello,

  • case_damier a vraiment besoin de connaître sa ligne et sa colonne ?
  • pourquoi pas un tableau 2D pour damier ?
  • On sent qu’il y a une volonté d’isoler le code moche dans assigne_curseur (ce qui est plutôt une bonne chose), mais je reste convaincu qu’il n’y a pas à répéter autant de code (c’est vraiment un truc qui me fait frémir à titre perso, mais c’est peut-être pas si grave). Je crois de mémoire qu’on avait eu un débat à ce sujet avec @Taurre (et je maintiens mon avis :p). Voilà par exemple comment j’imaginerais itérer sur toutes les directions :
1
2
3
4
for (int dligne = -1; dligne <= 1; ++dligne)
  for (int dcolonne = -1; dcolonne <= 1; ++dcolonne)
    if (dligne != 0 || dcolonne != 0)
      // ...

ÉDIT : Sinon y a aussi le plus flexible :

1
2
3
4
5
const int dirlin[] = {-1, -1, -1, 0, 0, +1, +1, +1};
const int dircol[] = {-1, 0, +1, -1, +1, -1, 0, +1};

for (int dir = 0; dir < NB_DIRS; ++dir)
  // ... dirlin[dir], dircol[dir]

et hop, plus besoin de assigne_curseurs, directions, etc. Ça fait sauter 50 lignes de code je pense (et c’est plus clair de mon pdv). ^^

Édité par Lucas-84

Je crois de mémoire qu’on avait eu un débat à ce sujet avec @Taurre (et je maintiens mon avis :p).

Lucas-84

Yep, au sujet du corps de la fonction calcule_nb_jetons_depuis du TP si ma mémoire est bonne.

1
2
3
4
5
6
7
max = calcule_nb_jetons_depuis_vers(pos, 0, 1, jeton);
max = umax(max, calcule_nb_jetons_depuis_vers(pos, 1, 0, jeton) + \
calcule_nb_jetons_depuis_vers(pos, -1, 0, jeton) - 1);
max = umax(max, calcule_nb_jetons_depuis_vers(pos, 1, 1, jeton) + \
calcule_nb_jetons_depuis_vers(pos, -1, -1, jeton) - 1);
max = umax(max, calcule_nb_jetons_depuis_vers(pos, 1, -1, jeton) + \
calcule_nb_jetons_depuis_vers(pos, -1, 1, jeton) - 1);

Et pour ma part je ne maintiens pas mon avis : vive les boucles. :p

D’accord, j’ai pris tes remarques en compte et j’ai enlevé quelques variables en trop, et vérifié que je regardais bien de vider la memoire de mon buffer (si j’ai bien compris),

Meithal

Tu vérifies à présent bien si la saisie est valide, mais tu as toujours un problème au niveau de la temporisation du flux d’entrée (stdin). Je t’invite à relire cette section et cette autre section du cours. ;)

Édité par Taurre

#JeSuisArius

+0 -0
Auteur du sujet

Je suis allé sur differents IRC et autre service de messagerie instantanée pour glaner d’autres conseils et je les ai intégrés pour mettre à jour mon script.

Notamment on m’a conseillé les enumerations plutot que les define, et aussi que le fait que curseur.soi pouvait etre NULL est deroutant, au lieu de rajouter un test, un assert suffisait pour verifier que on dirige le curseur correctement. En effet dans la fonction gagnant() on deplacait le curseur jusqu’à ce qu’il soit NULL, ce qui veut dire que le curseur est dans un mur, et on verifie à posteriori. Avec l’assert, on force les vérifications à priori et sans doute un code plus robuste.. merci à "jo_link_noir" pour ces suggestions (si ca le derange d’être mentionné qu’il me le dise et je ferai le nécessaire).

Ensuite à l’usage j’ai decouvert que le programme a deux gros defauts :

Premièrement, lorsque l’intelligence pondère une colonne, il va regarder dans les 8 directions pour trouver la plus longue chaine, c’est cette longueur qui va être utilisée pour trouver le poids de cette colonne, mais dans une configuration comme celle-ci

1
x xx

En vérifiant la deuxième colonne, il va trouver depuis le haut en tournant dans le sens des aiguille un poids de 1, 1, 3, 1, 1, 1, 2, 1, et donc le poids de cette colonne est de 3, alors qu’en fait elle est de 4.

J’ai résolu ce problème en additionnant les directions opposées.

Deuxième problème, l’intelligence quand elle voit une colonne avec un poids egal à sa meilleure cible va faire un test de une chance sur 2, ca veut dire que les cases "à droite" sont plus avantagées, pour faire en sorte que l’IA joue plus souvent au milieu, sa meilleure colonne sera au centre par defaut et la probabilité en cas de poids égal est diminuée d’une valeur arbitraire, donc l’IA jouera plus souvent au milieu.

Sinon j’ai (enfin) compris le problème quand on ne vide pas le tampon, j’ai trouvé plein de techniques sur internet j’espère que celle que j’ai retenue est suffisamment solide, bizarre qu’il faille faire autant de choses pour lire un seul caractère :magicien:

  • case_damier a vraiment besoin de connaître sa ligne et sa colonne ?
  • pourquoi pas un tableau 2D pour damier ?
  • On sent qu’il y a une volonté d’isoler le code moche dans assigne_curseur (ce qui est plutôt une bonne chose), mais je reste convaincu qu’il n’y a pas à répéter autant de code (c’est vraiment un truc qui me fait frémir à titre perso, mais c’est peut-être pas si grave).
Lucas-84
  • Oui ces variables sont accédées à plusieurs reprises
  • Dans une aire de jeu quadrillée un tableau de tableau peut fonctionner, mais si l’aire de jeu est fait de hex, ou des formes aléatoires, j’ai pris l’habitude de stocker les informations d’environnement de manière indépendante de la configuration actuelle
  • C’est vrai que la fonction assigne_curseur peut être améliorée, j’espère que les modifications que j’ai faites vont dans le bon sens

https://gist.github.com/Meithal/265c6498f729275bc10da3b9bb69260e/revisions#diff-e48c5ac669830310a57e56bb8e47c60a

Édité par Meithal

+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