Skip to content

[CS2113-T15-2] SecureNUS#32

Open
ollayf wants to merge 432 commits into
nus-cs2113-AY2223S2:masterfrom
AY2223S2-CS2113-T15-2:master
Open

[CS2113-T15-2] SecureNUS#32
ollayf wants to merge 432 commits into
nus-cs2113-AY2223S2:masterfrom
AY2223S2-CS2113-T15-2:master

Conversation

@ollayf

@ollayf ollayf commented Mar 2, 2023

Copy link
Copy Markdown

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

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

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

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 represent the classes using the standard representation adopted by CS2113 instead of 3-dimensional boxes?

Comment thread docs/DGDiagramsCreator/ClassHigh.puml Outdated
frame Backend {
node Backend
}
Commands --> SecureNUS.SecureNUS

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 include details as part of the arrows for additional clarity?

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

autonumber

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

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 like the clarity described by the arrows in the diagram.

@ysl-28 ysl-28 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.

DG is generally clear and understandable

Comment thread docs/DeveloperGuide.md Outdated

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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?

Comment thread docs/DeveloperGuide.md Outdated

<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
Copy Markdown

Choose a reason for hiding this comment

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

Typo for instantiated.

Comment thread docs/DeveloperGuide.md Outdated
<img src="./DGDiagramsCreator/DGUsedDiagrams/ArchitectureDiagram.png" width="100%" />


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

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 use the standard CS2113 representation for diagrams? (instead of using the symbols)

Comment thread docs/DeveloperGuide.md Outdated
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

Copy link
Copy Markdown

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.

Comment thread docs/DeveloperGuide.md Outdated
* is reasonably comfortable using CLI apps


## Value proposition:

Copy link
Copy Markdown

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.

Comment thread docs/DeveloperGuide.md Outdated

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

## SecretStorage 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.

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

Comment thread docs/DeveloperGuide.md Outdated

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

@kaceycsn kaceycsn Mar 29, 2023

Copy link
Copy Markdown

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