Remplacement mots par lettre ou chiffre dans les messages réseaux

david540 commented 7 years ago

Listing des choses effectuées:

Remplacement des demandes réseaux (ex "TOP") par un chiffre ou une lettre, tout en gardant une bonne lisibilité du code à l'aide de dictionnaire python côté client et constantes java côté serveur -Remplacement des actions par un chiffre qui correspond au numéro de l'action dans l'ordre alphabétique: -String Array Java et liste Python pour retrouver rapidement le nom de l'action -Ajout d'une méthode dans 'core.Action.java" pour ne pas lister les actions au dur et garder une bonne flexibilité du code.

Je n'ai fait celà que dans un sens: client->serveur pour l'instant mais il y a déjà beaucoup de modifications

matthieu637 commented 7 years ago

Quelques commentaires :

core.Action::nombreActions() c'est plus simplement Action.values().length() Pour éviter ça à l'avenir, penses à faire CTRL+SPACE dans eclipse. Par exemple tu écris "Action.", après espace il te propose "Action.values()", tu ajoutes un "." et refait CTRL+SPACE, il te propose "Action.values().length()".
dans core.Action::nomActions() tu as un compteur inutile
dans network.Client :
- Top -> TOP
- nomAction et nombreActions n'ont pas besion d'être des champs puisqu'ils sont utilisé dans une seule méthode, tu peux les déclarer localement au même endroit où tu fais leurs affectations.
- bonne idée les private static final string
- tu répètes les parseInt dans les conditions Integer.parseInt(arguments[1]), donc tu perds en complexité. Il faudrait faire le parse une seule fois (après être sur que c'est bien un nombre) et tester ensuite s'il est dans le bon intervalle.
- Action a = Action.from(nomAction[Integer.parseInt(arguments[1])]); ça me semble sous-optimal à cause de Action.from qui fait une comparaison de chaîne de caractères sur l'ensemble des actions. Imaginons plutôt que ton tableau nomAction ne représente plus la fonction entier -> String, mais une fonction du type entier -> entier qui transforme un indice par ordre alphabétique par l'indice de stockage dans la mémoire. On pourrait alors écrire
```
Action a = Actions.values()[nomAction[Integer.parseInt(arguments[1])]]; 
```
  en passant d'une complexité en O(n) à O(1). Au passage il y a à nouveau une opération de parse en trop.
quand tu utilises github essaies de publier les changements par petit morceau. Par exemple tu aurais pu d'abord poster la réduction des messages, et ensuite seulement celle des actions. Ici ce n'est pas très grave, mais c'est une bonne habitude à prendre dans le cas de plus gros projet, cela simplifie la collaboration.
côté python ça me semble ok

matthieu637 commented 7 years ago

Au passage, je vois que tu as des branches inutiles sur github, tu peux les supprimer :

git push origin --delete david540-patch-1
git push origin --delete david540-patch-2
git push origin --delete david540-patch-3

Cette fois-ci tu as bien synchroniser ton fork avant le pull request :

Merge branch 'master' into master 8e6ed5c

david540 commented 7 years ago

C'est bon j'ai modifié mon code en fonction de vos recommandations, y a peut être ma nouvelle méthode Action::nomActions() qui n'est pas optimisé mais j'ai pas trouvé d'optimisation simple, il faudrait que j'écrive une fonction similaire à sort() non ?

matthieu637 commented 7 years ago

En fait le plus simple, c'est de changer l'ordre de déclaration de l'enum pour qu'il corresponde directement à l'ordre alphabétique. En ajoutant un commentaire pour s'en rappeler et en supprimant totalement la fonction core.Actions::nomActions(). D'ailleurs, avec tes changements je pense que les fonction core.Actions::from() et core.Actions::estValide deviennent obsolètes.

Ça va alléger le code, puisque tu passes de

Action a = Action.values()[nomAction[arg]];

à

Action a = Action.values()[arg];

Et tu auras moins de calculs. Par contre c'est pas très général. Mais dans le cas présent (70 clients qui envoient plusieurs requêtes à la seconde au serveur), il vaut mieux privilégier le temps de calcul à la généralité.

Pour le reste c'est ok. Je te laisse faire cette dernière modif. J'aurais dû y penser plus tôt.

david540 commented 7 years ago

Oui effectivement je n'avais pas pensé non plus à mettre l'énum dans l'ordre alphabétique, ça ne coute presque rien si on fait attention, par rapport à ce que ça apporte. J'ai fais ces changements. Sinon j'ai ajouté les noms des joueurs en return de Reseau::top() pour l'hôte de la partie, et j'ai commencé l'idée de la liste des coups de chaque joueur. Pour l'instant cette fonction ne donne que la liste des coups que l'on a effectué et pas ceux des autres. Je n'ai pas encore commenté ce que j'ai ajouté je le ferais plus tard.

matthieu637 commented 7 years ago

Ta fonction core.Marche::tempsEcoule() est déjà présente, il s'agit de core.Marche::est_fini(). Il ya des problèmes d'encodage sur ton fichier network.Client, passes le bien en UTF-8 dans eclipse.

matthieu637 commented 7 years ago

Je te propose de garder ton code stackAllAction en local, le temps de finir le reste pour que je puisse faire l'intégration morceau par morceau et créer un futur pull request à ce sujet.

L'intérêt de la fonction listeDesCoups étant qu'elle renvoie non pas seulement la liste des coups que j'ai moi même jouer (je les connais déjà) mais celle de tous les joueurs. Il faut traiter ça dans core.Marche avec une liste plutôt qu'une seule chaîne (même idée que historique, il faut créer une nouvelle classe).

matthieu637 commented 7 years ago

Dernier point : le retour de ta fonction top(). Tu peux prendre en exemple la fonction core.Marche::fin(). Actuellement ce que tu retournes ne pourra être compris par python directement comme une liste.

david540 commented 7 years ago

Ca y est je crois avoir enlevé tout ce que j'avais ajouté pour listeDesCoups, j'ai enlevé core.Marche::tempsEcoule(), j'ai changé mon codage en UTF-8 et j'ai changé la String qui est envoyé à top() côté serveur. J'avais oublié que le return s'adresserait a une IA. Il faut que je pense à directement faire un pull request sur ce projet et pas sur le mien, sinon ça modifie les pull requests que je fais avant.

matthieu637 commented 7 years ago

Merci je prends déjà ça. Tu peux déporter la suite dans un futur pull request. Par contre, penses à bien synchroniser ton projet et ton fork, apparemment tu n'avais pas récupérer toutes les modifications faites par @MatthieuDEVALLE. J'ai également fais quelques modifications.

#ajoute le projet officiel à votre fork
git remote add upstream https://github.com/matthieu637/cpp-2a-info.git
#récupère les modifications du projet officiel (qui peuvent également provenir d'autres étudiants)
git pull upstream master
#envoyer la fusion à votre fork github
git push origin master

matthieu637 / cpp-2a-info

Remplacement mots par lettre ou chiffre dans les messages réseaux #6