Skip to content

Implemented all items on checklist #3

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.

Sign up for GitHub

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

Closed
marksguitars wants to merge 1 commit into from
Closed

Implemented all items on checklist #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
49 changes: 49 additions & 0 deletions src/main/java/com/bravo/user/dao/model/User.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,26 @@ public class User {
@Column(name = "id")
private String id;

public void setFirstName(String firstName) {
Copy link
Member

@wheeleruniverse wheeleruniverse Aug 3, 2021

Choose a reason for hiding this comment

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

These getters/setters are not necessary since the Lombok @Data annotation provides them.

this.firstName = firstName;
}

public void setMiddleName(String middleName) {
this.middleName = middleName;
}

public void setLastName(String lastName) {
this.lastName = lastName;
}

public void setPhoneNumber(Integer phoneNumber) {
this.phoneNumber = phoneNumber;
}

public void setUpdated(LocalDateTime updated) {
this.updated = updated;
}

@Column(name = "first_name", nullable = false)
private String firstName;

Expand All @@ -27,9 +47,37 @@ public class User {
@Column(name = "last_name", nullable = false)
private String lastName;

public String getId() {
return id;
}

public String getFirstName() {
return firstName;
}

public String getMiddleName() {
return middleName;
}

public String getLastName() {
return lastName;
}

public Integer getPhoneNumber() {
return phoneNumber;
}

public LocalDateTime getUpdated() {
return updated;
}

@Column(name = "phone_number", nullable = false)
private Integer phoneNumber;
Copy link
Member

@wheeleruniverse wheeleruniverse Aug 3, 2021

Choose a reason for hiding this comment

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

phoneNumber should be a String not an Integer.


@Column(name = "updated", nullable = false)
private LocalDateTime updated;


public User(){
super();
this.id = UUID.randomUUID().toString();
Expand All @@ -41,5 +89,6 @@ public User(final UserSaveDto user){
this.firstName = user.getFirstName();
this.middleName = user.getMiddleName();
this.lastName = user.getLastName();
this.phoneNumber = user.getPhoneNumber();
}
}
5 changes: 5 additions & 0 deletions src/main/java/com/bravo/user/dao/model/mapper/ResourceMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import com.bravo.user.dao.model.User;
import com.bravo.user.model.dto.UserReadDto;

import java.time.LocalDateTime;
import java.util.Collection;
import java.util.List;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -31,6 +33,9 @@ public UserReadDto convertUser(final User user){
name = String.format("%s %s", user.getFirstName(), user.getLastName());
}
dto.setName(name);
dto.setPhoneNumber(user.getPhoneNumber());
dto.setUpdated(LocalDateTime.now());

return dto;
}
}
12 changes: 12 additions & 0 deletions src/main/java/com/bravo/user/dao/specification/AbstractSpecification.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,17 @@ protected <X, Y extends Collection<X>> void applyInFilter(
predicates.add(path.in(values));
}

protected <X, Y extends Collection<X>> void applyInLengthFilter(
Copy link
Member

@wheeleruniverse wheeleruniverse Aug 3, 2021

Choose a reason for hiding this comment

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

  1. This AbstractSpecification is only used for searching entities so validation at this layer is not practical.
  2. Although nice use of Generics +1.

final Expression<X> path,
final Y values,
final Integer length
){
if(ValidatorUtil.isInvalidLength(values, length)){
return;
}
predicates.add(path.in(values));
}

protected <X extends Collection<String>> void applyStringFilter(
final Expression<String> path,
final X values
Expand Down Expand Up @@ -103,4 +114,5 @@ protected <X extends Collection<String>> void applyStringFilter(
applyInFilter(targetPath, inClause);
}
}

}
11 changes: 11 additions & 0 deletions src/main/java/com/bravo/user/dao/specification/UserSpecification.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
import com.bravo.user.model.filter.UserFilter;
import javax.persistence.criteria.CriteriaBuilder;
import javax.persistence.criteria.CriteriaQuery;
import javax.persistence.criteria.Path;
import javax.persistence.criteria.Root;
import java.util.Set;

public class UserSpecification extends AbstractSpecification<User> {
Copy link
Member

@wheeleruniverse wheeleruniverse Aug 3, 2021

Choose a reason for hiding this comment

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

Similar to AbstractSpecification comment. This class is only used for searching and invoked from the service layer. The validation should happen before the code reaches this point.


Expand All @@ -27,5 +29,14 @@ public void doFilter(
applyStringFilter(root.get("firstName"), filter.getFirstNames());
applyStringFilter(root.get("lastName"), filter.getLastNames());
applyStringFilter(root.get("middleName"), filter.getMiddleNames());

// Validates whether phone number contains only digits
applyInFilter(root.get("phoneNumber"), filter.getPhoneNumbers());

// Validates whether phone number complies with correct length
applyInLengthFilter(root.get("phoneNumber"), filter.getPhoneNumbers(), filter.getPhoneNumberLength());

}


}
14 changes: 14 additions & 0 deletions src/main/java/com/bravo/user/model/dto/UserReadDto.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,19 @@ public class UserReadDto {

private String id;
private String name;
private Integer phoneNumber;
private LocalDateTime updated;

public void setName(String name) {
Copy link
Member

@wheeleruniverse wheeleruniverse Aug 3, 2021

Choose a reason for hiding this comment

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

These getters/setters are not necessary since the Lombok @Data annotation provides them.

this.name = name;
}

public void setPhoneNumber(Integer phoneNumber) {
this.phoneNumber = phoneNumber;
}

public void setUpdated(LocalDateTime updated) {
this.updated = updated;
}

}
19 changes: 19 additions & 0 deletions src/main/java/com/bravo/user/model/dto/UserSaveDto.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,23 @@ public class UserSaveDto {
private String firstName;
private String middleName;
private String lastName;
private Integer phoneNumber;

public String getFirstName() {
Copy link
Member

@wheeleruniverse wheeleruniverse Aug 3, 2021

Choose a reason for hiding this comment

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

These getters/setters are not necessary since the Lombok @Data annotation provides them.

return firstName;
}

public String getMiddleName() {
return middleName;
}

public String getLastName() {
return lastName;
}

public Integer getPhoneNumber() {
return phoneNumber;
}


}
31 changes: 31 additions & 0 deletions src/main/java/com/bravo/user/model/filter/UserFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,40 @@
@Data
public class UserFilter {

public Set<String> getIds() {
return ids;
}

private Set<String> ids;
private Set<String> firstNames;
private Set<String> lastNames;
private Set<String> middleNames;
private Set<Integer> phoneNumbers;

public Set<String> getFirstNames() {
Copy link
Member

@wheeleruniverse wheeleruniverse Aug 3, 2021

Choose a reason for hiding this comment

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

These getters/setters are not necessary since the Lombok @Data annotation provides them.

return firstNames;
}

public Set<String> getLastNames() {
return lastNames;
}

public Set<String> getMiddleNames() {
return middleNames;
}

public Set<Integer> getPhoneNumbers() {
return phoneNumbers;
}

public Integer getPhoneNumberLength() {
return phoneNumberLength;
}

public DateFilter<LocalDateTime> getDateFilter() {
return dateFilter;
}

private final Integer phoneNumberLength = 10;
Copy link
Member

@wheeleruniverse wheeleruniverse Aug 3, 2021

Choose a reason for hiding this comment

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

  1. This value doesn't belong here. It should be declared in a constants or validator file.
  2. This variable should be static so it can be referenced without an instance of the object.

Copy link
Author

@marksguitars marksguitars Aug 10, 2021

Choose a reason for hiding this comment

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

I agree and I made a comment to your point. I just did not have time to either add a validator or be familiar enough w/your code to know the proper place to put this. static of course should have been added.

private DateFilter<LocalDateTime> dateFilter;
}
3 changes: 3 additions & 0 deletions src/main/java/com/bravo/user/service/UserService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ public UserReadDto update(final String id, final UserSaveDto request){
if(ValidatorUtil.isValid(request.getLastName())){
user.setFirstName(request.getLastName());
}
if(ValidatorUtil.isValid(request.getPhoneNumber())){
user.setPhoneNumber(request.getPhoneNumber());
}

final User updated = userRepository.save(user);

Expand Down
27 changes: 27 additions & 0 deletions src/main/java/com/bravo/user/utility/ValidatorUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,27 @@ else if(value instanceof DateFilter<?>){
else if(value instanceof String){
isValid = isStringValid((String)value);
}
else if(value instanceof Integer) {
isValid = isIntegerValid((Integer)value);
}
else {
isValid = value != null;
}
return isValid;
}

public static <T> boolean isInvalidLength(T value, T length){
return !isValidLength(value, length);
}

public static <T> boolean isValidLength(T value, T length){
boolean isValid = false;
if(value instanceof Integer){
isValid = isIntegerLengthValid((Integer) value, (Integer) length);
}
return isValid;
}
Copy link
Member

@wheeleruniverse wheeleruniverse Aug 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think the implementation of this method is very effective. It would be better if it took a min and max range and then compared the value for numbers and the length for strings.

 public static <T> boolean isValidLength(T value, double min, double max){
      double numeric;
      if(value instanceof Number){
           numeric = ((Number)value).doubleValue();          
      }
      else if(value instanceof String){
           numeric = (double)((String)value).length;
      }
      else {
           throw new IllegalArgumentException("helpful message");
      }
      return numeric > min && numeric < max;
 }

Then if you really wanted you can overload a version that assumes 0 as the min value.

 public static <T> boolean isValidLength(T value, double max){
      return isValidLength(value, 0.0, max);
 }

Copy link
Author

@marksguitars marksguitars Aug 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking in a purely linear way here in regards to testing only Integer values. With that in mind, I am not sure if using a range where I would have to set both a min and max arg to, say values which only differ by two in order to test the ceiling of an Integer value would be more effective than my isValidLength function which when all told should handle checks on only Integers in less code than your re-factored example(unless your point was to emphasize additional data types being covered as well as the extensibility of testing for both a range on an Integer as well as a single value).



private static boolean isCollectionValid(final Collection<?> collection){
return !collection.isEmpty() && !collection.stream()
Expand All @@ -62,4 +77,16 @@ private static boolean isDateFilterValid(final DateFilter<?> date){
private static boolean isStringValid(final String string){
return string != null && !string.trim().isEmpty();
}

private static boolean isIntegerValid(final Integer integer) {
if(integer == null || integer == 0)
return false;
String onlyDigits = "\\d+";
String inString = integer.toString();
return inString.matches(onlyDigits);
}

private static boolean isIntegerLengthValid(final Integer integer, final Integer length) {
return String.valueOf(integer).length() == length;
}
}
17 changes: 17 additions & 0 deletions src/main/java/com/bravo/user/validator/UserValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import com.bravo.user.utility.ValidatorUtil;
import org.springframework.stereotype.Component;
import org.springframework.validation.Errors;
import com.bravo.user.model.filter.UserFilter;

@Component
public class UserValidator extends CrudValidator {
Expand All @@ -20,13 +21,25 @@ protected void validateCreate(Object o, Errors errors) {

UserSaveDto instance = (UserSaveDto) o;

/* Admittedly, this is a bit of a hack storing the phone number length in
* the UserFilter class and having to new up an instance here :(
*/
UserFilter userFilter = new UserFilter();

// required fields
if(ValidatorUtil.isInvalid(instance.getFirstName())){
errors.reject("'firstName' is required");
}
if(ValidatorUtil.isInvalid(instance.getLastName())){
errors.reject("'lastName' is required");
}
if(ValidatorUtil.isInvalid(instance.getPhoneNumber())){
errors.reject("'phoneNumber' is required and must contain only digits");
}
if(ValidatorUtil.isInvalidLength(instance.getPhoneNumber(), userFilter.getPhoneNumberLength())){
errors.reject("'phoneNumber' must be 10 digits in length");
}

}

@Override
Expand All @@ -37,5 +50,9 @@ protected void validateUpdate(Object o, Errors errors) {
if(ValidatorUtil.isEmpty(instance, "id", "updated")){
errors.reject("'request' modifiable field(s) are required");
}

if(String.valueOf(instance.getPhoneNumber()).length() > 0 && ValidatorUtil.isInvalid(instance.getPhoneNumber())){
errors.reject("'phoneNumber' is must contain only digits");
Copy link
Member

@wheeleruniverse wheeleruniverse Aug 3, 2021

Choose a reason for hiding this comment

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

Weird wording on this error message.

}
}
}
11 changes: 6 additions & 5 deletions src/main/resources/data.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@ create table user (
first_name varchar(100) not null,
middle_name varchar(100) null,
last_name varchar(100) not null,
phone_number integer not null,
updated timestamp not null default current_timestamp()
);

insert into user (id, first_name, middle_name, last_name) values
('097e1c2e-8ba3-463e-96d2-7a08f32944a6','Justin','Marquis','Wheeler'),
('b1f6c877-c4a2-4e00-a986-2cbed56e257f','James',null,'Wagon'),
('d7d77a1b-d0e4-49bb-804d-895db58027cb','John','Smith','Doe'),
('5f99aae9-7426-4e5b-932a-4f7066785e63','Jack',null,'Pool'),
('d54cbbb5-96f7-496e-8142-ce4966061940','Joan','Marie','River');
('097e1c2e-8ba3-463e-96d2-7a08f32944a6','Justin','Marquis','Wheeler', 6161112222),
('b1f6c877-c4a2-4e00-a986-2cbed56e257f','James',null,'Wagon', 6162223333),
('d7d77a1b-d0e4-49bb-804d-895db58027cb','John','Smith','Doe', 6163334444),
('5f99aae9-7426-4e5b-932a-4f7066785e63','Jack',null,'Pool', 6164445555),
('d54cbbb5-96f7-496e-8142-ce4966061940','Joan','Marie','River', 6165556666);
Copy link
Member

@wheeleruniverse wheeleruniverse Aug 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

You have a bug in this h2 seed file that is preventing the app from running with the following exception.

Caused by: org.h2.jdbc.JdbcSQLSyntaxErrorException: Column count does not match; SQL statement...

You should've updated the insert into statement to include the new column.

insert into user (id, first_name, middle_name, last_name, phone_number)