[W4.4b][T09-A4]JOHNNY CHAN JUN XUN #1706
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix comment in Address.java (initially it was email for address) update tests to validate: new name regex
… the name update expected.txt to reflect the new changes
hence any name with numeric characer would be rejected.
* [se-edu#108] add unit tests for add command * renamed methods for consistency * capitalize todo
* Fix code style violation reported by checkstyle * Fix checkstyle issues in AddCommandTest
* Add unit test for Utils class * Reduce the number of test cases in isAnyNull method * Reduce number of test cases in elementsAreUnique method * Change assertAreUnique and assertNotUnique methods to private and remove extra spaces * Remove @before * Remove try catch clause * Inline buildTag method * Change test tag to local variable * Remove unused import * Extract createList() method and remove arraylist * Remove constant * Rearrange test cases to avoid confusion * Rename variable * add test cases to prevent false negative * Inline createList method * Rearrange test cases and remove whitespaces * Add newline at eof * Added spaces for type casting * Update comment for UtilsTest
* Add unit test for StorageFile class * Change indentation and add comments * Make invalid data file more obvious * Create Utility method getStorage * Change contains method to equals method * Style change in StorageFileTest * Style changes in StorageFileTest * Inline getStorage method * Remove unused import * Revert "Inline getStorage method" This reverts commit f431d12. * Re-adjust getStorage() method * Change assertTrue() to assertEquals * Move test files to correct folder * Add test persons and extract to a method * Rework assertSaveSuccess() method * Move compareFiles method to TestUtil class * Minor changes in StorageFile class * remove trailing whitespaces and unused imports * remove unused import * Change to return directly as addressbook * Rework and rename compareFile() to assertTextFilesEqual() * Use of temporary folder and minor style issues * Remove trailing whitespaces * minor style changes * Extract temp.txt to constant string * rework assertSaveSuccess() method * change of javadoc * Minor changes * Javadoc language * Javadoc change * Javadoc change
* Add testutil classes to facilitate testing * Add TestMain class to facilitate testing of commands * Add equals() methond in AddressBook, UniquePersonList and UniqueTagList * Add FindCommandTest and CommandTest * Fix code style * Add indentation for equals() method * Reimplement FindCommandTest without use of TestMain * Remove duplicate equals() method in UniquePersonList * Remove TestPerson class * Refactor FindCommandTest * Refactor FindCommandTest * Refactor FindCommandTest * Remove empty line at top of assertCommandBehavior * Refactor TypicalPersons.java and shift instantiating a FindCommand in FindCommandTest to asserCommandBehavior method * Rename assertCommandBehavior to assertFindCommandBehavior * Remove PersonBuilder and make TypicalPersons fields non-static * Refactor FindCommandTest by inlining test methods * Remove unused imports * Add new test cases * Remove trailing empty lines * Fix coding style * Remove unused imports * Remove trailing whitespaces
…edu#132) * change file ext requirement from txt to xml * test script change
…f Person class (se-edu#137) Sometimes we want to check if 2 persons contain same exact values for all fields, and at other times, just to see if it is the same person (just field values).
se-edu#131) Tests for the `view` and `viewall` commands have been added.
While checkstyle is not covered as a learning objective in level 2, its absence makes it harder for maintainers of the repository to ensure that the coding standards are followed, as some violations (such as trailing whitespaces and redundant imports) can be difficult to spot unless one pays a lot of attention to them. checkstyle.xml is taken from level 4.
…ile (se-edu#142) load() method creates a new address book xml file if it does not exist. However, the responsibility of load() is to simply read the file, not write it. Even if the file does not exist, load() should simply treat the situation as if it is loading a blank file, and handle accordingly. Creating a new file is redundant anyway if load() does not even attempt to read it after creation, and load() will not gain any new information by reading it. Let's remove the saving functionality of load() in order to narrow down the responsibility of the method to purely just reading the xml files.
AddressBook#addTag() adds a Tag to the master list of Tags in AddressBook. This method is not used anywhere, and can be classified as unused code. There are no commands that support the addition of Tags only, and it is also not part of any learning objectives. Furthermore, AddressBook#syncTagsWithMasterList() already handles the addition of Tags from a Person gracefully without using exceptions. It should be removed as unused code contributes to maintenance and comprehension overheads. Let's remove AddressBook#addTag().
UniqueTagList#add() adds a Tag to the list of unique Tags, and throws an exception if it is a duplicate of an existing Tag in the list. With the removal of AddressBook#addTag(), this method is not used anywhere, and can be classified as unused code. It should be removed as unused code contributes to maintenance and comprehension overheads. Let's remove UniqueTagList#add().
AddressBook#removeTag() removes a Tag from the master list of Tags and from any Person in AddressBook. This method is not used anywhere, and can be classified as unused code. There are no commands that support the removal of Tags only, and it is also not part of any learning objectives. It should be removed as unused code contributes to maintenance and comprehension overheads. Let's remove AddressBook#removeTag().
UniqueTagList#remove() removes a Tag from the list of unique Tags, and throws an exception if it is not found in the list. With the removal of AddressBook#removeTag(), this method is not used anywhere, and can be classified as unused code. It should be removed as unused code contributes to maintenance and comprehension overheads. Let's remove UniqueTagList#remove().
AddressBook#containsTag() checks if the master list of Tags contains a Tag with the same value as the given Tag. AddressBook#containsTag() is not used in the production code. While it is used in test code, its use can be replaced by AddressBookTest#isTagObjectInAddressBookList(). Let's remove AddressBook#containsTag(). Notes: * Why not keep AddressBook#containsTag() and remove AddressBookTest#isTagObjectInAddressBookList() instead? The former does a value equality test while the latter goes further and does a reference equality test. The test code requires a reference equality test to verify Person objects refer to Tag objects in the common tag list instead of keeping its own copies of Tag objects. * Why not change AddressBook#containsTag() to use reference equality and use that in tests? Doing so will make AddressBook#containsTag()'s semantics inconsistent with the rest of our API.
Let's remove the following methods that are no longer used, including other related code that are made redundant by their removal. * Addressbook#addTag() * Addressbook#containsTag() * Addressbook#removeTag() * UniqueTagList#add() * UniqueTagList#remove()
Only JUnit tests are run by Travis. Let's teach Travis to run I/O tests. Since the default Travis build command, which runs the JUnit tests, is being overwritten, we have to explicitly run the JUnit tests as well.
… run (se-edu#192) There are three issues with the logic of 'runtest.sh' for deleting './actual.txt' from previous run: 1. It treats 'actual.txt' as a directory (with "-d"). 2. It tries to delete 'actual.txt' when it does not exist (with "!"). 3. It looks at the wrong directory for 'actual.txt' (with ".." instead of "."). Let's update runtest.sh to fix the above issues.
As we are moving to IntelliJ, the Eclipse project files are no longer needed. Let's remove them.
…du#206) If the project is not set up properly, StorageFileTest will fail because it cannot find the test data file that it uses. The instructions in DeveloperGuide are not clear enough, as the resolution of this problem is only in the Troubleshooting section, and is not mentioned anywhere else. Let's make the DeveloperGuide instructions clearer by addressing this issue directly in the setup section.
Command class is abstract. Abstract classes are not an LO covered at level2. Let's make Command class non-abstract so that students who haven't learned about abstract classes are not confused by it.
…ration (se-edu#226) The working directory needs to be changed for the tests to run properly. Therefore, the developer guide instructs the developer to modify the generated test configuration. If the IDE generates another new test configuration, the settings needs to be changed again, otherwise the tests will fail again. Let's change the instructions to ask the developers to modify the default test configuration instead of the generated one.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.