Salut!

J'ai fait l'exercice du cours de C# où il faut faire un tirage. Mon code est fonctionnelle, mais deux de mes correcteur me disent qu'il n'est pas optimisé parce que mes méthodes appelle d'autres méthode. J'aimerais avoir un peu plus d'explication, est-ce que c'est vraiment une mauvaise chose? Si oui pourquoi? Pour vous expliquer ce que j'ai fait, j'ai fait 3 méthode: random(), loto() et afficher(). random() retourne un nombre aléatoire entre 1 et 49, loto() retourne un tableau de 7 numéro qui ne se répète pas généré par random() et finalement afficher() affiche chaque élément du tableau fait par loto(). Selon moi, dans un plus gros projet, ça pourrait être utile si j'ai a généré d'autre numéro aléatoire ou a créé d'autre tableau.

Merci d'avance pour vos réponse.

static void Main(string[] args)
{
    afficher();                                 //appelle la méthode afficher() qui affiche les élément du tableau
}

static void afficher()
{
    int[] tableLoto = loto();                           //tableLoto contient le tableau de la méthode loto()
    for (int i = 0; i < tableLoto.Length; i++)
    {
        Console.WriteLine(tableLoto[i]);                //on affiche chaque élément du tableau
    }
}

static int[] loto()
{
    int[] tableau = new int[7];
    for(int i = 0; i < tableau.Length; i++)
    {
        int number = random();
        while (tableau.Contains(number))              //tant que le tableau contient déjà le numéro, on rappele la méthode random()
        {
            number = random();
        }
        tableau[i] = number;                          //une fois confirmer que le numéro n'est pas dans le tableau on l'ajoute
     }
    return tableau;
}

static int random()
{
    Random random = new Random();
    int number = random.Next(1, 49);            //numéro aléatoire entre 1 et 49
    return number;
}

Hello,

L'intérêt d'une méthode effectivement comme tu le soulignes toi même c'est pas seulement d'organiser le code un peu plus proprement, c'est aussi de pouvoir être réutilisable.

Le problème avec tes méthodes, c'est qu'elles ne prennent aucun paramètre, ne sont influencées par rien du tout, aucune portion de ta méthode ne va pouvoir "varier" et ça c'est pas terrible à part si effectivement tu as besoin de plusieurs fois un random entre 1 et 49, ou plusieurs fois de faire un tirage de 7 boules ou d'afficher une une même grille de loto plusieurs fois.

Si tu mettais des paramètres, par exemple une borne min/max à ton random, ou encore un choix plus étendu de taille de tableau. Alors là, ok, tes méthodes auraient une valeur ajoutée par rapport à un gros bloc de code autre que, "mon code est bien rangé". Le petit plus Periglioni, on utilise souvent des verbes pour les méthodes pour décrire l'action qu'il s'y passe, afficher c'est très bien, les deux autres, un peu moins

Je suis d'accord avec tes correcteurs sur le fait que tes méthodes n'ont pas besoin de s'appeler les unes les autres, tu peux appeler deux méthodes, une pour récupérer ton tableau de loto qui prend tous les paramètres et une pour afficher les résultats qui prendrait ton tableau en paramètre. Le fait d'avoir des méthodes les unes dans les autres comme ça sans "réél" intérêt gène un peu à la compréhension de ton code.

Bonne continuation !

-
Edité par Pyrobolser 13 septembre 2016 à 5:29:22

Faisant les choses dans l'ordre :

Est-ce que découper le code en méthode simple est une bonne chose ? oui, dans la mesure où c'est fait judicieusement tout de même.

Est-ce que le code est "optimisé" (à dire vrai je trouve le terme mal choisi dans le contexte j'aurais préféré un "bien découpé" à la place) ? non pas vraiment, comme l'a déjà souligné Pyrobolser.

Maintenant allons un peu plus loin dans le détail (en rectifiant quelques autres problèmes du code) :

La méthode "random" présente un "gros" souci, tu crées à chaque fois un nouvel objet Random, comme par défaut Random se sert de l'heure système pour initialiser son générateur de nombre pseudo-aléatoires, cela signifie que pour la même heure système 2 objets Random généreront les mêmes nombres.

Comme il y a de fortes chances que le code s'exécute suffisamment vite tu vas souvent créer des mêmes Random et donc générer les mêmes nombres, qui seront bloqués par le Contains dans loto, mais va ralentir le code (parce qu'il faudra que l'heure système ait changée pour obtenir un nombre différent).

D'une manière générale on ne créé un objet Random qu'une seule fois et on le réutilise par conséquent random devrait plutôt utiliser un objet Random créé au niveau de la classe.

// j'ajoute readonly parce qu'il n'a pas vocation à être changé par la suite
private static readonly Random monRandom = new Random ();

static int random ()
{
    return monRandom.Next (1, 50);
}

Tu noteras au passage que j'utilise 50 et pas 49, parce que la documentation de Next précise bien que le second paramètre (la borne max) est exclue.

Ce que l'on voit aussi c'est que là la méthode fait juste une seule ligne, du coup on peut se poser la question de son utilité. Il n'y a pas forcément de réponse absolue, en gros il y a 2 chemins possibles là soit on se dit que ça sert à rien d'avoir une méthode et on utilise directement monRandom.Next au niveau de l'appelant (en définissant 1 et 49 comme des constantes ça permet d'être plus clair)

private const int NombreMin = 1;
private const int NombreMax = 49;

static int[] loto ()
{
    // ...
    int number = monRandom.Next (NombreMin, NombreMax + 1); // ne pas oublier le + 1
    // ...
}

ou alors on peut garder la méthode, qui sait un jour on génèrera les nombres de manière aléatoire via une autre méthode qu'avec Random et ce jour là on aura juste à changer cette méthode et le reste du code ne verra même pas la différence.

Après la méthode loto, il est dommage (même si ce n'est pas faux) dans un sens de créer un nombre au pif entre 1 et 49 puis vérifier si on l'a pas déjà (si on a pas de bol et qu'il me donne 1 million de fois le même nombre d'affilé on va rester bloqué là un "moment") alors que dès le départ on a un ensemble fixé de valeurs et qu'on en veut juste 7 parmi elles.

Quand on a ce type de scénario, il est souvent plus judicieux (mais peut-être plus complexe selon le niveau que tu as) d'emblée de jeu de faire cet ensemble de valeur au complet, et juste les mélanger et en prendre autant qu'on en a besoin ; soit ici mélanger un tableau contenant les nombres 1 à 49, le mélanger et prendre les 7 premiers (je renvoie à ceci au besoin)

Enfin la méthode afficher, son vrai problème c'est qu'elle ne fait pas qu'afficher, elle créé aussi la grille, alors que ce n'est pas son rôle (une méthode ne devrait avoir qu'un seul rôle). Là en l'état comment on utilise la grille de loto si on veut en faire autre chose que l'afficher (c'est probablement pas le but de l'exo et c'est dommage dans un sens) parce qu'à la fin de la méthode afficher ta grille est perdue (c'est un élément local de la méthode afficher).

Si le rôle de la méthode afficher est d'afficher n'importe quelle grille de loto, c'est qu'elle doit l'attendre comme paramètre (autrement dit, demander à ce qu'on lui fournisse la grille, pas la fabriquer elle-même).

Si je résume un peu le tout (outre les autres remarques de Pyrobolser concernant le nommage des choses), une méthode doit (essayer de) n'avoir qu'un seul rôle et avoir le moins de dépendances possibles ; là afficher dépend de la méthode loto alors que dans l'absolu cette méthode afficher pourrait servir à afficher n'importe quel tableau d'entier si on le lui donnait elle ne devrait pas voir la différence (et ne devrait même pas savoir qu'elle sert pour du loto).

La méthode loto quant à elle dépend d'une forme de génération aléatoire et donc a une dépendance sur ça, reste à savoir si on veut un couplage fort sur une méthode particulière (utiliser monRandom dans la méthode) ou un couplage faible, utiliser la méthode random qui elle cache le comment c'est généré et donc peut être modifié sans que loto le sache (et s'en soucie d'ailleurs).

Tout cela étant dit à part le souci du Random local (dont beaucoup de ceux qui débutent font l'erreur) et afficher qui génère la grille, ça reste du code pas trop mal (et ça change de ceux qui font tout dans le Main)

Salut,

Beaucoup de choses ont été dites par mes deux collègues ci-dessus. Je vais juste ajouter quelques petites remarques :

Les noms de méthode sont censés commencer par un verbe (comme dit ci-dessus). De plus, ils sont censés commencer par une majuscule. Enfin, afin de garder une cohérence entre ton code, le code du framework et du code d'une bibliothèque tierce que tu voudrais utiliser, afin également d'éviter tout problème d'accent, je te conseille de nommer tout (classes, méthodes, propriétés, champs...) en anglais.

La doc est très importante. En plus, celle de .NET (MSDN) est bien faite. La lire t'aurait permis d'éviter 2 erreurs : 1) Créer un objet random local 2) supposer que le deuxième paramètre était une borne incluse. En revanche, le moteur de recherche du site n'est pas efficace. Je te conseille plutôt de Googler "C# NomDeLaClasse" et ce sera le premier (éventuellement deuxième) résultat.

Comme dit Sehnsucht, plutôt que de créer un nouveau nombre et de vérifier s'il y est déjà, mieux vaut créer une collection avec tous les éléments. En revanche, mélanger une collection, ça marche, mais il faut toujours vérifier que l'algo de mélange est bien uniforme. Il est plus simple de prendre le nième élément (n nombre aléatoire compris entre 0 (inclus) et la taille de ta collection (exclue)), puis le retirer de ta liste.

Quant à ce que disent des correcteurs : Qu'appellent-ils "optimiser" ? Au niveau des performances, un appel à une méthode est très peu coûteux. Je viens de faire le test sur une méthode (comme dans ton cas) statique, sans paramètre, qui renvoie une valeur. L'appel, sur mon core i7, prend au alentours de 2 ns. 2 MILLIARDIÈMES de seconde. Autrement dit : RIEN.

Pour la question principale, je crois que je comprenais déjà bien le concept des méthodes, c'est simplement les réponses que j'ai eu qui m'ont mis dans le doute. Pour ce qui est de l'utilité de mes méthodes, c'est surtout que dans le contexte de l'exercice les données son plus ou moins fixe donc sur le coup j'ai pas pensé a mettre de paramètres, mais maintenant que j'y pense c'est vrai que ça aurais été plus logique.

Sinon, merci beaucoup pour les commentaires et explications supplémentaire tous très bien expliqué! Je reviendrais ici si j'ai d'autre question

Merci encore!

-
Edité par homer94 12 octobre 2022 à 20:45:16

Est-ce qu'on est encore en 2016?
Il y a un sujet semblable ici:

https://openclassrooms.com/forum/sujet/loto-5

Ce serait mieux si tu créais ton propre sujet.

@homer94 Bonsoir, merci de créer votre propre sujet et de ne pas squatter celui des autres. Merci également de nous fournir le code que vous avez écrit inséré sur le forum à l'aide du bouton code </> de la barre d'outil.

Déterrage

Citation des règles générales du forum :

Avant de poster un message, vérifiez la date du sujet dans lequel vous comptiez intervenir.

Si le dernier message sur le sujet date de plus de deux mois, mieux vaut ne pas répondre.
En effet, le déterrage d'un sujet nuit au bon fonctionnement du forum, et l'informatique pouvant grandement changer en quelques mois il n'est donc que rarement pertinent de déterrer un vieux sujet.

Au lieu de déterrer un sujet il est préférable :

• soit de contacter directement le membre voulu par messagerie privée en cliquant sur son pseudonyme pour accéder à sa page profil, puis sur le lien "Ecrire un message"
• soit de créer un nouveau sujet décrivant votre propre contexte
• ne pas répondre à un déterrage et le signaler à la modération

Je ferme ce sujet. En cas de désaccord, me contacter par MP.