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

Scooter: Field validation + Exception handling + DTOs + delete a scooter by id. #28

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

Conversation

dbernatt
Copy link
Collaborator

@dbernatt dbernatt commented Dec 3, 2021

Tasks:

  • Field validation
  • Exception handling
  • DTOs
  • Delete scooter by id

Bernat and others added 30 commits November 9, 2021 23:03
…oter function + rebase cleanup

added the delete scooter function

added validation to fields and created dtos for the scooter model
@dbernatt
Copy link
Collaborator Author

dbernatt commented Dec 3, 2021

#20 (comment)
If the spacing does not increase readability, it is redundant.
.
Fixed.

@dbernatt
Copy link
Collaborator Author

dbernatt commented Dec 3, 2021

#20 (comment)

What is the advantage of using com.google.common.base.Objects.equal vs java.util.Objects.equals or is this just a wrong import?

I don't know the advantage. It was just a wrong import. :) Fixed.

@dbernatt
Copy link
Collaborator Author

dbernatt commented Dec 3, 2021

#20 (review)

Should case sensitivity be enforced?

No. I changed to ignore-case.

@dbernatt
Copy link
Collaborator Author

dbernatt commented Dec 3, 2021

#20 (review)

Is the logger used?

Yes. If an Exception occure we log the stackTrace. (Exception.class)

@dbernatt
Copy link
Collaborator Author

dbernatt commented Dec 3, 2021

#20 (review)

If documentation does not add value, it is redundant. You could instead document the purpose of the class itself.

I think the class name is descriptive enough. Removed these comments from the class.

@dbernatt
Copy link
Collaborator Author

dbernatt commented Dec 3, 2021

#20 (review)

If you look into NoHandlerFoundException, you will see that it is thrown if throwExceptionIfNoHandlerFound property is set to true, otherwise the default app behaviour is to return 404 status code.
Does wrapping this into a BadRequest status add any value?

Ups. I don't think so. I change it to 404 then.

@dbernatt
Copy link
Collaborator Author

dbernatt commented Dec 3, 2021

#20 (review)

Considering http status code is already returned, does this add value?
You could stick with a simpler version for exception handling.

Its helpful because on the client side the status code value as number can be used directly without any parsing and checking.

@dbernatt
Copy link
Collaborator Author

dbernatt commented Dec 3, 2021

#20 (review)

Could you highlight the way to reach an IllegalArgumentException here?

No. I think I did this accidentally. :). Fixed.

@dbernatt
Copy link
Collaborator Author

dbernatt commented Dec 3, 2021

#20 (review)

Notes regarding the use of DTOs, which should be discussed with the team

  • when using DTOs, they should be used both for input & output (since the idea is to provide another model, potentially simplified, to the client/user)
  • discuss with colleagues about an approach for validation, since if the only reason for having DTOs is for validation (and are as result a wrapper over the model class), perhaps sth else might be more suitable

I use DTOs for validation and data hiding so the user can't send the a Scooter with a battery status. The other reason is the use of reflection which automatically parses the model with the default constructor and I can't specify which constructor is called in that phase. So If I don't use dto for the scooter then I can't limit the user to send a scooter request(POST) without a battery status.

I am open for any other suggestions. But this is the only solution that I see working right know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add validation + DTOs + exception handling to the Scooter Controller
3 participants