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

[CS2113-W14-3] Future Academic Planner (FAP) #58

Open
wants to merge 550 commits into
base: master
Choose a base branch
from

Conversation

dextboy
Copy link

@dextboy dextboy commented Mar 8, 2024

Future Academic Planner (FAP) is a robust academic management software designed to help streamline NUS Computer Engineering (CEG) student’s academic journey at NUS. If you can type fast, FAP can get your module planning done faster than your traditional GUI app.

yeozongyao added a commit to yeozongyao/tp that referenced this pull request Apr 2, 2024
…yao-rating-system

Add rating system for books. Rate books and list books based on good …
jensonjenkins and others added 24 commits April 2, 2024 22:10
…into update_UG_and_bugs

* 'master' of https://github.com/AY2324S2-CS2113T-W14-3/tp:
  Update UserGuide.md
  Remove "CS" grade argument from UserGuide
  Remove "CU" grade from occurence
  Update UserGuide.md
  Update UserGuide.md

# Conflicts:
#	docs/UserGuide.md
Update UG and fix bugs for desiredgpa command
Refactor FAP and Storage to improve code quality
Copy link

@EdmundTangg EdmundTangg left a comment

Choose a reason for hiding this comment

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

Overall good effort for DG. Just some minor touch ups and edit. Good job!

`GPA = SUM(Course Grade Point * Course Units) / SUM(Course Units Counted Towards GPA)`

Below is the sequence diagram for `ViewGpaCommand`.
![View Gpa Command Sequence Diagram](diagrams/ViewGpaCommand.png)

Choose a reason for hiding this comment

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

Overall good sequence diagram drawn.
For the ViewGpaCommand sequence diagram:

  • Can write what output is returned to the user
  • Is the log message and GpaNullException supposed to be returned to ModuleList? (No need to write them for the return line)

| Version | As a ... | I want to ... | So that I can ... |
|---------|----------|---------------------------|-------------------------------------------------------------|
| v1.0 | new user | see usage instructions | refer to them when I forget how to use the application |
| v2.0 | user | find a to-do item by name | locate a to-do without having to go through the entire list |

Choose a reason for hiding this comment

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

Are there sufficient user stories?


## Product scope

### Target user profile

{Describe the target user profile}

Choose a reason for hiding this comment

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

Is the target user profile and value proposition updated?

System.out.println("Module MC: " + jsonManager.getModuleMC());
}
```
6. **Viewing modules left to graduate**

Choose a reason for hiding this comment

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

Is it possible to include sequence diagrams for each of the command class for visualisation?

System.out.println("Module MC: " + jsonManager.getModuleMC());
}
```
6. **Viewing modules left to graduate**

Choose a reason for hiding this comment

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

Should there be a C/E beside ur class names? (Should remove the C and E logos)
Are the symbols appropriate? (Should remove the green circles and red squares with + and - for public and private attributes and methods) -> applies to all diagrams.

Copy link

@z-wenqing z-wenqing left a comment

Choose a reason for hiding this comment

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

Very detailed DG!

Loop For each Module
ModuleList -> Module:
activate Module
Module -> Module: Check if grade is countable

Choose a reason for hiding this comment

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

For objects, must write :ClassName .

Parser --> Command : returns
FAP ..> Command : uses

@enduml

Choose a reason for hiding this comment

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

The circle representing the class does not comply with the standard. Should off it in plantUML.

@@ -0,0 +1,34 @@
@startuml
class FAP {
- static moduleList: ModuleList

Choose a reason for hiding this comment

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

Use "+" and "-" to represent public and private attributes, not squares and cirlces generated by plantUML.

GpaNullException --> ModuleList: GpaNullException
deactivate GpaNullException
ModuleList --> ViewGpaCommand: GpaNullException
else Calculate GPA

Choose a reason for hiding this comment

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

The square bracket here should be the condition (eg. else)?

Copy link

@dtaywd dtaywd 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 overall, the developer guide was clear and easy to read. However, you can consider giving a brief description about what your application does at the beginning to give clarity on the classes that you introduce later. Also you can consider removing some of the default settings of your plantuml class and sequence diagrams.

```
#### UML Diagram

![FAP class diagram](diagrams/FAP.png)
Copy link

Choose a reason for hiding this comment

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

When defining classes in the class diagram, you can consider using '-' and + should be used to denote private and public attributes and methods respectively instead of using the default settings of plantUML.


```java
JsonManager jsonManager = new JsonManager();
if (jsonManager.moduleExist("CS1010")) {
Copy link

Choose a reason for hiding this comment

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

You can consider using try catch when checking in the module exists

`GPA = SUM(Course Grade Point * Course Units) / SUM(Course Units Counted Towards GPA)`

Below is the sequence diagram for `ViewGpaCommand`.
![View Gpa Command Sequence Diagram](diagrams/ViewGpaCommand.png)
Copy link

Choose a reason for hiding this comment

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

Is it possible to crop out the the classes that appear at the bottom of the sequence diagram?

us **safety in the arguments** that passes through to the commands via the userInput.
<br />
<br />
**Developer usage FAP: Parser & CommandMetadata class as of v2.0**: **How to create a new command**
Copy link

Choose a reason for hiding this comment

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

Great job in going the extra mile in describing the way to maintaining and adding to the codebase for future developers!

Comment on lines 356 to 361
### UML Diagram

A simplified UML diagram for the Storage class and its interaction with the Module, User, and exception classes is shown below:
- ![View Storage Class](diagrams/Storage.png)

### Integration with FAP
Copy link

Choose a reason for hiding this comment

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

You can consider editing this part of the developer guide as the "###" used here is not recognised as the "header"

fungg0 and others added 30 commits April 15, 2024 19:07
…into deliverables_update

* 'master' of https://github.com/AY2324S2-CS2113T-W14-3/tp:
  adhere to gradle checkstyle
  add javaDoc and PPP
  Add picture
  Update PPP and AboutUs
* 'master' of https://github.com/AY2324S2-CS2113T-W14-3/tp: (63 commits)
  add image to about us
  Amended aboutus page
  Test table for aboutus
  adhere to gradle checkstyle
  Add more page breaks
  add javaDoc and PPP
  Test line break
  Add picture
  Update PPP and AboutUs
  Amendments do DG
  Update DG and PPP
  Modify activation line for parser sequence diagram
  Update DG target user and glossary
  Update diagrams
  Add photo and update AboutUs and PPP
  Include proper formatting for PPP
  Resolve merge conflict
  Add more Javadocs
  Include contributions to PPP
  Update PPP and AboutUs
  ...
WenWu PPP update and synced root README to docs README
Update Ui photo in readme
Update PPP for Thaw and Dexter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.