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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
.idea/
out/
23 changes: 23 additions & 0 deletions ComIT-Code-Review.iml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?xml version="1.0" encoding="UTF-8"?>
<module type="JAVA_MODULE" version="4">
<component name="NewModuleRootManager" inherit-compiler-output="true">
<exclude-output />
<content url="file://$MODULE_DIR$">
<sourceFolder url="file://$MODULE_DIR$/src" isTestSource="false" />
</content>
<orderEntry type="inheritedJdk" />
<orderEntry type="sourceFolder" forTests="false" />
<orderEntry type="module-library">
<library name="JUnit5.3">
<CLASSES>
<root url="jar://$MAVEN_REPOSITORY$/org/junit/jupiter/junit-jupiter-api/5.5.1/junit-jupiter-api-5.5.1.jar!/" />
<root url="jar://$MAVEN_REPOSITORY$/org/apiguardian/apiguardian-api/1.1.0/apiguardian-api-1.1.0.jar!/" />
<root url="jar://$MAVEN_REPOSITORY$/org/opentest4j/opentest4j/1.2.0/opentest4j-1.2.0.jar!/" />
<root url="jar://$MAVEN_REPOSITORY$/org/junit/platform/junit-platform-commons/1.5.1/junit-platform-commons-1.5.1.jar!/" />
</CLASSES>
<JAVADOC />
<SOURCES />
</library>
</orderEntry>
</component>
</module>
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# ComIT-Code-Review

TDD Kata 3
Create a Password verifications class called “PasswordVerifier”.
Create a Password verifications class called “main.PasswordVerifier”.
Add the following verifications to a master function called “Verify()”

- password should be larger than 8 chars
Expand Down
110 changes: 110 additions & 0 deletions src/main/PasswordVerifier.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
package main;

import java.util.regex.Pattern;

public class PasswordVerifier {

public PasswordVerifier() {

}

public boolean verify(String password) {
int numValid = 0;

// password is never OK if item 1.4 (password should have one lowercase letter at least) is not true.
try {
if (containsLowercase(password)) numValid++;
} catch (IllegalArgumentException e) {
System.out.println(e);
return false;
}

try {
if (isNotNull(password)) numValid++;
} catch (IllegalArgumentException e) {
System.out.println(e);
}

try {
if (isLargerThanEight(password)) numValid++;
} catch (IllegalArgumentException e) {
System.out.println(e);
}

if (numValid >= 3) return true; // for optimization

try {
if (containsUppercase(password)) numValid ++;
} catch (IllegalArgumentException e) {
System.out.println(e);
}

if (numValid >= 3) return true; // for optimization

try {
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);
}


return numValid >= 3;

/*
Optimization:
- The containsLowercase() function is executed first.
If a password doesn't contain lowercase, we can return false and skip the other validations.

- Returns true whenever the value of numValid is three.
The if statements are added after the function#3 and #4 are executed.

*/
}

public boolean isLargerThanEight(String password) throws IllegalArgumentException {
Boolean isValidPassword = (password != null) && (password.length() > 8);

if (!isValidPassword)
throw new IllegalArgumentException("Password length has to be greater than 8.");

return true;
}

public boolean isNotNull(String password) throws IllegalArgumentException {
Boolean isValidPassword = (password != null);

if (!isValidPassword)
throw new IllegalArgumentException("Password cannot be blank.");

return true;
}

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.");

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.");
}

}

public boolean containsLowercase(String password) throws IllegalArgumentException {
Pattern lowercasePattern = Pattern.compile("^(?=.*[a-z]).+$");
Boolean isValidPassword = (password != null) && lowercasePattern.matcher(password).matches();

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

return true;
}

public boolean containsDigit(String password) throws IllegalArgumentException {
Pattern digitPattern = Pattern.compile("^(?=.*\\d).+$");
Boolean isValidPassword = (password != null) && digitPattern.matcher(password).matches();

if (!isValidPassword)
throw new IllegalArgumentException("Password should contain at least one number.");

return true;
}
}
107 changes: 107 additions & 0 deletions src/test/PasswordVerifierTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
package test;

import main.PasswordVerifier;
import org.junit.jupiter.api.Test;

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.

private PasswordVerifier tester = new PasswordVerifier();

@Test
void verify() {
assertEquals(true, tester.verify("7ShiftComIT")); // not null, upper, lower, digit, >8
assertEquals(true, tester.verify("7Shift")); // not null, upper, lower, digit
assertEquals(true, tester.verify("Shift")); // not null, upper, lower
assertEquals(false, tester.verify("SHIFT")); // not null, upper
assertEquals(false, tester.verify("7SHIFTCOMIT")); // not null, upper, digit, >8, ** no lowercase **
assertEquals(false, tester.verify(null));
}

@Test
void isLargerThanEight() {
assertEquals(true,tester.isLargerThanEight("abcd56789"));
assertEquals(true,tester.isLargerThanEight("1234567890"));
}

@Test
void isLargerThanEightShouldThrowIllegalArgumentException() {
assertThrows(IllegalArgumentException.class,
() -> {
tester.isLargerThanEight("abc");
tester.isLargerThanEight("abcd5678");
tester.isLargerThanEight(null);
});
}

@Test
void isNotNull() {
assertEquals(true, tester.isNotNull("abc"));
assertEquals(true, tester.isNotNull(""));
assertEquals(true, tester.isNotNull("null"));
}

@Test
void isNotNullShouldThrowIllegalArgumentException() {
assertThrows(IllegalArgumentException.class,
() -> {
tester.isNotNull(null);
});
}

@Test
void containsUppercase() {
assertEquals(true, tester.containsUppercase("7Shift"));
assertEquals(true, tester.containsUppercase("7shifT"));
assertEquals(true, tester.containsUppercase("Shift7"));
}

@Test
void containsUppercaseShouldThrowIllegalArgumentException() {
assertThrows(IllegalArgumentException.class,
() -> {
tester.containsUppercase("7shift");
tester.containsUppercase("7777777");
tester.containsUppercase("");
tester.isLargerThanEight(null);
});
}

@Test
void containsLowercase() {
assertEquals(true, tester.containsLowercase("7Shift"));
assertEquals(true, tester.containsLowercase("7shifT"));
assertEquals(true, tester.containsLowercase("Shift7"));
}

@Test
void containsLowercaseShouldThrowIllegalArgumentException() {
assertThrows(IllegalArgumentException.class,
() -> {
tester.containsLowercase("7SHIFT");
tester.containsLowercase("7777777");
tester.containsLowercase("");
tester.isLargerThanEight(null);
});
}

@Test
void containsDigit() {
assertEquals(true, tester.containsDigit("7Shift"));
assertEquals(true, tester.containsDigit("7shifT"));
assertEquals(true, tester.containsDigit("Shift7"));
assertEquals(true, tester.containsDigit("7777777"));
}

@Test
void containsDigitShouldThrowIllegalArgumentException() {
assertThrows(IllegalArgumentException.class,
() -> {
tester.containsDigit("SevenShift");
tester.containsDigit("");
tester.isLargerThanEight(null);
});

}
}