- CLI: mauvaise gestion
--help
; privilégiez uncontains
plutôt qu'unargs.length==1
- Déclaration des types
List<Operator> ...
plutôt queArrayList<Operator> ...
- Dommage de pas avoir fait une classe
Operators
, qui aurait pu contenir la recherche du bon operateur plutôt que la boucle for dans la classeCalculator
for(Operator operator : operations){
if(token.equals(operator.symbol)){
...
}
}
- Gestion bizarre d'un boolean pour faire le break, autant le faire directement dans le
if
; en fait vu des deuxbreak
successif une méthode dédiée avec un return aurait été plus lisible - Gestion acrobatique des index
rightOperand = operation.get(--indexOperator);
operation.remove(indexOperator);
leftOperand = operation.get(--indexOperator);
operation.remove(indexOperator);
- Malin le
Operator2Operands
; on appelle cela plutôt unBinaryOperator
- Aurait plutôt tendance à injecter dans Calculator, le Parser à utiliser plutôt que ce soit lui qui l'instancie
- Super les tests par Classes!!!
- OK, pour Operators, mais une interface et plusieurs implémentations auraient été plus adaptée et plus facilement extensible
Operators.calculate(Double,Double)
devraient prendre la stack en paramètre- Pas de
Tokenizer
... - Dommage vous n'exploitez pas le découpage en classe pour faire des tests correspondant
RpnCalculator.evaluate
static dommage: il n'est pas possible du coup de spécifier leTokenizer
new RpnCalculator(new Tokenizer()).evaluate(expression);
Tokenizer
manquant:String [] parts = exp.split("\\s+");
on est donc bloqué (non évoluable sans changer explicitement le code) avec cette implémentation- Pas de classe
Operator
...; obligé de changer le code pour ajouter un nouvel operateur - Operateur binaire uniquement
- Pas d'exploitation du découpage au travers des Tests
- package
opertators
: c'est le package desoperateurs
out'as tort?
BaseOperator
aurait pu s'appellerBinaryOperator
- Bien vu le
DuplicationOperator
- Arf
OperatorUtil
dommage... on est obligé de changer le code pour ajouter un nouvel opérateur:
OperatorRegistry registry = new OperatorRegistry();
registry.register("+", new PlusOperator());
registry.register("DUP", new DuplicationOperator());
...
IOperator operator = registry.GetOperator(operatorKey);
- Manque le
Tokenizer
- Pas d'exploitation du découpage au travers des Tests: vous pouviez tester chaque operateur isolément du reste
- Euh c'est RPN1 non ?
- pas de
Tokenizer
,Operator
, ...
(ressemble beaucoup à GR7)
IOperator
dans le bon package! dommage qu'il ne prenne pas la stack; du coup il ne gère que les opérateurs binairesCalculator.Ops()
- la méthode renvoie la
Map
créé ET l'affecte à un champ; du coup il y a un appel loucheOps()
en début de méthode...;private final Map<String, IOperator> map = Ops();
aurait été plus clair - dommage de ne pas avoir déporté ce code dans une classe dédiée e.g.
Ops
- la méthode renvoie la
public class Ops {
private final Map<String, IOperator> map = defaultOps();
public IOperator get(String operatorKey) {
return map.get(operatorKey);
}
private static Map<String, IOperator> Ops(){
Map<String, IOperator> map = new HashMap<>();
map.put("+", new More());
map.put("-", new Minus());
map.put("*", new Multiplication());
map.put("/", new Division());
map.put("%", new Modulo());
map.put("^", new Power());
return map;
}
}
exp.split("[ ]+")
aurait pu être dans unTokenizer
- Pas d'exploitation du découpage au travers des Tests: vous pouviez tester chaque operateur isolément du reste
#GR7
(ressemble beaucoup à GR6)
IOperator
dans le bon package! dommage qu'il ne prenne pas la stack; du coup il ne gère que les opérateurs binairesOperators
coolCalculator.setAllOperators()
- la méthode renvoie la
Map
créé ET l'affecte à un champ; du coup il y a un appel louchesetAllOperators()
en début de méthode...;private final Map<String, IOperator> map = setAllOperators();
aurait été plus clair en renommant endefaultOperators()
- dommage de ne pas avoir exploité le code de la classe
Operators
qui fait déjà ça !!
- la méthode renvoie la
splitExpression(exp,"[ ]+")
aurait pu être dans unTokenizer
- Pas d'exploitation du découpage au travers des Tests: vous pouviez tester chaque operateur isolément du reste
#GR8
- Très bien!
Calculator
devrait prendre leTokenizer
dans son constructeur, ce qui permet de passer une autre implémentation de manière transparente- Dommage que
Operator
ne prenne pas la Stack en paramètre, on est limité du coup aux operations binaires - Oulalala code compliqué en approche:
EnumUtils.isValidEnum(Operator.class,Operator.find(s))
; dommage vous aviez déjà tout fait pour pas mettre cette horreur :D
public static Operator find(String operator) {
switch (operator) {
case "+":
return Operator.PLUS;
case "-":
return Operator.MINUS;
case "*":
return Operator.TIMES;
case "/":
return Operator.DIVIDE;
}
return null; // or throw an OperatorNotFoundException
}
- Pas d'exploitation du découpage au travers des Tests: vous pouviez tester chaque classe isolément du reste
#GR9
- RPN1?
- pas de
Tokenizer
,Operator
, ...
#GR10
- pas de code
#GR11
- Très bien! même si le code de gestion (
Operation.calculateFromRefinedExpression
) est un peu compliqué CLI.evaluate
aurait pu être dans une classe dédiée- Evitez les méthodes statiques, on ne peut pas injecter le Tokenizer que l'on veut ici:
class CLI {
static double evaluate(String input){
new RPNExpression(new Token()).evaluate(input);
}
}
class RPNExpression {
private final Token token;
...
public double evaluate(String expression) {
List<String> content = token.refineExpression( input );
...
}
}
Calculator
devrait prendre leTokenizer
dans son constructeur, ce qui permet de passer une autre implémentation de manière transparente- Dommage que
Operator
ne prenne pas la Stack en paramètre, on est limité du coup aux operations binaires static final Map symbolsMap = new HashMap();
malin la de faire une map de symbol; ici c'est pas forcément nécessaire car il n'y a QUE 4 éléments dedans, on pouvait se contenter de faire une bouclefor
systématique- Pas d'exploitation du découpage au travers des Tests: vous pouviez tester chaque classe isolément du reste
String[] split_exp = expression.split("\\s+");
aurait pu être dans une classeTokenizer
Operator.getOperator(String)
il n'est pas nécessaire d'appellercontains
vu que au final tu fais la même bouclefor
; en l'occurence là tu parcours deux fois les éléments (une fois pour contains un fois pour le trouver)- pourquoi
Operator implements DoubleBinaryOperator
? - Pas d'exploitation du découpage au travers des Tests: vous pouviez tester chaque classe isolément du reste
#GR13
String tabChar[]=npiString.split(" ");
aurait pu être dans une classeTokenizer
Stack s = new Stack();
ets.push(Double.parseDouble(c));
du coup ta stack contient des double ? pourquoi ne pas la typer:Stack<Double> s = new Stack<>();
- Vu que la stack contient des doubles pourquoi passer son temps à les transformer en chaines de caractères puis en double à nouveau ?
double a=Double.parseDouble(s.pop().toString());
- Pas d'utilisation d'Operateur...
- en fait c'est RPN1.
#GR14
- Calculatrice: ne pas mettre de traitement bloquant dans un constructeur; faire une méthode dédiée que l'on appellera si besoin; tu gardes ainsi un unique constructeur
public class Calculatrice {
...
public void waitForInput () {
Scanner scan = new Scanner(System.in);
System.out.println("Entrez les operations: ");
expression = scan.next();
}
}
- MAIS ce n'est pas à la calculatrice de faire la gestion de l'entrée: elle calcule c'est tout; la méthode précédente va dans
CLI
et appelle laCalculatrice
avec l'entrée en expression - Ce n'est pas le role du
Tokenizer
d'"éliminer" les tokens invalides: cela rajoute une dépendance forte entre le Tokenizer et l'interpretation des Tokens dans un second temps - Dommage de faire une méthode statique, cela ne permet de choisir une autre implémentation; surtout qu'il y a une utilisation maladroite: tu créés une instance mais tu appelles la méthode statique (donc non rattachée à l'instance)
Tokenizer tokenizer = new Tokenizer();
List<String> tokens = Tokenizer.tokenize(expression,"\\s+");
- dommage
public static double operate(Stack <String> pile,String s){
; tu fais despop
dans l'Operator
mais lespush
sont faits en dehors, tu aurais pu laissé à l'operateur la responsabilité de consommer la pile et de l'enrichir de son résultat. Cela permet aussi d'avoir des opérateurs genreSWAP
qui intervertissent deux valeurs dans la pile par exemple... Tu gères un nombre variables d'opérandes mais pas de résultats du coup `
#GR15
Parser
pas static: coolOperators
cool tu gères un nombre quelconque de paramètres; dommage de pas avoir mis la même de recherche dans cette classe:
for (Operators op : Operators.values()) {
if (op.getSign().equals(list.get(i))) {
operator = op;
...
}
}
deviendrait:
operator = Operators.findOperator(list.get(i));
float[] args = operator.buildArgs(list, i);
- compliqué la gestion de ce tableau d'args en plus, faut pas se tromper dans les indices..(
arrayFloatPush
ouch! tu créés un tableau à chaque nouvel argument...) (pseudo code non testé):
class Operator {
...
public float[] buildArgs(List<String> params, int startIndex) {
float[] args = new float[getNbArgs()];
int nb = getNbArgs();
for(int i = nb; i > 0; i--) {
args[nb-i] = Float.parseFloat(params.get(startIndex-i));
}
return args;
}
}
- Ceci étant ça reste compliqué!!! Faut pas se perdre dans les indices:
i = -1;
}
i++;
- Pas d'exploitation du découpage au travers des Tests: vous pouviez tester chaque classe isolément du reste
#GR16
Calculator
problème de l'interface unique qui prend toutes les opérations: non extensible, il n'est pas possible de rajouter des operateurs à la volée, d'en permettre certains, et d'en enlever d'autres- C'est louche que l'interface
Operate
renvoie une instance de lui même:
public class Division extends Operator implements Operate {
...
@Override
public Operate getInstance(String value){
return new Division(value);
}
}
- Vu que j'ai déjà une instance de
Division
pourquoi en renvoyer une nouvelle par cette méthode? Tokenizer
devrait être passé en paramètre du constructeur deRpnCalculator
ce qui permet de pouvoir brancher une autre implémentation au besoin:
RpnCalculator rpnCalculator = new RpnCalculator(new Tokenizer());
- Pas d'exploitation du découpage au travers des Tests: vous pouviez tester chaque classe isolément du reste
#GR17
- pas de code