Skip to content

Instantly share code, notes, and snippets.

@Arnauld

Arnauld/al1.md

Created June 15, 2018 15:44
Show Gist options
  • Save Arnauld/207dcfd0169caaddb954a35808f0ce6c to your computer and use it in GitHub Desktop.
Save Arnauld/207dcfd0169caaddb954a35808f0ce6c to your computer and use it in GitHub Desktop.
Download ZIP
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment