the dishes are done #10
Conversation
There was a problem hiding this comment.
It has a lot of little issues that can be fixed easy. I also was thinking that we could refactor the controllers to be more descriptive. Along with adding in a new Exception called: InvalidEmailException for the email endpoint. We should also package our exceptions in a exception folder.
| logger.info("Preloading " + repository.save(new Contact("Selina Kyle"))); | ||
| logger.info("Preloading " + repository.save(new Contact("Oswald Cobblepot"))); | ||
| logger.info("Preloading " + repository.save(new Contact("Edward Nygma"))); | ||
| logger.info("Preloading " + repository.save(new Contact("Bruce Wayne", "[email protected]"))); |
There was a problem hiding this comment.
Add this next to your logger object.
private final String TEST_EMAIL_TAG = "@gothamcity.com";
Then you should use this for each new Contact:
logger.info("Preloading " + repository.save(new Contact("Bruce Wayne", "bwayne".concat(TEST_EMAIL_TAG))));
I would add in this to cut down on redundancy. They all look like they are using the same email tag.
| @@ -45,12 +55,13 @@ public boolean equals(Object o) { | |||
| return false; | |||
| Contact contact = (Contact) o; | |||
| return Objects.equals(this.id, contact.id) && | |||
| Objects.equals(this.name, contact.name); | |||
| Objects.equals(this.name, contact.name) && | |||
| Objects.equals(this.name, contact.email); | |||
There was a problem hiding this comment.
Objects.equals(this.email, contact.email);
You missed the email value here.
| @@ -45,12 +55,13 @@ public boolean equals(Object o) { | |||
| return false; | |||
| Contact contact = (Contact) o; | |||
There was a problem hiding this comment.
Why would you want to cast an Object to your Contact while inside the same class? I would of liked to understand why we should be doing it this way.
| return this.email; | ||
| } | ||
|
|
||
| public void setEmail(String email) { |
There was a problem hiding this comment.
public void setEmail(final String email) {
(This is nit-picky) Should build all setters with final tags.
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(this.id, this.name); | ||
| return Objects.hash(this.id, this.name, this.email); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
I can't reach it exactly here.... but your toString() should also look like this..
String.format("Contact {id=%s, name=\"%s\", email=\"%s\"}", this.id, this.name, this.email);
|
|
||
| Optional<List<Contact>> contacts = repository.findByEmailContaining(email); | ||
| if (contacts.get().isEmpty()) { | ||
| throw new RuntimeException(String.format("no contacts with email [%s] found.", email)); |
There was a problem hiding this comment.
throw new RuntimeException(String.format("no contacts with email [%s] found.", email));
See last comment about custom exceptions.
| @@ -44,6 +46,20 @@ | |||
| .orElseThrow(() -> new ContactNotFoundException(name)); | |||
| } | |||
|
|
|||
| @GetMapping("/query") | |||
| public List<Contact> getByEmail(@RequestParam final String email) { | |||
There was a problem hiding this comment.
public List<Contact> getByEmail(@RequestParam @NotBlank final String email) {
Add in the @NotBlank to keep with consistency. Just like how the getByName Mapping is.
If this is done you can remove the Objects.isNull(email) in the if block below.
or we can change it to look like this:
public List<Contact> getByEmail(@ApiParam(value = "Contact's email", required = true, example = "[email protected]") @NotNull(message = "Contact's email can not be null") @PathVariable(name = "email") final String email) {
| @@ -44,6 +46,20 @@ | |||
| .orElseThrow(() -> new ContactNotFoundException(name)); | |||
| } | |||
|
|
|||
| @GetMapping("/query") | |||
There was a problem hiding this comment.
we should add in @ApiResponcses({...}) and @ResponseStatus(HttpStatus.OK) to all of our endpoints that we have so far. Any other Mapping we would want to change it to something else. depending on the Http curl request.
We could also change the names of the endpoints to be more fitting for each also. Maybe something that looks like this? @GetMapping("/query/{email}")
| @@ -14,4 +14,6 @@ | |||
| * @return zero or more Contacts | |||
| */ | |||
| Optional<List<Contact>> findByNameContainingIgnoreCase(final String name); | |||
|
|
|||
There was a problem hiding this comment.
you should add in comments here like there is for the name.
| @@ -44,6 +46,20 @@ | |||
| .orElseThrow(() -> new ContactNotFoundException(name)); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
add in comments like how the other endpoints are
|
Kudos, SonarCloud Quality Gate passed!
|
1 similar comment
|
Kudos, SonarCloud Quality Gate passed!
|
No description provided.