Skip to content
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

Remove Phone and Email instances, refactor TypicalQAndA #71

Merged
merged 8 commits into from
Oct 25, 2020

Conversation

joshruien
Copy link

No description provided.

joshruien and others added 4 commits October 25, 2020 19:29
Refactor typical question set.
* refactor-phone-email:
  Delete phone and email instances. Refactor typical question set.

# Conflicts:
#	src/main/java/seedu/medmoriser/logic/commands/AddCommand.java
#	src/main/java/seedu/medmoriser/logic/commands/EditCommand.java
#	src/main/java/seedu/medmoriser/logic/parser/EditCommandParser.java
#	src/main/java/seedu/medmoriser/storage/JsonAdaptedQAndA.java
#	src/test/data/JsonSerializableMedmoriserTest/duplicateQAndAMedmoriser.json
#	src/test/data/JsonSerializableMedmoriserTest/invalidQAndAMedmoriser.json
#	src/test/data/JsonSerializableMedmoriserTest/typicalQAndAsMedmoriser.json
#	src/test/java/seedu/medmoriser/logic/LogicManagerTest.java
#	src/test/java/seedu/medmoriser/logic/commands/CommandTestUtil.java
#	src/test/java/seedu/medmoriser/logic/commands/EditCommandTest.java
#	src/test/java/seedu/medmoriser/logic/commands/EditQAndADescriptorTest.java
#	src/test/java/seedu/medmoriser/logic/commands/FindCommandTest.java
#	src/test/java/seedu/medmoriser/logic/parser/AddCommandParserTest.java
#	src/test/java/seedu/medmoriser/logic/parser/EditCommandParserTest.java
#	src/test/java/seedu/medmoriser/logic/parser/ParserUtilTest.java
#	src/test/java/seedu/medmoriser/model/MedmoriserTest.java
#	src/test/java/seedu/medmoriser/model/ModelManagerTest.java
#	src/test/java/seedu/medmoriser/model/qanda/AnswerContainsKeywordPredicateTest.java
#	src/test/java/seedu/medmoriser/model/qanda/QAndATest.java
#	src/test/java/seedu/medmoriser/model/qanda/QuestionContainsKeywordsPredicateTest.java
#	src/test/java/seedu/medmoriser/model/qanda/UniqueQAndAListTest.java
#	src/test/java/seedu/medmoriser/storage/JsonAdaptedQAndATest.java
#	src/test/java/seedu/medmoriser/storage/JsonMedmoriserStorageTest.java
#	src/test/java/seedu/medmoriser/testutil/EditQuestionSetDescriptorBuilder.java
#	src/test/java/seedu/medmoriser/testutil/QAndABuilder.java
#	src/test/java/seedu/medmoriser/testutil/TypicalQAndA.java
@joshruien joshruien added Testing issues to do with test cases / testing in general Refactor Clean up / Refactor code labels Oct 25, 2020
@joshruien joshruien added this to the v1.3 milestone Oct 25, 2020
@joshruien joshruien self-assigned this Oct 25, 2020
@joshruien joshruien linked an issue Oct 25, 2020 that may be closed by this pull request
QAndABuilder qAndAInList = new QAndABuilder(lastQAndA);
QAndA editedQAndA = qAndAInList.withQuestion(VALID_QUESTION_BOB).withPhone(VALID_PHONE_BOB)
.withTags(VALID_TAG_HUSBAND).build();
QAndABuilder questionSetInList = new QAndABuilder(lastQAndA);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you edited the qAndAInList back to questionSetInList

QAndA editedQAndA = qAndAInList.withQuestion(VALID_QUESTION_BOB).withPhone(VALID_PHONE_BOB)
.withTags(VALID_TAG_HUSBAND).build();
QAndABuilder questionSetInList = new QAndABuilder(lastQAndA);
QAndA editedQAndA = questionSetInList.withQuestion(VALID_QUESTION_B).withTags(VALID_TAG_TAG2).build();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

questionSetInList

public void resetData_withDuplicateQAndAs_throwsDuplicateQAndAException() {
// Two qAndAs with the same identity fields
QAndA editedAlice = new QAndABuilder(ALICE).withAnswer(VALID_ANSWER_BOB).withTags(VALID_TAG_HUSBAND)
public void resetData_withDuplicateQuestionSets_throwsDuplicateQuestionSetException() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change questionSet back to qAndA

// Two qAndAs with the same identity fields
QAndA editedAlice = new QAndABuilder(ALICE).withAnswer(VALID_ANSWER_BOB).withTags(VALID_TAG_HUSBAND)
public void resetData_withDuplicateQuestionSets_throwsDuplicateQuestionSetException() {
// Two questionSets with the same identity fields

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change questionSet back to qAndA

@@ -60,22 +59,22 @@ public void hasQAndA_nullQAndA_throwsNullPointerException() {
}

@Test
public void hasQAndA_qAndANotInMedmoriser_returnsFalse() {
assertFalse(medmoriser.hasQAndA(ALICE));
public void hasQAndA_questionSetNotInMedmoriser_returnsFalse() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change questionSet back to qAndA

public void hasQAndA_qAndAInMedmoriser_returnsTrue() {
medmoriser.addQAndA(ALICE);
assertTrue(medmoriser.hasQAndA(ALICE));
public void hasQuestionSet_questionSetInMedmoriser_returnsTrue() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change questionSet back to qAndA

public void hasQAndA_qAndAWithSameIdentityFieldsInMedmoriser_returnsTrue() {
medmoriser.addQAndA(ALICE);
QAndA editedAlice = new QAndABuilder(ALICE).withAnswer(VALID_ANSWER_BOB).withTags(VALID_TAG_HUSBAND)
public void hasQuestionSet_questionSetWithSameIdentityFieldsInMedmoriser_returnsTrue() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change questionSet back to qAndA

@@ -78,14 +78,14 @@ public void hasQAndA_nullQAndA_throwsNullPointerException() {
}

@Test
public void hasQAndA_qAndANotInMedmoriser_returnsFalse() {
assertFalse(modelManager.hasQAndA(ALICE));
public void hasQAndA_questionSetNotInMedmoriser_returnsFalse() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change questionSet back to qAndA

public void hasQAndA_qAndAInMedmoriser_returnsTrue() {
modelManager.addQAndA(ALICE);
assertTrue(modelManager.hasQAndA(ALICE));
public void hasQuestionSet_questionSetInMedmoriser_returnsTrue() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change questionSet back to qAndA

uniqueQAndAList.add(ALICE);
uniqueQAndAList.add(BOB);
assertThrows(DuplicateQAndAException.class, () -> uniqueQAndAList.setQAndA(ALICE, BOB));
public void setQAndA_editedQuestionSetHasNonUniqueIdentity_throwsDuplicateQuestionSetException() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change questionSet back to qAndA

public void remove_existingQAndA_removesQAndA() {
uniqueQAndAList.add(ALICE);
uniqueQAndAList.remove(ALICE);
public void remove_existingQuestionSet_removesQAndA() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change questionSet back to qAndA

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch!

assertEquals(expectedUniqueQAndAList, uniqueQAndAList);
}

@Test
public void setQAndAs_listWithDuplicateQuestions_throwsDuplicateQAndAException() {
List<QAndA> listWithDuplicateQAndAs = Arrays.asList(ALICE, ALICE);
public void setQAndAs_listWithDuplicateQuestions_throwsDuplicateQuestionSetException() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change questionSet back to qAndA

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch!

Copy link

@tengjianling tengjianling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just need to change some instances of questionSets back to qAndA, otherwise its good to go!

public void resetData_withDuplicateQAndAs_throwsDuplicateQAndAException() {
// Two qAndAs with the same identity fields
QAndA editedAlice = new QAndABuilder(ALICE).withAnswer(VALID_ANSWER_BOB).withTags(VALID_TAG_HUSBAND)
public void resetData_withDuplicateQuestionSets_throwsDuplicateQAndAException() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

questionSet

public void hasQAndA_qAndAInMedmoriser_returnsTrue() {
modelManager.addQAndA(ALICE);
assertTrue(modelManager.hasQAndA(ALICE));
public void hasQuestionSet_qAndAInMedmoriser_returnsTrue() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

questionSet

public void setQAndA_editedQAndAHasDifferentIdentity_success() {
uniqueQAndAList.add(ALICE);
uniqueQAndAList.setQAndA(ALICE, BOB);
public void setQAndA_editedQuestionSetHasDifferentIdentity_success() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

questionSet

uniqueQAndAList.add(ALICE);
uniqueQAndAList.add(BOB);
assertThrows(DuplicateQAndAException.class, () -> uniqueQAndAList.setQAndA(ALICE, BOB));
public void setQAndA_editedQAndAHasNonUniqueIdentity_throwsDuplicateQuestionSetException() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

questionSet

Copy link

@tengjianling tengjianling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change questionSet

public void remove_qAndADoesNotExist_throwsQAndANotFoundException() {
assertThrows(QAndANotFoundException.class, () -> uniqueQAndAList.remove(ALICE));

public void remove_qAndADoesNotExist_throwsQuestionSetNotFoundException() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

questionSet

Copy link

@tengjianling tengjianling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

questionSet

"tagged" : [ "friends" ]
"question" : "Question One",
"answer" : "Answer 1",
"tagged" : [ "DNA" ]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can have 1 or two showing the phrase version instead of 1 word, but no biggie

@codecov-io
Copy link

codecov-io commented Oct 25, 2020

Codecov Report

Merging #71 into master will decrease coverage by 0.73%.
The diff coverage is 87.50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #71      +/-   ##
============================================
- Coverage     68.15%   67.42%   -0.74%     
+ Complexity      445      419      -26     
============================================
  Files            79       77       -2     
  Lines          1410     1326      -84     
  Branches        150      135      -15     
============================================
- Hits            961      894      -67     
+ Misses          404      390      -14     
+ Partials         45       42       -3     
Impacted Files Coverage Δ Complexity Δ
...va/seedu/medmoriser/logic/commands/AddCommand.java 85.71% <ø> (ø) 8.00 <0.00> (ø)
.../java/seedu/medmoriser/logic/parser/CliSyntax.java 75.00% <ø> (-8.34%) 1.00 <0.00> (ø)
...java/seedu/medmoriser/logic/parser/ParserUtil.java 96.15% <ø> (-1.07%) 10.00 <0.00> (-4.00)
...va/seedu/medmoriser/model/util/SampleDataUtil.java 20.00% <ø> (ø) 1.00 <0.00> (ø)
.../main/java/seedu/medmoriser/model/qanda/QAndA.java 96.77% <50.00%> (+15.14%) 16.00 <0.00> (-4.00) ⬆️
...a/seedu/medmoriser/logic/commands/EditCommand.java 92.98% <100.00%> (-1.23%) 12.00 <0.00> (ø)
...eedu/medmoriser/logic/parser/AddCommandParser.java 100.00% <100.00%> (+13.33%) 5.00 <0.00> (ø)
...edu/medmoriser/logic/parser/EditCommandParser.java 91.30% <100.00%> (-1.29%) 9.00 <0.00> (-2.00)
...ava/seedu/medmoriser/storage/JsonAdaptedQAndA.java 100.00% <100.00%> (ø) 7.00 <0.00> (-4.00)
...eedu/medmoriser/storage/JsonMedmoriserStorage.java 100.00% <0.00%> (+14.28%) 8.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c11e91...5c90d9f. Read the comment docs.


// different qAndA -> returns false
assertFalse(ALICE.equals(BOB));
// different questionSet -> returns false

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better to refer to this as QAndA?

public static final QAndA BOB = new QAndABuilder().withQuestion(VALID_QUESTION_BOB)
.withPhone(VALID_PHONE_BOB).withEmail(VALID_EMAIL_BOB).withAnswer(VALID_ANSWER_BOB)
.withTags(VALID_TAG_HUSBAND, VALID_TAG_FRIEND).build();
// Manually added - QuestionSet's details found in {@code CommandTestUtil}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This too, just to make it less confusing for readers

Copy link

@yongmingyang yongmingyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than some name changes, I think it's good to go

Copy link

@tengjianling tengjianling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great job brother

@joshruien joshruien merged commit 93a1549 into AY2021S1-CS2103T-W15-1:master Oct 25, 2020
@yongmingyang yongmingyang added this to To review (for merging) in Team Project Oct 26, 2020
@yongmingyang yongmingyang moved this from To review (for merging) to Done in Team Project Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor Clean up / Refactor code Testing issues to do with test cases / testing in general
Projects
Development

Successfully merging this pull request may close these issues.

Refactor out phone and email fields
4 participants