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

Working code for Password verification #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ankt84rgb
Copy link

Pull request for the Password verification. Thank you.

Copy link
Collaborator

@raulchedrese raulchedrese left a comment

Choose a reason for hiding this comment

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

Good work 👍

// Response is : Password is OK but atleast one uppercase letter will make it strong
//
// Password is : abcd1ABC
// Response is : OK
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's great that you've identified some input cases to test and manually verified that they work but for larger systems this can get tedious fast. It's well worth your time to learn to use JUnit to write these tests. It will make it much easier to keep running them as you make changes to your code and is a valuable skill when applying for jobs.

boolean isdigit = false;
if(pass==null || pass.trim().isEmpty() || pass.length()<8) {
throw new InvalidPasswordException("Password must contains atleast 8 characters");
}else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a minor thing but you have inconsistency in your spacing. There is a space before the { but no space after }. Some interviewers will feel that this shows a lack of attention to detail. Definitely don't want this to be the reason you lose out on a job opportunity 😀.
You could also look at adding an auto code formatter to your code editor. These have become quite popular recently and make it so you never need to think about code formatting.


public static String verify(String pass) throws InvalidPasswordException {
int count=0;
boolean isupper=false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

For improved readability I'd suggest adopting a variable naming convention. Usually each language has an idiomatic one. Snake Case and Camel Case are two common ones.
Snake Case:
is_upper

Camel Case:
isUpper

}
}

public static String verify(String pass) throws InvalidPasswordException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rename this variable to password. The four characters you save are not worth the potential confusion.

}else {
count++;
}
if(!pass.matches("(.*)[a-z](.*){1,}")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Regular expressions are generally quite hard to read. You could extract this check into a method called containsLowercase. Then this would read:

if (!containsLowercase(password) {

This is much easier for someone unfamiliar with the code to read.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could also move the whole if statement into a method. It might look something like this:

public void validateContainsLowercase(String password) {
    if (!pass.matches("(.*)[a-z](.*){1,}")) {
        throw new InvalidPasswordException("Password must contains atleast one lowercase letter");
    }
}

Then you could rewrite this as:

validateContainsLowercase(password);
count++

}else if(count >= 3 && !isdigit) {
throw new InvalidPasswordException("Password is OK but atleast one numeric character will make it strong");
}
return "OK";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this return value is not used I would remove it altogether. It may cause confusion.

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.

2 participants