Skip to content

Exception#44

Merged
LordRekishi merged 22 commits into
mainfrom
exception
Mar 21, 2022
Merged

Exception#44
LordRekishi merged 22 commits into
mainfrom
exception

Conversation

@FelixJacobsen

Copy link
Copy Markdown
Contributor

Fix #11

Includes three custom exceptions. MethodNotAllowed exception needs to be implemented after update methods have been created in all controller and service classes.

@FelixJacobsen FelixJacobsen marked this pull request as draft March 11, 2022 15:46
@helenahalldiniths helenahalldiniths marked this pull request as ready for review March 17, 2022 13:08
@helenahalldiniths

Copy link
Copy Markdown
Contributor

I accedently markt this as ready for review, my bad!

@helenahalldiniths helenahalldiniths marked this pull request as draft March 17, 2022 13:10
@FelixJacobsen FelixJacobsen marked this pull request as ready for review March 18, 2022 14:42
@helenahalldiniths helenahalldiniths self-requested a review March 18, 2022 14:54

@helenahalldiniths helenahalldiniths left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have looked throught the code and it looks good. However, the Victim-class is missing som error handling

@helenahalldiniths

Copy link
Copy Markdown
Contributor

I think this is ok! I will look at the "exceptions" videos when I get a chance and after that I can do a final review!

… to use Object "It is highly recommended not to use wildcard types as return types".
@helenahalldiniths helenahalldiniths self-requested a review March 21, 2022 08:43

@helenahalldiniths helenahalldiniths left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've made some small changes, but overall it looks good.
My only thaught is that maybe we shouldn't throw an exception when "findAll" returns an empty list? If there are no entitys an empty list is a correct response! What do you think?

@helenahalldiniths

Copy link
Copy Markdown
Contributor

Tips on inprovment later on:

In GlobalExceptionHandler:

  • Override some of the methods that already exists (handleHttpMessageNotReadable, handleHttpRequestMethodNotSupported…).

  • Implement a ”handleAll” method that handels all errors that are not handled by any other.

@LordRekishi

Copy link
Copy Markdown
Contributor

Tips on inprovment later on:

In GlobalExceptionHandler:

  • Override some of the methods that already exists (handleHttpMessageNotReadable, handleHttpRequestMethodNotSupported…).
  • Implement a ”handleAll” method that handels all errors that are not handled by any other.

Sounds good. You making some "enhancement" issues and putting them further down the line?

@LordRekishi LordRekishi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just made comments on Address, but it applies to all controllers.

@LordRekishi LordRekishi merged commit a8891a9 into main Mar 21, 2022
@LordRekishi LordRekishi deleted the exception branch March 21, 2022 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Custom Exceptions

3 participants