Provicevk/unified responses #843
Conversation
|
SonarCloud Quality Gate failed. |
ghost
requested review from
DmyMi
force-pushed
the
develop
branch
7 times, most recently
from
8fdf2a5 to
f6a4108
Compare
| @@ -0,0 +1,23 @@ | |||
| using Ardalis.SmartEnum; | |||
There was a problem hiding this comment.
(Excessive code) Is this using necessary?
| @@ -0,0 +1,22 @@ | |||
| using Ardalis.SmartEnum; | |||
There was a problem hiding this comment.
(Excessive code) Is this using necessary?
| Message = message; | ||
| } | ||
|
|
||
| public static ApiErrorResponse Create(string groupName, string code, string message) |
There was a problem hiding this comment.
Please explain why instead of public ctor you have private ctor and public static? Statics are worser from testability point of view. As it does the same as ctor - what is the main reason to have it?
| public class InputApiError : ApiError | ||
| { | ||
| public static readonly InputApiError InputDataIncorrect | ||
| = new (nameof(InputDataIncorrect), "Provided data input is incorrect"); |
There was a problem hiding this comment.
(Non-uniqueness) nameof is a cool idea, but it will give you only method/prop name. Thus, in other class (i.e. child of this class named InputAdminApiError) method/prop with same name (InputDataIncorrect) could exist and you'll get eror-code collision if you forget to redefine GroupName. Please use simple enum containing all error codes, or more complex schema ot generate them - at least
$"{nameof(class)}.{nameof(method or property)}"
Or even more complex approach could be used with CallerMemberNameAttribute and it's friends, if you like it
|
|
||
| public sealed class ProviderApiError : ApiError | ||
| { | ||
| public static readonly ProviderApiError ProviderNotCreated |
There was a problem hiding this comment.
See previous comment re nameof and error codes
| { | ||
| } | ||
|
|
||
| public override string GroupName => nameof(InputApiError); |
There was a problem hiding this comment.
(Performance) Do you need this override? Why not set GroupName in ctor?
There was a problem hiding this comment.
(Related to all overiden GroupNames)
|
|
||
| public static class ApiErrorExtensions | ||
| { | ||
| public static ApiErrorResponse CreateApiErrorResponse(this ApiError apiError) |
There was a problem hiding this comment.
(Code style) Consider necessity of this extension class.
Ususally extensions are good for code decoupling. Mapping extensions - when you have 2 classses not related to each other and you don't want introduce any hard dependency between them.
It is not the case for ApiError and ApiErrorResponse, as latter is direct child of former. So why not put this method in ApiErrorResponse class?
There was a problem hiding this comment.
Probably I don't get what problem you are fixing. Please explain why we need lot of inherited error types with no changes in data.
Are there any plans for extending logic or properties in future children?
Maybe it could be simpler? Like 1 class with lot of static properties for each api error code case, or simple enum, list of tuples (if you need that GroupName as separate thing), whatever?
Incoming:
Note that ApiError model should be used for 4xx, 5xx Http Codes.
All Api errors are introduced with a group and code. Therefore we use a dictionary of dictionaries in Frontend.
For future discussions:
BadRequest(ApiError error),NotFound(ApiError error)that takesApiErroras parameter.For thinking:
return Err.WithMessage(ProviderApiError.ProviderNotCreated).WithCode(HttpStatusCode.BadRequest);