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

ProviderController, Service, DTO #134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SebaMergip
Copy link

Description

1. Cree el ProviderController para manejar los request de cada ruta del provider.
2. Cree el ProviderService para realizar la logica de negocio de cada CRUD solicitado.
3. Cree los DTO para transportar los datos del servidor al cliente y viceverza. (ProviderDTO y ProviderSearchDTO)
4. Luego, descomenté las lineas del Startup.cs para habilitar el servicio ProviderService para que pueda ser llamado por la app.
5. Pruebas:

a. Probé los endpoint desde Swagger para corroborar su buen comportamiento
b. Luego, probé el flujo de la app completa levantando el cliente en mi localhost
c. Finalmente, corrí los UNIT TEST dando el siguiente resultado que se muestra en la foto:

WhatsApp Image 2021-11-17 at 20 38 16

Opinion

Muy buen ejercicio. Me pareció bastante simple y la documentación estaba clara y concisa.

@gerardoaquino25
Copy link
Collaborator

Buenas! Te dejo algunas preguntas y sugerencias:

  • Manejo de respuesta: ante errores controlados en algunos maneja un mensaje de respuesta 200 con un mensaje, en otros se arroja un llano 500 sin loguear.
    Cual te parece la forma más adecuada en general? Sugerencia
  • "this": a veces se usa y otra no. Ves alguna forma de reducir los ya existentes con algún criterio? Pista.

Dejo también algunos comentarios en las líneas de código especificas.
Para todos las sugerencias, usa el código ya existente como guía pero no te limites a pensar que no se puede mejorar!
Saludos!

/// </summary>
/// <param name="name">Provider name to check.</param>
/// <returns></returns>
private bool NombreUnico(string name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Esta funcion es identica a la que esta en la clase StoreService. Se te ocurre alguna forma de refactorizar el codigo para evitar duplicacion?

/// <param name="id">Provider id.</param>
/// <param name="value">Provider information.</param>
[HttpPut("{id}")]
public void Put(string id, [FromBody] ProviderDTO value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Que pasa si el parámetro id es distinto al del id contenido dentro del parámetro value?

}
catch (Exception ex)
{
logger.LogCritical(ex.StackTrace);
Copy link
Collaborator

Choose a reason for hiding this comment

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

¿Por qué se usa critical? ¿Por qué el mensaje de la respuesta es que ya existe si esta en un catch general?

/// <param name="value">Provider info.</param>
/// <returns></returns>
[HttpPost]
public ActionResult Post([FromBody] ProviderDTO value)
Copy link
Collaborator

@gerardoaquino25 gerardoaquino25 Nov 18, 2021

Choose a reason for hiding this comment

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

Cual es la diferencia entre usar como respuesta un ActionResult, ActionResult<ProviderDTO> y ProviderDTO? Cual te parece más adecuado?

@SebaMergip
Copy link
Author

@gerardoaquino25 en el transcurso del día lo reviso en detalle, lo analizo y te contesto todo. Gracias por tu feedback!! . También intento de corregir los cambios en el codigo y lo vuelvo a subir.

Saludos!

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.

2 participants