-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Addition of the QuestionPriceArea class #45
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
La classe QuestionPriceAreaTests n'est pas dans le bon package profile. Je vois qu'il y a ici 2 packages profile dans le dossier test : .../apartments/profile et .../apartments/valuefunction/profile.
Il faut mettre la classe de test dans celui contenu dans le package de la valuefunction et supprimer le premier package.
...java/io/github/oliviercailloux/y2018/apartments/valuefunction/profile/QuestionPriceArea.java
Outdated
Show resolved
Hide resolved
...java/io/github/oliviercailloux/y2018/apartments/valuefunction/profile/QuestionPriceArea.java
Outdated
Show resolved
Hide resolved
...java/io/github/oliviercailloux/y2018/apartments/valuefunction/profile/QuestionPriceArea.java
Show resolved
Hide resolved
...java/io/github/oliviercailloux/y2018/apartments/valuefunction/profile/QuestionPriceArea.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ajouter le test unitaire suivant. Créer une AVF linéaire (par exemple, celle par défaut pour un étudiant). Poser la question : êtes-vous prêt à payer x € pour y m² en plus ? Supposer que la réponse est positive. Adapter l’AVF. Considérer un appartement a et un appartement b x € plus cher et avec y m² en plus. Vérifier que b est préféré à a avec la nouvelle avf.
Il faut que ceci fonctionne pour toute valeur (ce n’est pas possible de tester pour toute valeur, mais le test peut vérifier pour certaines).
...java/io/github/oliviercailloux/y2018/apartments/valuefunction/profile/QuestionPriceArea.java
Outdated
Show resolved
Hide resolved
...java/io/github/oliviercailloux/y2018/apartments/valuefunction/profile/QuestionPriceArea.java
Outdated
Show resolved
Hide resolved
...java/io/github/oliviercailloux/y2018/apartments/valuefunction/profile/QuestionPriceArea.java
Outdated
Show resolved
Hide resolved
...java/io/github/oliviercailloux/y2018/apartments/valuefunction/profile/QuestionPriceArea.java
Outdated
Show resolved
Hide resolved
...java/io/github/oliviercailloux/y2018/apartments/valuefunction/profile/QuestionPriceArea.java
Outdated
Show resolved
Hide resolved
...java/io/github/oliviercailloux/y2018/apartments/valuefunction/profile/QuestionPriceArea.java
Outdated
Show resolved
Hide resolved
Compte tenu de la dépendance de cette tâche avec le travail de Gabriel et de Morgane, les équipes m'ont demandé s'il serait possible de résoudre le problème de resolve grâce à une issue (qui serait traitée avant la fin de cette itération) et de merge la PR actuelle. Cela permettrait à Morgane et Gabriel de vous soumettre leur travail. Qu'en pensez-vous ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quelle est cette PR dépendante que vous mentionnez ici ? Je ne suis pas enthousiaste à l’idée de fusionner cette PR ci, car la résolution est la partie cruciale de cette PR, et étant incorrecte actuellement, elle risque de mal orienter le projet. Ceci s’applique d’autant plus que l’autre PR utiliserait cette résolution. Si l’autre PR n’utilise pas la résolution, je vous propose de retirer l’aspect résolution pour le moment (lancer UnsupportedOperationException) et fusionner.
...java/io/github/oliviercailloux/y2018/apartments/valuefunction/profile/QuestionPriceArea.java
Outdated
Show resolved
Hide resolved
...java/io/github/oliviercailloux/y2018/apartments/valuefunction/profile/QuestionPriceArea.java
Outdated
Show resolved
Hide resolved
Bonjour Monsieur, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ne paniquons pas : la seule chose à corriger dans cette PR est la méthode resolve. Je pense que mon e-mail précédent, que vous indiquez avoir compris, donne le calcul à effectuer pour trouver la nouvelle borne de l’intervalle, et c’est tout ce qui me semble nécessaire pour cette méthode.
J’insiste donc pour une implémentation corrigée, et je vous propose de réduire les ambitions. Sinon, on oriente véritablement le projet dans une direction qui n’a pas grand sens.
Concentrez-vous sur l’implémentation d’une question : surface par rapport au prix, par exemple.
Votre logique de profils me semble bonne, et je ne crois pas qu’il faille y changer grand-chose. En particulier, ça ne change pas le calcul de la fonction de valeur. Le profil tel que vous le concevez actuellement me semble correct pour ce qui est de ses champs, pour l’essentiel (sauf que j’aurais commencé par un seul range pour seulement le critère surface, pour simplifier). Il faut seulement interpréter le range différemment. Je ne vois pas de contradiction par rapport à mon discours passé.
Nous pouvons en discuter via Teams vers 16h, si cela vous convient. |
Merci, cela nous convient. Nous serons disponible pour 16h (@oliviercailloux) |
La classe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FUTURE : test impliquant fonctions de valeurs avant / après résolution d’une question (comme indiqué dans une autre review).
Lors de l'ajout des tests, nous avons découvert un problème : l'adaptation se fait mal lorsque l'on place certaines valeurs dans la question. L'issue #63 a été ouverte à ce sujet. |
Profile
dependency.We have created a fake
Profile
class so that the code can compileOnce PR #44 (Profile) is merged to master, we will merge master with our PR and resolve the conflict by taking the
Profile
class from masterDespite a slight delay on the
Profile
side, we still insist on this PR and hope that the PR on addingProfile
will be merge because we will needQuestionPriceArea
for the last Java iteration.We assume the rules and we know that this PR will be sanctioned if
Profile
is not merge in time.