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

[W7][M11-2]Lee Wenhao Nicholas #39

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

Conversation

nicholasleeeee
Copy link

Added , , feature
Updated User Guide

Copy link

@devamanyu devamanyu left a comment

Choose a reason for hiding this comment

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

This PR adds three command features: Signup, Login, and Logout to AB3. Accompanied is a hash function to hash the password (Good job).

I couldn't find any logic component associated to the PR, i.e., how is the login supposed to happen. (Did I miss it anywhere, kindly point out if thats the case)

Always add test cases whenever new features are added.
Your commit message format seems to be incorrect. Checkout the correct way to write commit messages.

Please close this PR after you read the comments.

@@ -8,6 +8,8 @@
import seedu.addressbook.ui.Gui;
import seedu.addressbook.ui.Stoppable;



Choose a reason for hiding this comment

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

Minor comment - Avoid unnecessary whitespaces.


Boolean isAuthenticated = false;

String testPassword = TEST + password;

Choose a reason for hiding this comment

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

Why is the TEST prefix used? If any particular use, mention it as a comment.

isAuthenticated = true;
new CommandResult(MESSAGE_SUCCESS);
} else {
isAuthenticated = false;

Choose a reason for hiding this comment

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

Has the use of isAuthenticated implemented? I can't seem to find it.

import java.security.NoSuchAlgorithmException;
import java.lang.String;

public class generateHash {

Choose a reason for hiding this comment

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

Add comments explaining the hashing process. An important part of SE is to aid future developers easily understand your code.

yingrong1996 added a commit to yingrong1996/addressbook-level3 that referenced this pull request Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants