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-F10-2] BadMaths #46

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

Conversation

YC-Michael
Copy link

@YC-Michael YC-Michael commented Mar 3, 2023

BadMaths is a quick lookup tool for trigonometry graphs and basic matrix operations that aims to help students save time when doing maths assignments.

kyrixn added a commit to kyrixn/tp that referenced this pull request Mar 16, 2023
Copy link

@wuzhzn wuzhzn left a 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

1. This is step 1.
2. This is step 2.

![img.png](img.png)
Copy link

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?



## Product scope
## Product Scope
### Target user profile

{Describe the target user profile}
Copy link

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

Copy link

@haoyangw haoyangw left a 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!! :)

Comment on lines 8 to 18
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

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

deactivate UI
end opt
deactivate Quadratic
Quadratic -> Command

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);

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"

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"

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

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.

activate TrigoGraphAnalyser
deactivate TrigoGraphAnalyser

TrigoGraph -> TrigoGraph

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

TrigoGraph -> TrigoGraph
deactivate TrigoGraph

TrigoGraph -->> TrigoGraphVisualiser: TrigoGraphVisualiser(amplitude,phase,frequency,verticalShift,trig)

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

activate Ui
deactivate Ui

TrigoGraph -> TrigoGraph

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()`

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

Copy link

@SSzeWen SSzeWen left a 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.

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)
Copy link

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)

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)
Copy link

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

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)
Copy link

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

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)
Copy link

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?

WilsonLee2000 and others added 30 commits April 10, 2023 20:05
Rectify UI output command for Invalid Number Entered
Add JavaDoc comments for the matrix part
change main README.md to BadMaths version
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