-
Notifications
You must be signed in to change notification settings - Fork 248
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-F10-2] BadMaths #46
base: master
Are you sure you want to change the base?
[CS2113-F10-2] BadMaths #46
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps more diagrams (class/architectural/sequence can be added to show the overall flow of the code. There can also be more details given in the scope, user stories, glossary and such if it is not included
docs/DeveloperGuide.md
Outdated
1. This is step 1. | ||
2. This is step 2. | ||
|
||
![img.png](img.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this format of the sequence diagram correct? Should there be colon in the class such as :Notes?
docs/DeveloperGuide.md
Outdated
|
||
|
||
## Product scope | ||
## Product Scope | ||
### Target user profile | ||
|
||
{Describe the target user profile} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps include target user profile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very clear and detailed explanation given for your features! Would be good to read about why you implemented your logic this way, but great work nonetheless!! :)
docs/QuadraticSolver.puml
Outdated
Quadratic -> Quadratic: findA() | ||
activate Quadratic #FFBBBB | ||
deactivate Quadratic | ||
Quadratic -> Quadratic: findB() | ||
activate Quadratic #FFBBBB | ||
deactivate Quadratic | ||
Quadratic -> Quadratic: findC() | ||
activate Quadratic #FFBBBB | ||
deactivate Quadratic | ||
Quadratic -> Quadratic: quadraticFormula() | ||
activate Quadratic #FFBBBB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you can adjust the method call arrows for findA()
, findB()
, findC()
and quadraticFormula()
such that they point to the start of the corresponding activation bars, like what they arrow for printAnswer()
does
docs/QuadraticSolver.puml
Outdated
deactivate UI | ||
end opt | ||
deactivate Quadratic | ||
Quadratic -> Command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe in this case, the arrow should be a dashed arrow since it represents a method return. Also, you should try to connect the method return arrow to exactly the end of the activation bar for Quadratic
docs/Notes.puml
Outdated
deactivate Parser | ||
alt inputCommand is null | ||
activate Command | ||
BadMaths -> Command : Command(command, toDo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this method invocation arrow should be connected to the constructor activation bar instead rather than a regular method activation bar. Maybe try adding create Command
to display the constructor activation bar earlier in the PUML file?
docs/Notes.puml
Outdated
Command -> Quadratic : Quadratic(toDo) | ||
activate Quadratic | ||
deactivate Quadratic | ||
alt "Bye" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can phrase it more like a condition, e.g. command is "Bye"
, so that it's clearer what this "Bye" is referring to
docs/Notes.puml
Outdated
activate Quadratic | ||
deactivate Quadratic | ||
alt "Bye" | ||
else "Graph" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment as for the "Bye" condition above
## Acknowledgements | ||
|
||
{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well} | ||
|
||
|
||
## Design & implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concise and clear explanation of what your features does in this section! Consider explaining why you implemented your feature logic this way, maybe even describing some alternative implementations you considered but ultimately dropped, which would strengthen the appeal of your features to a developer.
docs/TrigoGraph.puml
Outdated
activate TrigoGraphAnalyser | ||
deactivate TrigoGraphAnalyser | ||
|
||
TrigoGraph -> TrigoGraph |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this arrow should be a dashed arrow instead since it's a method return
docs/TrigoGraph.puml
Outdated
TrigoGraph -> TrigoGraph | ||
deactivate TrigoGraph | ||
|
||
TrigoGraph -->> TrigoGraphVisualiser: TrigoGraphVisualiser(amplitude,phase,frequency,verticalShift,trig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a solid arrow since it's a method call
docs/TrigoGraph.puml
Outdated
activate Ui | ||
deactivate Ui | ||
|
||
TrigoGraph -> TrigoGraph |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a dashed arrow since it's a method return
|
||
#### TrigoGraph class: | ||
![img_1.png](img_1.png) | ||
Step 2. Constructor for the TrigoGraph class takes in `2*sin(2*x+5)-1` and assigns it to `trigoEqn` of type String. When `startGraphAnalysis()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to insert another newline before Step 2.
because the Step 2.
line currently begins beside the UML diagram
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, table of contents and format look great, maybe more could be put into the the return arrow back for sequence diagrams.
docs/DeveloperGuide.md
Outdated
Step 5. If any exceptions are caught in the above steps, `printQuadraticFormulaError` would be called from UI to show an error message | ||
to the user. | ||
|
||
![img_3.png](img_3.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DG for the line quadraticFormula(), I think it would be better to include the variables to become something like
quadraticFormula(a,b,c)
docs/DeveloperGuide.md
Outdated
Step 5. If any exceptions are caught in the above steps, `printQuadraticFormulaError` would be called from UI to show an error message | ||
to the user. | ||
|
||
![img_3.png](img_3.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to also add return statements, such as the variables names the functions are returning for such as the findA(), findB(), and findC() fuctions
docs/DeveloperGuide.md
Outdated
which will be stored in a list, and to display a list of all notes | ||
stored by users. This functionality is achieved through the `Store.` and `List.` commands. | ||
|
||
![img_2.png](img_2.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think getCommand and getToDo for the DG at the top, the arrow should be the other way round cause it is badmaths that is calling parser's function, and the calls should have a separate activation bar
docs/DeveloperGuide.md
Outdated
which will be stored in a list, and to display a list of all notes | ||
stored by users. This functionality is achieved through the `Store.` and `List.` commands. | ||
|
||
![img_2.png](img_2.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for trigoGraph(), Calculator() and Quadratic(), they are constructors which make a new object, would it be better to show it in the Sequence Diagram by linking it straight to the bar instead of those objects already existing?
Implement JUnit Test for CommandHistory.java
update imports for NotesFileWriterTest
Added PPP
Reflect tp review for the matrix part
Implement DeleteTest.java
Reflect tp review for the matrix part
Update draft PPP
Added Store Sequence Diagram
add header comment
Seq diagram rectify
Add Sequence Diagram for Store
Rectify UI output command for Invalid Number Entered
Update UserGuide.md
Refine Seungjun's PPP
Fix format
handle indentation problem
Update DG
Add JavaDoc comments for the matrix part
change main README.md to BadMaths version
BadMaths is a quick lookup tool for trigonometry graphs and basic matrix operations that aims to help students save time when doing maths assignments.