EzequielVilla / m9-backend

m9-desafio.vercel.app
0 stars 0 forks source link

Feedback #1

Open zapaiamarce opened 2 years ago

zapaiamarce commented 2 years ago

Eze: En lineas generales está de lujo! Bien organizados los endpoints y sus schemas. Bien la cantidad de lógica dentro de estos endpoints, quizás se puede seguir pasando cosas a los controllers para que los endpoints tengan menos y menos lógica pero esto con la práctica y también con el contacto con otros proyectos cuando empieces a trabajar, vas a ir viendo en que ocasiones te conviene invertir tiempo en hacer algo bien abstracto y separado del endpoint y cuando te conviene dejar todo ahi nomás, prolijo pero ahi nomás, para escalarlo después.

Con respecto a los controllers, están super pero como te decía antes creo que pueden hacer cosas más grandes todavía. Está genial que hagas funciones más chicas que hagan cosas concretas, pero siempre podés crear controllers que resuelvan problemas más grandes como el de tu endpoint del IPN donde usas dos controllers:

https://github.com/EzequielVilla/m9-backend/blob/400d9b68ffac24807d8a3c3c0f3cc42a4a21d715/pages/api/ipn/mercadopago.ts#L18

Esto está super pero eventualmente te vas a dar cuenta que esas dos acciones:

const order = await getOrderStatus(id,topic);
const orderId = await checkOrderStatusAndProcess(order);

pueden estar detras de un solo controller asi el endpoint no sabe nada de que lógica hay acá y solo recibe una respuesta concreta para responderle al cliente. De hecho cuando tus controllers sean más grandes estas funciones más chicas que usas hoy como controllers van a pasar a tu capa lib (o capa utils) donde viven funciones para hacer cosas de este estilo, bien cerca de Mercado Pago en este caso.

Tus modelos están impecables, obviamente hay librerías que resuelven esto y bueno, podés agregarle un millon de cosas a estas clases pero lo importante acá es que te armaste tu propia cada de abstracción para interactuar con la DB y esto te da la oportuidad de cambiar de base de datos si querés y usar las mismas clases con distintos "enchufes".

Te felicito!!!


Te dejo algunas cosas que podrías revisar, son cosas mínimas. Ojota con declarar cosas globales como acá: https://github.com/EzequielVilla/m9-backend/blob/main/interfaces.d.ts Está bueno que los tipos estén en un lugar predecible, como el upgradeData que debería estar al lado de la función que lo user y si necesita ser usado por otro mod que se el otro mod haga un import de ese type o interface. Los globales los dejaría solo para cosas super globales/genéricas que no tengan que ver con tu negocio como:

type coords = {
  lat: string;
  lon: string;
};

/auth Guarda que tu endpoint de Auth devuelve el código, justo este endpoint no tiene que devolver el código si no entra cualquiera. Pero bueno, para el ejemplo está bien.

En tu collection de postman podés definir la variable del token sin el "bearer " y después podés configurar este token para toda tu collection así: image

Estaría bueno que el POST de /order te devuelva el id del order creado, asi lo podemos chequear en el get order/id

EzequielVilla commented 2 years ago

Marce! Muchas gracias por la review como siempre.

-Despues de leerlo y re leerlo entendi lo que querias decir con respecto al endpoint ipn -Hubo un momento en que me hice un matete terrible con los controllers y senti que algo estaba errando. En un momento estaba haciendo dentro la conexion directa con la db y ahi fue cuando lo pase a los models, capaz quedo algo raro o medio indefinido. -Y respecto a la interface global, soy un cabezon, ya me lo habias dicho en el modulo anterior jajaja. -Ups, ahora arreglo lo de auth!

Y gracias por la data de postman, tengo que meterme un poquito mas con esa herramienta.

EzequielVilla commented 2 years ago

Ahora que lo recuerdo. En un momento tuve la duda de que podia ir en lib y que en controller. Creo que el caso mas concreto es el modulo orders de la carpeta lib. Ahi me parecio que algo no estaba bien.