Required and Science command #97

yan-soon · 2020-10-21T18:14:23Z

RequiredCommand

Added Required Command that showcases all the leftover modules to take after cross referencing against user's GradPad
Can be accessed by typing 'required'
Modules are split into 5 various categories
1. Foundation
2. IT Professionalism
3. Math And Science
4. Science
5. Industrial Experience
Respective modules can be found under data folder
If user has completed all modules, a success message will be shown for each category(eg. "You have completed all your Foundation Modules!")
Focus Area modules have not been added (KIV)
For Industrial Experience, 12MCs are needed, thus, the required command also calculates the current MC the user is at at reflects it in the Command Line Display

ScienceCommand

-Added Science command that displays all the available Science modules.
-Can be accessed by typing 'science'

Respective modules can be found under data folder as well

General Design Approach

-The RequiredCommand and ScienceCommand have various attributes to help store the modules, namely a ReadOnlyGradPad for storage of modules and an actual ObservableList to do the comparisons with.
-Each module category will have a method to set up the module storage(eg. setFoundationStorage) and compare that storage against the user's currents modules (compareFoundation) to see if the user is missing anything out.
-The execute method then invokes all the various methods to set up the storages and does the comparisons, keeping track of all the undone modules, before displaying it to the user.

Notes

Since the command works by retrieving the stored modules in a JSON file and that the RequiredCommand extends the Command class, I had to change the execute method, to allow it to throw IOExceptions and DataConversionExceptions so that it can handle exceptions if the JSON file is not found. As a result, I changed a couple methods to throw IOExceptions and DataConversionExceptions, but this should not affect the general flow of the app.

Silvernitro

Great job in general man, the logic is sound and the approach really makes sense, but just a few comments/suggestions :D

And also some general comments/suggestions:

We can consider abstracting out all the storage access logic in RequiredCommand into a separate class like RequiredCommandStorage. This way, RequiredCommand becomes more lean since it doesn't need to store storage fields and their setters/getters. It also adheres to the single-responsibility and open-closed principle cos RequiredCommand doesn't need to care abt how it gets the data, and we are free to change the storage logic without touching RequiredCommand next time. RequiredCommand can then call stuff like .getFoundationModules()
We can't store the JSON files in the main/data directory cos they won't be bundled by ShadowJar and the smoke tests won't work properly. They need to be put in the resources folder instead. BUT ITS OK I also just realised this cos the modules.json in Nusmods needs to move too oooops

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

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

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

Silvernitro · 2020-10-22T09:07:52Z

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

+    public void setFoundationStorageInvalidPath_returnsEmptyOptional() throws IOException, DataConversionException {
+        requiredCommand.setFoundationStorage(INVALID_PATH);
+        Optional<ReadOnlyGradPad> actual = requiredCommand.getFoundationStorage();
+        assertEquals(actual, Optional.empty());


Perhaps we can consider using this instead:

Suggested change

assertEquals(actual, Optional.empty());

assertTrue(actual.isEmpty());

As mentioned in the java API, it's generally not recommended to do equality checks for Optionals cos they produce unpredictable results 😁 Same applies for the other tests below and also ScienceCommandTest

okay roger doger! changed accordingly

yan-soon · 2020-10-22T20:03:42Z

As per Sam's reviews, I have made some changes to the PR.

The major changes are as such:

RequiredCommandStorage

Added this class to deal with all the pulling of modules from the respective JSON files.

RequiredCommandMessages

Added this class to store all the standard messages and paths that the RequiredCommand will use.

RequiredCommand

Changed the class such that it no longer has to deal with storage of the required modules! It now simply uses Getter methods from the RequiredCommandStorage to get the respective modules from the various fields.
The class now mainly deals with all the comparison of GradPad modules against all the required modules.

ScienceCommand

Changed the class slightly to improve the overall performance and design

RequiredCommandTest

Changed the tests to suit the new methods

RequiredCommandStorageTest

Added the test class to test the new RequiredCommandStorage class.

shaokiat

LGTM! Nice one Yan! 💯💯💯

Silvernitro

Thanks for addressing the previous comments! The code looks great :D Just a few more clarifications cos I got confused at some parts ><

Silvernitro · 2020-10-23T08:37:48Z

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

+        for (Module module : requiredScience) {
+            if (doesModuleAlreadyExist(module, currentModules)) {
+                leftOverModules += MESSAGE_SUCCESS_SCIENCE + "\n";
+                areModulesCleared = false;


Shouldn't areModulesCleared become true when we find out that the user has taken a science module?

accidentally flipped the booleans HAHAH changed the conditions and booleans accordingly

Silvernitro · 2020-10-23T08:38:35Z

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

+        } if (areModulesCleared) {
+            leftOverModules += MESSAGE_SCIENCE + "\n";


Likewise if areModulesCleared is true then shouldn't we be showing MESSAGE_SUCCESS_SCIENCE instead since the user has cleared this basket HAHA

Silvernitro · 2020-10-23T08:41:06Z

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

+            if (doesModuleAlreadyExist(module, currentModules)) {
+                int modularCredits = Integer.parseInt(module.getModularCredits().toString());
+                modularScore += modularCredits;
+                areModulesCleared = false;


How come taking a single internship module means that areModulesCleared is false ahh? I tot it should only be false if total MCs < 12 since we need >= 12 in order to clear the internship basket ><

I FEEL LIKE IM MISSING SOMETHING OBVIOUS LOL SO PLS FORGIVE ME IF I AM HAHA IM DAMN CONFUSED

sorry for the misleading boolean names AHAHHA changed the whole code, so no more booleans, just an If-else block to settle if the module is present or not

Silvernitro · 2020-10-23T08:49:03Z

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

+            boolean areModulesCleared = true;
+            if (doesModuleAlreadyExist(module, currentModules)) {
+                int modularCredits = Integer.parseInt(module.getModularCredits().toString());
+                modularScore += modularCredits;
+                areModulesCleared = false;
+            }
+            if (areModulesCleared) {
+                String moduleToAdd = module.getModuleCode() + " (" + module.getModularCredits() + " MCs)";
+                leftOverInternship.append("\n").append(moduleToAdd);
+            }


Actually how come we need the areModulesClearedflag instead of using an if-else statement ahhh?

Just to clarify, is this code saying: if a module is taken, then add its MCs to modularScore. If it's not, add the module title/mc to the result to display instead.

PAISEH I'm very confused here ><

Suggested change

boolean areModulesCleared = true;

if (doesModuleAlreadyExist(module, currentModules)) {

int modularCredits = Integer.parseInt(module.getModularCredits().toString());

modularScore += modularCredits;

areModulesCleared = false;

}

if (areModulesCleared) {

String moduleToAdd = module.getModuleCode() + " (" + module.getModularCredits() + " MCs)";

leftOverInternship.append("\n").append(moduleToAdd);

}

if (doesModuleAlreadyExist(module, currentModules)) {

int modularCredits = Integer.parseInt(module.getModularCredits().toString());

modularScore += modularCredits;

} else {

String moduleToAdd = module.getModuleCode() + " (" + module.getModularCredits() + " MCs)";

leftOverInternship.append("\n").append(moduleToAdd);

}

codecov-io · 2020-10-23T09:17:15Z

Codecov Report

Merging #97 into master will increase coverage by 0.39%.
The diff coverage is 78.68%.

@@             Coverage Diff              @@
##             master      #97      +/-   ##
============================================
+ Coverage     73.31%   73.70%   +0.39%     
- Complexity      424      460      +36     
============================================
  Files            76       79       +3     
  Lines          1274     1392     +118     
  Branches        123      141      +18     
============================================
+ Hits            934     1026      +92     
+ Misses          302      293       -9     
- Partials         38       73      +35

Impacted Files	Coverage Δ	Complexity Δ
...java/seedu/address/logic/parser/GradPadParser.java	`85.00% <0.00%> (-15.00%)`	`13.00 <0.00> (ø)`
src/main/java/seedu/address/ui/UiManager.java	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
.../seedu/address/logic/commands/RequiredCommand.java	`70.83% <70.83%> (ø)`	`14.00 <14.00> (?)`
...seedu/address/storage/RequiredCommandMessages.java	`83.33% <83.33%> (ø)`	`1.00 <1.00> (?)`
...a/seedu/address/logic/commands/ScienceCommand.java	`87.50% <87.50%> (ø)`	`5.00 <5.00> (?)`
.../seedu/address/storage/RequiredCommandStorage.java	`100.00% <100.00%> (ø)`	`11.00 <11.00> (?)`
...el/module/ModuleCodeContainsKeywordsPredicate.java	`77.77% <0.00%> (-22.23%)`	`8.00% <0.00%> (+1.00%)`	⬇️
...java/seedu/address/nusmods/NusmodsDataManager.java	`80.00% <0.00%> (-20.00%)`	`7.00% <0.00%> (+1.00%)`	⬇️
...java/seedu/address/logic/commands/FindCommand.java	`81.81% <0.00%> (-18.19%)`	`7.00% <0.00%> (+1.00%)`	⬇️
src/main/java/seedu/address/model/GradPad.java	`78.57% <0.00%> (-13.10%)`	`13.00% <0.00%> (+1.00%)`	⬇️
... and 15 more

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 3495472...15ba3ac. Read the comment docs.

…n/tp into required-modules-command

yan-soon added 10 commits October 20, 2020 12:37

add basic RequiredCommand function for some modules

10b845f

add RequiredCommand for Foundation, Science and Internship modules

0e4e890

add RequiredCommand for ITProf and Math&Sci modules

cb31214

add javadocs for methods under RequiredCommand

9ddd566

refine ScienceCommand and add javadocs for its methods

0fcf607

delete some files

eac4e2f

update ScienceCommand and add tests

3a85bbb

update RequiredCommand and add some tests

b64843f

add all tests for RequiredCommand

6d6f3ad

fix checkstyle

b6c1333

yan-soon added type.Task priority.Medium labels Oct 21, 2020

yan-soon added this to the v1.3 milestone Oct 21, 2020

yan-soon requested a review from a team October 21, 2020 18:14

yan-soon self-assigned this Oct 21, 2020

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

Implement Required and Science command #96

Closed

yan-soon added 5 commits October 22, 2020 02:16

fix bugs

972a9df

fix typos

5c8a256

add various module JSON data files

e305a94

fix checkstyle

2a27524

fix bugs regarding file directories

20a536a

Silvernitro reviewed Oct 22, 2020

View reviewed changes

fix changes brought up by team

0b9255e

edited RequiredCommand upon team review

41afcad

shaokiat approved these changes Oct 23, 2020

View reviewed changes

Silvernitro reviewed Oct 23, 2020

View reviewed changes

fix typos according to team review

15ba3ac

Silvernitro approved these changes Oct 23, 2020

View reviewed changes

shift module data into resources folder

c1b73da

yan-soon added the hacktoberfest-accepted label Oct 23, 2020

yan-soon and others added 3 commits October 23, 2020 17:41

Merge branch 'master' into required-modules-command

39e53f3

fix test cases

d4e5f74

Merge branch 'required-modules-command' of https://github.com/yan-soo…

ad338e1

…n/tp into required-modules-command

yan-soon merged commit 4f7d1aa into AY2021S1-CS2103T-T09-1:master Oct 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Required and Science command #97

Required and Science command #97

yan-soon commented Oct 21, 2020

Silvernitro left a comment •

edited

Silvernitro Oct 22, 2020

yan-soon Oct 22, 2020 •

edited

yan-soon commented Oct 22, 2020

shaokiat left a comment

Silvernitro left a comment

Silvernitro Oct 23, 2020

yan-soon Oct 23, 2020

Silvernitro Oct 23, 2020

Silvernitro Oct 23, 2020

yan-soon Oct 23, 2020

Silvernitro Oct 23, 2020 •

edited

codecov-io commented Oct 23, 2020

	assertEquals(actual, Optional.empty());
	assertTrue(actual.isEmpty());

		} if (areModulesCleared) {
		leftOverModules += MESSAGE_SCIENCE + "\n";

Required and Science command #97

Required and Science command #97

Conversation

yan-soon commented Oct 21, 2020

Silvernitro left a comment • edited

Choose a reason for hiding this comment

Silvernitro Oct 22, 2020

Choose a reason for hiding this comment

yan-soon Oct 22, 2020 • edited

Choose a reason for hiding this comment

yan-soon commented Oct 22, 2020

shaokiat left a comment

Choose a reason for hiding this comment

Silvernitro left a comment

Choose a reason for hiding this comment

Silvernitro Oct 23, 2020

Choose a reason for hiding this comment

yan-soon Oct 23, 2020

Choose a reason for hiding this comment

Silvernitro Oct 23, 2020

Choose a reason for hiding this comment

Silvernitro Oct 23, 2020

Choose a reason for hiding this comment

yan-soon Oct 23, 2020

Choose a reason for hiding this comment

Silvernitro Oct 23, 2020 • edited

Choose a reason for hiding this comment

codecov-io commented Oct 23, 2020

Codecov Report

Silvernitro left a comment •

edited

yan-soon Oct 22, 2020 •

edited

Silvernitro Oct 23, 2020 •

edited