Implemented all items on checklist#3
Implemented all items on checklist#3marksguitars wants to merge 1 commit intoBravoLT:mainfrom marksguitars:main
Conversation
|
I want to start by saying that we are not expecting any additional commits. Feel free to make them if you wish, but that's not an expectation that we have. With that said I will go through your changes and leave technical comments on them. |
| @Column(name = "id") | ||
| private String id; | ||
|
|
||
| public void setFirstName(String firstName) { |
There was a problem hiding this comment.
These getters/setters are not necessary since the Lombok @Data annotation provides them.
| } | ||
|
|
||
| @Column(name = "phone_number", nullable = false) | ||
| private Integer phoneNumber; |
There was a problem hiding this comment.
phoneNumber should be a String not an Integer.
| predicates.add(path.in(values)); | ||
| } | ||
|
|
||
| protected <X, Y extends Collection<X>> void applyInLengthFilter( |
There was a problem hiding this comment.
- This
AbstractSpecificationis only used for searching entities so validation at this layer is not practical. - Although nice use of Generics +1.
| import javax.persistence.criteria.Root; | ||
| import java.util.Set; | ||
|
|
||
| public class UserSpecification extends AbstractSpecification<User> { |
There was a problem hiding this comment.
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.
| private Integer phoneNumber; | ||
| private LocalDateTime updated; | ||
|
|
||
| public void setName(String name) { |
There was a problem hiding this comment.
These getters/setters are not necessary since the Lombok @Data annotation provides them.
| private String lastName; | ||
| private Integer phoneNumber; | ||
|
|
||
| public String getFirstName() { |
There was a problem hiding this comment.
These getters/setters are not necessary since the Lombok @Data annotation provides them.
| private Set<String> middleNames; | ||
| private Set<Integer> phoneNumbers; | ||
|
|
||
| public Set<String> getFirstNames() { |
There was a problem hiding this comment.
These getters/setters are not necessary since the Lombok @Data annotation provides them.
| return dateFilter; | ||
| } | ||
|
|
||
| private final Integer phoneNumberLength = 10; |
There was a problem hiding this comment.
- This value doesn't belong here. It should be declared in a constants or validator file.
- This variable should be
staticso it can be referenced without an instance of the object.
There was a problem hiding this comment.
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.
| isValid = isIntegerLengthValid((Integer) value, (Integer) length); | ||
| } | ||
| return isValid; | ||
| } |
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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).
| } | ||
|
|
||
| if(String.valueOf(instance.getPhoneNumber()).length() > 0 && ValidatorUtil.isInvalid(instance.getPhoneNumber())){ | ||
| errors.reject("'phoneNumber' is must contain only digits"); |
There was a problem hiding this comment.
Weird wording on this error message.
| ('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); |
There was a problem hiding this comment.
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)
Thanks for the review. I did make a couple of comments so hopefully they made sense. |
2 participants
No description provided.