Refactor commands #35

yan-soon · 2020-10-05T18:02:49Z

Refactored Command, Parser and Storage packages.

Changes to Production Classes:

Name -> ModuleCode
Phone -> ModularCredits
Deleted Email and Address fields

Person -> Module

EditPersonDescriptor -> EditModuleDescriptor

AddressBookParser -> GradPadParser
AddressBookStorage -> GradPadStorage
JsonAdaptedPerson -> JsonAdaptedModule
JsonAddressBookStorage -> JsonGradPadStorage

Changes to Test Classes:

EditPersonDescriptorTest -> EditModuleDescriptorTest

AddressBookParserTest -> GradPadParserTest
JsonAdaptedPersonTest -> JsonAdaptedModuleTest
JsonAddressBookStorageTest -> JsonGradPadStorageTest
JsonSerializableAddressBookTest -> JsonSerializableGradPadTest

Changes to Test Util:

EditPersonDescriptorBuilder -> EditModuleDescriptorBuilder
AddressBookBuilder -> GradPadBuilder

Changes to test/data folders and files:

JsonAddressBookStorageTest -> JsonGradPadStorageTest (FOLDER)
invalidAndValidPersonAddressBook.json -> invalidAndValidModuleGradPad.json
invalidPersonAddressBook.json -> invalidModuleGradPad.json
notJsonFormatAddressBook -> notJsonFormatGradPad

JsonSerializableAddressBookTest -> JsonSerializableGradPadTest (FOLDER)
duplicatePersonAddressBook.json -> duplicateModuleGradPad.json
invalidPersonAddressBook.json -> invalidModuleGradPad.json
typicalPersonsAddressBook.json -> typicalModulesGradPad.json

Contents of the test data has also been changed from Person to Module

Notes

When refactoring, changed some names from the UI package (person -> module, name -> modulecode, etc).
Currently passes all tests from the Command and Parser packages and passes most of the tests from the Storage package (currently failing 3 tests)

…ekyll-config Patch site-wide settings

…09-1/tp

src/main/java/seedu/address/logic/parser/ParserUtil.java

src/main/java/seedu/address/logic/commands/AddCommand.java

src/main/java/seedu/address/logic/parser/EditCommandParser.java

src/main/java/seedu/address/logic/commands/EditCommand.java

src/main/java/seedu/address/logic/commands/DeleteCommand.java

Silvernitro

This review covers the rest of the production classes (I haven't reviewed the tests yet).

Also the getAddressBookFilePath method in the Logic interface/LogicManager class might have been missed out in the refactoring hehe but I couldn't leave a comment cos those lines weren't marked as diffs.

src/main/java/seedu/address/storage/JsonAdaptedModule.java

src/main/java/seedu/address/storage/JsonSerializableAddressBook.java

Silvernitro · 2020-10-06T04:40:18Z

src/main/java/seedu/address/storage/JsonSerializableAddressBook.java

+        for (JsonAdaptedModule jsonAdaptedModule : modules) {
+            Module module = jsonAdaptedModule.toModelType();
+            if (gradPad.hasModule(module)) {
+                throw new IllegalValueException(MESSAGE_DUPLICATE_MODULE);


Not a suggestion/request for change, but I was just wondering why the original code does this redundant check here when it could just call gradPad's setModules fxn, which already uses UniqueModuleList to ensure all modules are unique.

I just put this comment here first in case we want to improve code quality next time.

Suggested change

for (JsonAdaptedModule jsonAdaptedModule : modules) {

Module module = jsonAdaptedModule.toModelType();

if (gradPad.hasModule(module)) {

throw new IllegalValueException(MESSAGE_DUPLICATE_MODULE);

List<Modules> gradPadModules = modules.stream().map(JsonAdaptedModule::toModelType).collect(Collectors.toList());

try {

gradpad.setModules(gradPadModules);

} catch (DuplicateModuleException ex) {

throw new IllegalValueException(MESSAGE_DUPLICATE_MODULE);

}

src/main/java/seedu/address/storage/JsonSerializableAddressBook.java

Silvernitro

Reviewed the test classes and mostly looks great man; just a few nits!

src/test/java/seedu/address/logic/parser/EditCommandParserTest.java

src/test/java/seedu/address/logic/parser/FindCommandParserTest.java

src/test/java/seedu/address/logic/commands/EditCommandTest.java

src/test/java/seedu/address/logic/commands/EditModuleDescriptorTest.java

src/test/java/seedu/address/testutil/EditModuleDescriptorBuilder.java

src/test/java/seedu/address/storage/JsonAdaptedModuleTest.java

src/test/data/JsonSerializableGradPadTest/invalidModuleGradPad.json

shaokiat

NICE ONE 👍

src/main/java/seedu/address/logic/Logic.java

src/main/java/seedu/address/logic/commands/AddCommand.java

src/test/java/seedu/address/logic/commands/DeleteCommandTest.java

src/test/java/seedu/address/logic/commands/EditCommandTest.java

src/test/java/seedu/address/logic/commands/FindCommandTest.java

src/test/java/seedu/address/testutil/EditModuleDescriptorBuilder.java

src/test/java/seedu/address/testutil/ModuleUtil.java

yan-soon · 2020-10-06T17:31:17Z

Great find guys, I've edited accordingly to your comments. Thanks :)

Silvernitro

Steady brother 👍 Thanks for addressing all the comments 💯

Silvernitro and others added 9 commits September 27, 2020 12:04

Update _config.yml

4f74753

Update index.md

c66c293

Merge pull request AY2021S1-CS2103T-T09-1#28 from Silvernitro/patch-j…

e2e7bcc

…ekyll-config Patch site-wide settings

Merge branch 'v1.2-refactor' of https://github.com/AY2021S1-CS2103T-T…

63f0207

…09-1/tp

Refactor Command and Parser Packages

6ec7220

refactor command and parser test classes

d380d64

fix testing bugs for commands and parser packages

866fe56

refactored storage and add tests

266b457

fix spelling errors

b73c7ec

yan-soon requested a review from a team October 5, 2020 18:02

yan-soon linked an issue Oct 5, 2020 that may be closed by this pull request

Refactor Commands, Parser and Storage Package #36

Closed