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

Kitiya: TDD Kata 3 - Password Verifier #2

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

Conversation

kitiya
Copy link

@kitiya kitiya commented Sep 4, 2019

Thank you very much for the presentation on Friday and providing us this code challenge. I've learned some new topics by working on this exercise. Thanks in advance for your feedback and please let me know if you have any questions.

@kitiya kitiya changed the title Kitiya: TDD Kata 3 - PasswordVerifier Kitiya: TDD Kata 3 - Password Verifier Sep 4, 2019
change if statement from (numValid == 3) to (numValid >= 3)
@kitiya
Copy link
Author

kitiya commented Sep 4, 2019

To be safe, I changed the if statement to return true when (numValid >=3)

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.

Nice work 👍 I'd encourage you to take a look at some of the other submissions and the comments as well.

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

class PasswordVerifierTest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice tests. I this case I would consider only testing the verify method. You could test all these things through that method and then you could make the specific checks private.

if (!isValidPassword)
throw new IllegalArgumentException("Password should have at least one uppercase letter.");

return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not convinced that you should be returning anything in these methods. I find it confusing. These methods will either throw an exception or return true so if no exception is thrown we can assume all went well instead of having to check for return values and exceptions.

public boolean containsUppercase(String password) throws IllegalArgumentException {
    Pattern uppercasePattern = Pattern.compile("^(?=.*[A-Z]).+$");
    Boolean isValidPassword = (password != null) && uppercasePattern.matcher(password).matches();

    if (!isValidPassword)
        throw new IllegalArgumentException("Password should have at least one uppercase letter.");
}

if (containsDigit(password)) numValid ++;
} catch (IllegalArgumentException e) {
System.out.println(e);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

All these try catch statements do the same thing. Can we simplify to something like:

try {
    if (!containsLowercase(password)) return false
    numValid++
    if (isNotNull(password)) numValid++;
    if (isLargerThanEight(password)) numValid++;
    if (numValid >= 3) return true; // for optimization
    if (containsUppercase(password)) numValid ++;
    if (numValid >= 3) return true; // for optimization
    if (containsDigit(password)) numValid ++;
} catch (IllegalArgumentException e) {
    System.out.println(e);
}

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