Open ggomma opened 5 months ago
아래 코드는 routes 가 아닌 다른 곳으로 옮겨보는 것은 어떨까요? routes 에는 정말 요청을 처리하는 로직만 있는 것이 좋습니다. 나중에 배우시겠지만 계산, database I/O 등은 보통 router 함수, 파일에 넣지 않습니다.
https://github.com/lemonpie313/sparta-nodejs-teamproject-newsfeed/blob/f6a37724f87a772cc08792f634682db6a942ba29/src/routers/auth.router.js#L145-L170 https://github.com/lemonpie313/sparta-nodejs-teamproject-newsfeed/blob/f6a37724f87a772cc08792f634682db6a942ba29/src/routers/auth.router.js#L200-L224
민감한 데이터는 console로 출력하지 않는 것이 좋습니다. console로 출력을 하게 되면 추후 보안 문제가 생길 수도 있습니다.
https://github.com/lemonpie313/sparta-nodejs-teamproject-newsfeed/blob/f6a37724f87a772cc08792f634682db6a942ba29/src/routers/auth.router.js#L182-L185
아래 코드는 if (profile.Role == ROLE.ADMIN) 으로 대체할 수 있지 않을까요?
if (profile.Role == ROLE.ADMIN)
https://github.com/lemonpie313/sparta-nodejs-teamproject-newsfeed/blob/f6a37724f87a772cc08792f634682db6a942ba29/src/routers/users.router.js#L156-L162
아래 코드의 경우 어떤 역할을 수행하는 것인지 바로 이해하기가 어렵습니다. 이런 경우 주석을 달아주거나 별도의 함수를 사용하는 것이 협업에 도움이 됩니다.
https://github.com/lemonpie313/sparta-nodejs-teamproject-newsfeed/blob/f6a37724f87a772cc08792f634682db6a942ba29/src/routers/users.router.js#L279-L284
현재 PATCH로 follow/:userId를 호출하여 follow, unfollow를 토글 스위치를 켜고 끄는것 처럼 동작하는 데요. 이 경우 어떤 동작이 일어날 것인지 URL 에서 바로 알기가 여려워 직관성이 떨어지는 API가 됩니다. API 경로 혹은 query나 body에 follow 혹은 unfollow에 대한 요청을 명시해주는 것이 좋습니다.
follow/:userId
아래 경로는 posts/:postId/detail이 좋아보입니다.
posts/:postId/detail
https://github.com/lemonpie313/sparta-nodejs-teamproject-newsfeed/blob/f6a37724f87a772cc08792f634682db6a942ba29/src/routers/posts.router.js#L285
아래 코드는 Group: group 으로 사용해야하지 않을까요?
Group: group
https://github.com/lemonpie313/sparta-nodejs-teamproject-newsfeed/blob/f6a37724f87a772cc08792f634682db6a942ba29/src/routers/posts.router.js#L547
피드백 확인했습니다 감사합니다.😊
Feedback
총평
개선점
코드 분리
아래 코드는 routes 가 아닌 다른 곳으로 옮겨보는 것은 어떨까요? routes 에는 정말 요청을 처리하는 로직만 있는 것이 좋습니다. 나중에 배우시겠지만 계산, database I/O 등은 보통 router 함수, 파일에 넣지 않습니다.
https://github.com/lemonpie313/sparta-nodejs-teamproject-newsfeed/blob/f6a37724f87a772cc08792f634682db6a942ba29/src/routers/auth.router.js#L145-L170 https://github.com/lemonpie313/sparta-nodejs-teamproject-newsfeed/blob/f6a37724f87a772cc08792f634682db6a942ba29/src/routers/auth.router.js#L200-L224
민감한 데이터는 출력 X
민감한 데이터는 console로 출력하지 않는 것이 좋습니다. console로 출력을 하게 되면 추후 보안 문제가 생길 수도 있습니다.
https://github.com/lemonpie313/sparta-nodejs-teamproject-newsfeed/blob/f6a37724f87a772cc08792f634682db6a942ba29/src/routers/auth.router.js#L182-L185
불필요한 코드
아래 코드는
if (profile.Role == ROLE.ADMIN)
으로 대체할 수 있지 않을까요?https://github.com/lemonpie313/sparta-nodejs-teamproject-newsfeed/blob/f6a37724f87a772cc08792f634682db6a942ba29/src/routers/users.router.js#L156-L162
주석 혹은 로그
아래 코드의 경우 어떤 역할을 수행하는 것인지 바로 이해하기가 어렵습니다. 이런 경우 주석을 달아주거나 별도의 함수를 사용하는 것이 협업에 도움이 됩니다.
https://github.com/lemonpie313/sparta-nodejs-teamproject-newsfeed/blob/f6a37724f87a772cc08792f634682db6a942ba29/src/routers/users.router.js#L279-L284
Follow / Unfollow
현재 PATCH로
follow/:userId
를 호출하여 follow, unfollow를 토글 스위치를 켜고 끄는것 처럼 동작하는 데요. 이 경우 어떤 동작이 일어날 것인지 URL 에서 바로 알기가 여려워 직관성이 떨어지는 API가 됩니다. API 경로 혹은 query나 body에 follow 혹은 unfollow에 대한 요청을 명시해주는 것이 좋습니다.RESTful API
아래 경로는
posts/:postId/detail
이 좋아보입니다.https://github.com/lemonpie313/sparta-nodejs-teamproject-newsfeed/blob/f6a37724f87a772cc08792f634682db6a942ba29/src/routers/posts.router.js#L285
동작하지 않는 코드
아래 코드는
Group: group
으로 사용해야하지 않을까요?https://github.com/lemonpie313/sparta-nodejs-teamproject-newsfeed/blob/f6a37724f87a772cc08792f634682db6a942ba29/src/routers/posts.router.js#L547