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][T11-2]Li Guanlong #40

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

[W7][T11-2]Li Guanlong #40

wants to merge 13 commits into from

Conversation

liguanlong
Copy link

Added a new command: login, which is used for the user to login to the system before actually using the address book.

@matthiaslum
Copy link

This is a useful feature to prevent unwanted people from using the addressbook

Copy link

@stephlewyh stephlewyh left a comment

Choose a reason for hiding this comment

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

Hi Guan Long,

For 3kLoc, you are only required to submit one enhancement, not your team code.
I'm not able to assess which particular enhancement you have done from this submission.

You may close the PR after reviewing the comment.

Copy link

@stephlewyh stephlewyh left a comment

Choose a reason for hiding this comment

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

Hi Guan Long,

Good work here in the login enhancement, updating the documentation and in providing the unit tests for the new enhancement. One thing to note: Header Comments are important, for code readability; you have missed that out in several of the new non-trivial methods.

See coding standard conventions for more information on what is required: https://nuscs2113-ay1819s2.github.io/website/coding-standards/java/intermediate.html

public boolean getLoginStatus(){
return loginStatus;
}

Choose a reason for hiding this comment

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

Missing header comment. All non-trivial methods should have java doc format header comments.

public String checkLoginInfo(String userCommandText){
String[] accountInfo = userCommandText.split(" ");
try {
if (accountInfo.length == 3 && accountInfo[0].equals("login")) {

Choose a reason for hiding this comment

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

You could apply some refactoring principles here. Multi-condition criteria could be extracted into booleans.

private static final String Invalid_FORMAT = "Please input the correct login command: login username password";
private Login login = new Login();

@Test

Choose a reason for hiding this comment

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

Missing header comment. All non-trivial methods should have java doc format header comments.

assertEquals(login.getLoginStatus(), true);
}

@Test

Choose a reason for hiding this comment

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

Missing header comment. All non-trivial methods should have java doc format header comments.

assertEquals(login.getLoginStatus(), false);
}

@Test

Choose a reason for hiding this comment

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

Missing header comment. All non-trivial methods should have java doc format header comments.

final String input1 = "login guanlong 12345";
final String input2 = "some_random_string";
final String input3 = "login wrong_username wrong_password";
assertEquals(login.checkLoginInfo(input1), SUCCESS);

Choose a reason for hiding this comment

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

Good job here, providing the positive and negative test cases.

yingrong1996 pushed a commit to yingrong1996/addressbook-level3 that referenced this pull request Mar 19, 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.

4 participants