Skip to content

[CS2113-T12-1] Study It#33

Open
farice9 wants to merge 821 commits into
nus-cs2113-AY2021S1:masterfrom
AY2021S1-CS2113T-T12-1:master
Open

[CS2113-T12-1] Study It#33
farice9 wants to merge 821 commits into
nus-cs2113-AY2021S1:masterfrom
AY2021S1-CS2113T-T12-1:master

Conversation

@farice9

@farice9 farice9 commented Oct 1, 2020

Copy link
Copy Markdown

Study It helps manage your study-related matters. It can keep track of your various study matters, present them in an organized and thoughtful manner to help you organize your study life.

@ivanderjmw ivanderjmw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This developer guide contains many details, including specific methods. However, it looks a bit messy and disorganized in Github Pages. Some errors are also present in the diagrams. You can improve it much more!

Comment thread docs/DeveloperGuide.md
Comment on lines +46 to +48

![Bookmark Sequence Diagram](Images/sequence_bookmark.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are some problems in this diagram.

  • Overlapping of method calls.
  • Placement of conditional in alt block is not correct.
  • I am unsure of the use of the dotted line that cuts horizontally through alt block.
  • Self invocation is not drawn correctly.
  • Some actuation bars extends after the method returns a value.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Screenshot 2020-10-28 at 11 41 18 AM
Here are more details.

Comment thread docs/DeveloperGuide.md
Comment on lines +68 to +70

![BookmarkCommand ClassDiagram](Images/AddCommand_sequenceDiagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this an exact copy of the Bookmark Sequence diagram above?

Comment thread docs/DeveloperGuide.md
Comment on lines +33 to +35

![Bookmark Class Diagram](Images/BookmarkRun_Class.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It would be nice to have a label for each association.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can add lables under each figure.

Comment thread docs/DeveloperGuide.md
Comment on lines +62 to +64

![BookmarkCommand ClassDiagram](Images/bookmarkCommand_Class.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Some lines are detached. Use the inheritance arrow instead of association here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You might also need to use dotted lines.

Comment thread docs/DeveloperGuide.md
Comment on lines +115 to +117

![Flashcard Component Class Diagram](Images/Flashcard_Class.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Some arrows are not quite clear. Maybe you can point out both directions of multiplicity.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Are Flashcard and FlashcardSotorage abstract? Would be nice if you specify using the label {abstract}, if they are not, you can remove the italics.

Comment thread docs/DeveloperGuide.md
Comment on lines +152 to +155
1. SEVERE: A critical problem detected which may cause the termination of the application.
2. WARNING: Can continue, but with caution.
3. INFO: Information showing the noteworthy actions by the App.
4. FINE: Details that are not usually noteworthy but may be useful in debugging.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can emphasize the log message levels.

Comment thread docs/DeveloperGuide.md Outdated
This project uses code coverage that is in IntelliJ IDE to check for the coverage of the code.

### Value proposition
##Appendix: Requirement

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing

Comment thread docs/DeveloperGuide.md Outdated
Comment on lines +3 to +4
[to be filled]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Add an introduction to the developer guide.

Comment thread docs/DeveloperGuide.md Outdated
[to be filled]

## Setting up, Getting Started
###Setting up the project in your computer

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This header is incorrectly rendered in Github Pages

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can instead use a space before the title.

### Setting up...

instead of

###Setting up...

Comment thread docs/DeveloperGuide.md Outdated
Comment on lines +18 to +21
1. Configure the coding style.
Ensure Coding style is standardised throughout.
2. Learn the design.
When you are ready to start coding, we recommend that you get some

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think it would be good to specify more details? Learn which design, and what coding style to refer to.

@Mick609

Mick609 commented Oct 28, 2020

Copy link
Copy Markdown

You guys probably should change the name of the project to CS2113T-T12-1. Now the name (CS2113-T12-1) is missing a T.

@gua-guargia gua-guargia left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Overall, I think your developer guide is very concise and easy to follow. It will be better if you can standardize the size and resolution of the diagrams.

Comment thread docs/DeveloperGuide.md
### **Major components**
This section introduces the multiple components in Study It.

![](Images/ArchitectureDiagrams/components.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps you can consider the following format to standardize your diagrams which might will help with the document's neatness :
image

Comment thread docs/DeveloperGuide.md
This section will describe in detail how some features inside the academic tracker section have been implemented.
Figure x illustrates the general overview, the associations and the multiplicity of the academic classes.

![Academic_Class_Diagram](Images/Academic_Class_Diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The resolution of this image is a bit low, perhaps can export the diagram with a higher resolution?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps the diagram can be broken down into two separate parts. The :BookmarkRun, :TimetableRun, :AcademicRun and :FlashcardRun components could be grouped into a separate sub diagram and represented in the main diagram with a black-box. This will make the diagram more readable.

Comment thread docs/DeveloperGuide.md

![Academic_Class_Diagram](Images/Academic_Class_Diagram.png)

API: `java.academic`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should it be academic.java instead?

Comment thread docs/DeveloperGuide.md
### **Major components**
This section introduces the multiple components in Study It.

![](Images/ArchitectureDiagrams/components.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps can remove the small negative sign box on the top of every class box. It can be a little bit confusing.
image

Comment thread docs/DeveloperGuide.md Outdated
general commands, it'll be processed and provide output. Otherwise, the command will be relayed
to different components to be processed based on the software's current mode.

### **Main Component**

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This title 'Main Component' can be a little bit confusing with the previous title 'Major Components', and the content for these two sections also have some replications. Perhaps you can try to combine these two sections into one entity or introduce directly to the separated sub-components sections (i.e. skips the general 'main component' and directly shows the 'Bookmark Component' section)

Comment thread docs/DeveloperGuide.md Outdated
## Design & implementation
This section explains the architecture of our software and the design of each component.

### **Major components**

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should be in title case for consistency.

Comment thread docs/DeveloperGuide.md
### **Main Component**
This section will give an overview of the main component of Study It

![Main Component Diagram](Images/MainComponentDiagrams/maincomponent.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Diagram is quite difficult to read. Maybe a font change or size increase could increase the readability?

Comment thread docs/DeveloperGuide.md
Comment on lines +128 to +144
As there are multiple modes of our app (main menu/bookmark/timetable/academic/flashcard),
the Mode enumeration and a private static currentMode of Mode type is introduced to monitor the current
operating mode of StudyIt. This is to allow the program to respond to different commands under different modes.

Each command input by the user will be processed by CommandParser class to determine the command type.
This information, along with the original command string will be passed into Command class and executed under
executeCommand(). If it is one of the general commands, it will be processed by that class.
If it isn’t, the Command class will detect the current mode of the program and run that command string under that
specific mode, distributed under handleNonGeneralCommand() method. If it still doesn’t parse successfully,
it will be considered an invalid command and an error message will be printed out.

The classes Ui, ErrorMessage, HelpMessage and MainMenu help handle the printing of various user interfaces of the
program. ErrorMessage, HelpMessage and MainMenu are subclasses of Ui to make use of the methods in Ui.

StudyIt class will also initialize various instances of classes such as TimeTableRun, FlashCardRun,
ArrayList<BookmarkCategory>, ArrayList<Grade>, ArrayList<Person> and pass it to Command class to perform
each mode’s functionality.

@xuche123 xuche123 Oct 30, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe surround all classes like Ui, ErrorMessage and so on with ` ` instead of just StudyIt.

Comment thread docs/DeveloperGuide.md Outdated

![BookmarkCommand ClassDiagram](Images/BookmarkDG/AddCommand_sequenceDiagram.png)

###Timetable Component

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not rendered properly on github pages due to the missing space in between # and T

image

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.

9 participants