Arnauld/al1.md

## al1.md

      
    Raw
  

              al1.md
            
          
    GR1


CLI: mauvaise gestion --help; privilégiez un contains plutôt qu'un args.length==1
Déclaration des types List<Operator> ...  plutôt que ArrayList<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 classe Calculator

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 deux break 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 un BinaryOperator
Aurait plutôt tendance à injecter dans Calculator, le Parser à utiliser plutôt que ce soit lui qui l'instancie
Super les tests par Classes!!!

GR2


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

GR3


RpnCalculator.evaluate static dommage: il n'est pas possible du coup de spécifier le Tokenizer

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

GR4


package opertators: c'est le package des operateurs ou t'as tort?
BaseOperator aurait pu s'appeller BinaryOperator
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

GR5


Euh c'est RPN1 non ?
pas de Tokenizer, Operator, ...

GR6

(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 binaires
Calculator.Ops()

la méthode renvoie la Map créé ET l'affecte à un champ; du coup il y a un appel louche Ops() 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


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 un Tokenizer
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 binaires
Operators cool
Calculator.setAllOperators()

la méthode renvoie la Map créé ET l'affecte à un champ; du coup il y a un appel louche setAllOperators() en début de méthode...; private final Map<String, IOperator> map = setAllOperators(); aurait été plus clair en renommant en defaultOperators()
dommage de ne pas avoir exploité le code de la classe Operators qui fait déjà ça !!


splitExpression(exp,"[ ]+") aurait pu être dans un Tokenizer
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 le Tokenizer 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 le Tokenizer 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 boucle for systématique
Pas d'exploitation du découpage au travers des Tests: vous pouviez tester chaque classe isolément du reste

GR12


String[] split_exp = expression.split("\\s+"); aurait pu être dans une classe Tokenizer
Operator.getOperator(String) il n'est pas nécessaire d'appeller contains vu que au final tu fais la même boucle for; 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 classe Tokenizer
Stack s = new Stack(); et s.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 la Calculatrice 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 des pop dans l'Operator mais les push 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 genre SWAP 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: cool
Operators 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?
Tokenizerdevrait être passé en paramètre du constructeur de RpnCalculator 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
No results found