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-T15-2] SecureNUS #32

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

Conversation

ollayf
Copy link

@ollayf ollayf commented Mar 2, 2023

SecureNUS is a desktop CLI app for managing and storing passwords. SecureNUS provides the functionalities of a regular password manager tool, but on CLI. Simple and efficient to use especially for fast typists.

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

@Thunderdragon221 Thunderdragon221 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 DG with minor formatting errors.

!include Style.puml
title UI Component Diagram
actor User
node SecureNUS

Choose a reason for hiding this comment

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

Maybe represent the classes using the standard representation adopted by CS2113 instead of 3-dimensional boxes?

frame Backend {
node Backend
}
Commands --> SecureNUS.SecureNUS

Choose a reason for hiding this comment

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

Maybe include details as part of the arrows for additional clarity?

@startuml
'https://plantuml.com/sequence-diagram

autonumber

Choose a reason for hiding this comment

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

Maybe omit autonumbering to follow the standard practice adopted by CS2113?

SecretMaster -[dotted]-> SecretSearcher: alters
}
() SecureNUS -[dotted]right-> SecretMaster :requests alteration \n of data via
SecretMaster <-[dotted]right-> Backend : sends data for export OR\n collects data for import

Choose a reason for hiding this comment

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

I like the clarity described by the arrows in the diagram.

Copy link

@ysl-28 ysl-28 left a comment

Choose a reason for hiding this comment

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

DG is generally clear and understandable


The Sequence Diagram below shows how the components interact with each other for the scenario where the user creates a new basic password initiated using the command `new password.`

Sequence Diagram
Copy link

Choose a reason for hiding this comment

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

It might not be too clear that methods like extractName() and inquireURL() are only being called in AddBasicPassword and not in the other classes that inherit from Command.

Copy link

Choose a reason for hiding this comment

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

For methods like inquireURL() that return a string, perhaps the return arrows should be labelled with that they are returning?

Copy link

Choose a reason for hiding this comment

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

Might it be clearer to show object deletion at the end of its lifeline too?


<img src="./DGDiagramsCreator/DGUsedDiagrams/CommandComponent.png" width="100%" />

The Command consist of Command abstract class that handles all of its command constructors and executions through its child classes. The user inputs command in Ui, that is parsed in Parser, and then institiated in and then executed in Command classes.
Copy link

Choose a reason for hiding this comment

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

Typo for instantiated.

<img src="./DGDiagramsCreator/DGUsedDiagrams/ArchitectureDiagram.png" width="100%" />


This **Architecture Diagram** explains the high-level design of the App.
Copy link

Choose a reason for hiding this comment

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

Perhaps use the standard CS2113 representation for diagrams? (instead of using the symbols)

Comment on lines 173 to 182
## Target user profile:



* has a need to manage a significant number of passwords
* prefer desktop apps over other types
* can type fast
* prefers typing to mouse interactions
* is reasonably comfortable using CLI apps

Choose a reason for hiding this comment

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

May consider being more specific when defining the target users, and elaborate how the value proposition matches the needs of the target users.

* is reasonably comfortable using CLI apps


## Value proposition:

Choose a reason for hiding this comment

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

May consider defining the scope more clearly by specifying the boundary beyond which the app will not help.


This is the main component that initialises all other components and connects them when the application is running.

## SecretStorage Component

Choose a reason for hiding this comment

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

I like how the system is divided into the separate components in the documentation. The component diagrams are clear and simple.


The Sequence Diagram below shows how the components interact with each other for the scenario where the user creates a new basic password initiated using the command `new password.`

Sequence Diagram
Copy link

@kaceycsn kaceycsn Mar 29, 2023

Choose a reason for hiding this comment

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

In the sequence diagram, a semicolon should be added before the class name. Unnamed instances of a class should be denoted as :Class.

ollayf and others added 22 commits March 29, 2023 17:26
trying to resolve inf loop: 1
Remove infinite loop in github tests
# Conflicts:
#	src/test/java/seedu/duke/command/AddCommandTest.java
#	src/test/java/seedu/duke/command/DeleteCommandTest.java
# Conflicts:
#	src/main/java/seedu/duke/Backend.java
#	src/main/java/seedu/duke/Parser.java
#	src/main/java/seedu/duke/exceptions/secrets/FolderExistsException.java
#	src/main/java/seedu/duke/secrets/CreditCard.java
#	src/test/java/seedu/duke/DukeTest.java
# Conflicts:
#	src/main/java/seedu/duke/Backend.java
#	src/main/java/seedu/duke/command/ListCommand.java
#	src/main/java/seedu/duke/exceptions/secrets/FolderExistsException.java
#	src/main/java/seedu/duke/secrets/CreditCard.java
#	src/test/java/seedu/duke/DukeTest.java
#	src/test/java/seedu/duke/command/ExitCommandTest.java
Checkstyle fixes (incl. remove ExitCommandTest)
ollayf and others added 30 commits April 10, 2023 20:15
Convert current Jarfile name to SecureNUS.jar
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.

10 participants