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

[CS2113T-T09-1] ConTech #24

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

Conversation

marcusbory
Copy link

ConTech is a Command Line Interface (CLI) app for people in the computing industry (who prefer CLI) to manage computing-related contacts. It is optimised for CLI users so that they are able to access their contacts' information (ie Github ID, LinkedIn profile, Email, etc) quickly using commands.

docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
`Contact`.


## <a name="implementation"></a>Implementation

Choose a reason for hiding this comment

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

i think for each functionality, adding some sample inputs & outputs is much clear?

Copy link

@Mick609 Mick609 left a comment

Choose a reason for hiding this comment

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

Maybe you could use more types of UML diagrams in the DG. For example, object diagrams.

docs/DeveloperGuide.md Show resolved Hide resolved

![Main Parser Sequence Diagram](images/MainParserSequenceDiagram.png)

### <a name="command"></a>Command
Copy link

Choose a reason for hiding this comment

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

Sections under “Design” seems not finished. Include “Command”, “ContactList”, and “TextUi”. Please fill them in soon.

Copy link

Choose a reason for hiding this comment

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

For the Command section can consider add in the Class diagram to relate all the commands together


![Edit Sequence Diagram](images/EditContactCommandSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

image
This diagram is a bit too big and contains a bit too many details. It would be hard to view this in the FDF format. If all the implementation is necessary for the illustration, the loop in this diagram maybe can be placed in a separate diagram as reference.

Copy link

Choose a reason for hiding this comment

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

I think the constructor for EditContactCommand have not been called ?
2021-10-28 (4)

Choose a reason for hiding this comment

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

ya, I agree with Mick. The diagram seems to be too big which causes the words inside the diagram a bit hard to see. You can try changing the maxMessageLength of this diagram. Also you can consider putting new before a Constructor() call. :)


## Instructions for manual testing
## <a name="manual-test"></a>Instructions for manual testing

{Give instructions on how to do a manual product testing e.g., how to load sample data to be used for testing}
Copy link

Choose a reason for hiding this comment

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

Maybe you could provide some instructions here for the manual testing?

Copy link

@jushg jushg left a comment

Choose a reason for hiding this comment

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

Please try to separate larger diagrams (like the EditContactCommand sequence) out if possible to improve readability
2021-10-28 (5)

Copy link

@jushg jushg left a comment

Choose a reason for hiding this comment

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

Can consider to include some more class and object diagrams. Remember to add in the Object Deletion for the Command objects in your sequence diagrams. Overall a very detailed Developer Guide.

docs/DeveloperGuide.md Show resolved Hide resolved

![Edit Sequence Diagram](images/EditContactCommandSequenceDiagram.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 the constructor for EditContactCommand have not been called ?
2021-10-28 (4)

`parseSearchQuery` and the `getDetailFlag` methods respectively. A `SearchContactCommand` with the specified parameters
will be created and executed in `Duke`. The sequence diagram below shows how the whole process is carried out.

![Search Sequence Diagram](images/SearchContactCommandSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

The SearchContactCommand constructor have also not been called here


![Main Parser Sequence Diagram](images/MainParserSequenceDiagram.png)

### <a name="command"></a>Command
Copy link

Choose a reason for hiding this comment

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

For the Command section can consider add in the Class diagram to relate all the commands together

![Main Parser Sequence Diagram](images/MainParserSequenceDiagram.png)

### <a name="command"></a>Command
### <a name="contact-list"></a>ContactList
Copy link

Choose a reason for hiding this comment

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

Please remember to update the "Command" and "ContactList" sections soon.
2021-10-28 (2)

Copy link

@Mick609 Mick609 left a comment

Choose a reason for hiding this comment

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

The code looks clear and understandable. It would be better if descriptions of methods can be provided.

import java.io.IOException;

public class Storage {
private final String contactFilePath;
Copy link

Choose a reason for hiding this comment

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

[marcusbory] Maybe you should use all uppercase for constant.

}

//@@author mayankp291
public static void missingArgEditMessage() {
Copy link

Choose a reason for hiding this comment

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

[mayankp291] Maybe you could name the method name as a verb. For example, printMissingArgEditMessage in this case. Similar naming issues can be seen in other methods.

//allow lowercase email ids
String emailRegex = "^([\\w-\\.]+){1,64}@([\\w&&[^_]]+){2,255}.[a-z]{2,}$";
if (!detailToParse.matches(emailRegex)) {
//LOGGER.log(Level.INFO, "Regex check for Email id failed");
Copy link

Choose a reason for hiding this comment

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

[mayankp291] If you are not using this line of code for the LOGGER anymore, please delete it.

}

//@@author ashrafjfr
public static void helpMessage() {
Copy link

Choose a reason for hiding this comment

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

[ashrafjfr] Maybe you could name this method as an action. For example, printHelpMessage.

System.out.println(LINE);
}

public static void welcomeMessage() {
Copy link

Choose a reason for hiding this comment

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

[lezongmun] Maybe you could name the method as an action.

public static final String BUFFER = " ";

boolean[] hasDeletedDetail(String userInput) throws InvalidFlagException, InvalidDeleteDetailException {
boolean[] hasDeletedDetail = new boolean[7]; //all false by default
Copy link

Choose a reason for hiding this comment

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

[ng-andre] I do not really understand the design of the list of boolean variables for hasDeletedDetail. Could you please specify why the length is 7? Maybe avoid the usage of magic numbers.

docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
Copy link

@t-l-xin t-l-xin left a comment

Choose a reason for hiding this comment

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

Need add more details in some parts

docs/DeveloperGuide.md Outdated Show resolved Hide resolved
![Sysem Architecture Sequence Diagram](images/SystemArchitectureSequence.png)


### <a name="text-ui"></a>TextUi
Copy link

Choose a reason for hiding this comment

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

Text-ui does not seem to be complete, along with Command and ContactList, maybe can try finish up the details soon

docs/DeveloperGuide.md Outdated Show resolved Hide resolved

![Edit Sequence Diagram](images/EditContactCommandSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

diagram too big

same issue: diagram too big, can consider representing the circled part to something simpler

Choose a reason for hiding this comment

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

One way to solve this is by putting reference frame :D

docs/DeveloperGuide.md Outdated Show resolved Hide resolved

## Instructions for manual testing
## <a name="manual-test"></a>Instructions for manual testing

{Give instructions on how to do a manual product testing e.g., how to load sample data to be used for testing}
Copy link

Choose a reason for hiding this comment

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

Maybe can include some manual testing instructions?

`parseSearchQuery` and the `getDetailFlag` methods respectively. A `SearchContactCommand` with the specified parameters
will be created and executed in `Duke`. The sequence diagram below shows how the whole process is carried out.

![Search Sequence Diagram](images/SearchContactCommandSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

diagram too big 1

Diagram also quite big, summarise the diagram more, especially the green part

docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved

![Edit Sequence Diagram](images/EditContactCommandSequenceDiagram.png)

Choose a reason for hiding this comment

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

ya, I agree with Mick. The diagram seems to be too big which causes the words inside the diagram a bit hard to see. You can try changing the maxMessageLength of this diagram. Also you can consider putting new before a Constructor() call. :)


![Edit Sequence Diagram](images/EditContactCommandSequenceDiagram.png)

Choose a reason for hiding this comment

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

One way to solve this is by putting reference frame :D

docs/DeveloperGuide.md Show resolved Hide resolved
ashrafjfr and others added 30 commits November 7, 2021 23:44
Add line break to marcusbory.md
Update to UG theme and ashraf PPP
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.