[심주원] 스프린트 미션 6#82
Conversation
devbini
left a comment
devbini
left a comment
There was a problem hiding this comment.
🎉 고생 많으셨습니다!
이번 PR은 기존 프론트엔드 구현 위에 Express API부터 Prisma ORM에 Postgre 등등 범위가 꽤 넓었는데...
모델을 나누고 댓글 모델을 나눠서 각각 연결한 흐름까지 처음 백엔드 관계형 모델링을 적용하는 단계치고 굉장히 잘 하신 것 같아요.
특히 onDelete: Cascade라던가 HTTP 요청 샘플까지 준비한 점은 다음 작업자가 프로젝트를 이해하는 데 큰 도움이 됩니다. (유지보수적으로 너무 좋아요. 테스트할 때도 좋구요.)
그래도 요구사항 기준으로는 API의 존재뿐 아니라, 잘못된 입력을 어떻게 막을지라거나, 페이지네이션이 실제 데이터가 늘어났을 때 라던지,.. 조금 더 확인이 필요한 부분이 있기는 했어요. 그럼에도 이 부분은 조금만 정리해도 완성도가 많이 올라갈 것 같습니다.
또 기억에 남는 게, 로딩/에러/성공 상태에 대해 FE에서 분리하여 렌더링하는 부분도 좋았습니다. 특히 finally에서 로딩을 끄는 state를 사용하던데, 그것도 좋은 습관으로 보여요.
별도 훅을 만들어 검증하는 구조도 좋았구요. 어느정도 캡슐화에 대해서도 이해하고 응용할 줄 아는 것 같습니다.
앞으로도 기대가 되는 코드입니다.
너무나 고생 많으셨고, 파이팅입니다! 👍
| router.post("/", async (req, res) => { | ||
| const { name, description, price, tags, image } = req.body; | ||
|
|
||
| const product = await prisma.product.create({ | ||
| data: { | ||
| name, | ||
| description, | ||
| price: Number(price), | ||
| tags: tags ?? [], | ||
| image: image ?? "", | ||
| }, | ||
| }); | ||
|
|
||
| res.status(201).json(product); | ||
| }); |
There was a problem hiding this comment.
☕ Thinking
다른 부분도 마찬가지인데,
문법상 저장하는 데에는 문제가 없지만 실제 코드에 적용되었을 때 이상한 데이터를 검증(필터링)하는 부분이 없는 것 같아요.
API함수가 예측 가능한 범주 안에서 핸들링이 되어야한다
가 사실 프로덕션 레벨의 웹앱을 개발할 때 생각하는 기본 베이스라서, 한번 더 검증하는 코드를 넣는 습관이 있으면 어떨까요?
| const offset = Number(req.query.offset) || 0; | ||
| const limit = Number(req.query.limit) || 10; |
There was a problem hiding this comment.
☕ Thinking
이것도 문제는 아니지만,
숫자를 바로 DB로 던지고 있는 것 같아요. 페이지네이션 관련 데이터를..
정상적인 상황에서는 문제가 없지만, 실제 환경에서는 어떤 문제가 나올지 몰라요.
예를 들어- limit 데이터가 음수로 들어간다거나, 비정상적으로 엄청 큰 데이터가 들어간다던가...
Math 라이브러리를 활용하면 아마 상/하 제한을 걸 수 있을텐데, 서버 리소스 보호를 생각하는 방법이 하나 더 있지 않을까 싶어 남겨둡니다ㅎㅎ
| orderBy: { | ||
| createdAt: "desc", | ||
| }, |
There was a problem hiding this comment.
☕ Thinking
음, 음.. 이건 제 개인적인 경험에 빗대어 말씀드리는 거긴 합니다만
"동일한 시간대"의 데이터를 읽어오는 경우 꼬이는 경우가 종종 있어요.
거의 있을 수는 없겠지만, 그래도 정렬 기준을 하나만 잡기 보다는 유니크한 컬럼을 추가하여 두 개를 써 보는 것도 좋은 방법이지 않을까 싶어요!
예를 들면.. id 같은 컬럼이요!
| description: description.trim(), | ||
| price: Number(price), | ||
| tags: tags, | ||
| image: "", |
There was a problem hiding this comment.
🚩 Check Point
상품 등록 요청에 Image(상품 사진)이 있는데, 정작 가시화되는 화면엔 이미지 URL을 입력한다거나, 파일을 넣는 구조가 없는 것 같아요.
| res.status(200).json({ | ||
| list: comments, | ||
| nextCursor, | ||
| }); |
There was a problem hiding this comment.
☕ Thinking
이것도 개인적인 최적화 팁입니다!
유지보수의 꽃은 사실 "파일 수 줄이기" 로 볼 수도 있을 것 같아요 (개인적인 생각)
공통 함수로 묶을 수 있는 건 최대한 묶어서 캡슐화 하자-가 제 모토이기도 하거든요!
products.js에 보면 비슷한 함수가 있더라구요. 내부 로직은 거의 동일한데 쓰이는 데이터만 다른 느낌.
댓글 응답이나 페이지네이션에 있어서 두 파일을 각각 수정하면서 휴먼 에러가 나올 요소를 남기는 것 보단, 공통 서비스 함수를 하나 만들어보는 건 어떨까요?
| <h2 className="name-title">상품명</h2> | ||
|
|
||
| <div className="input-group"> | ||
| <input |
There was a problem hiding this comment.
☕ Thinking
이왕 다른 파일에서 label-htmlFor를 잘 쓰고 있는 만큼, 여기 등록 페이지에도 적용 해 보는 건 어떨까요?
접근성 적으로 큰 도움이 될 것 같아요~!
브라우저 자동 완성에도 도움이 된답니다!!
| </a> | ||
| <a href="faq.html" className="footer-link"> | ||
| </Link> | ||
| <Link to="faq.html" className="footer-link"> |
There was a problem hiding this comment.
🚩 Check Point
React Route를 쓰면서 Link를 사용한 건 좋습니다.
다만 지금 상대경로를 쓰고 있어서 조금 걱정이 됩니다.
클릭 가능한 링크는 맞지만, 만약 이 버튼을 /items 같은 다른 페이지에서 누르게 되었을 때
/items/faq.html 로 접속되는 불상사가 일어날 수도 있어요.
가능하면 각자의 전용 라우트를 만들어서 절대 경로로 연결 해 주시는 게 좋습니다ㅎㅎ
|
|
||
| console.error(err); | ||
|
|
||
| res.status(500).json({ |
There was a problem hiding this comment.
🚩 Check Point
음.. 이건 조금 위험해요.
요구사항에 500에 대한 응답 에러처리가 있어서 이렇게 해도 되는건지는 잘 모르겠지만..
500의 경우에는 DB 연결 내용이나 구현에 있어 치명적일 수 있는 서버 오류메시지가 사용자에게 그대로 전달되게 됩니다.
가능하면 message를 하드하게 별도로 만들거나, 에러핸들러를 따로 만들어서 관리하는 게 좋을 것 같아요.
블라인드 SQL 인젝션 등, 다양한 보안적 위험요소가 존재한답니다.
자세한 오류 로그는 서버 로그에만 남기는 게 좋아요! 🥹
| productId String? | ||
| articleId String? |
There was a problem hiding this comment.
🚩 Check Point
현재 스키마 구조를 보면,
productId와 articleId가 모두 없는 댓글도 만들 수 있고, 반대로 둘 다 있는 댓글을 만들 수 있네요.
XOR 제약을 직접 표현하기 어렵다면, 댓글 생성 라우터에서 productId와 articleId 중 하나만 들어가도록 검증을 추가하거나, ProductComment, ArticleComment와 같이 모델을 분리하는 방법이 필요 해 보여요.
2 participants
요구사항
기본 요구사항
중고마켓
공통
onDelete를 설정했습니다.자유게시판
id,title,content,createdAt,updatedAt필드를 가지도록 구현했습니다.title,content를 입력해 게시글을 등록하도록 구현했습니다.id,title,content,createdAt를 조회하도록 구현했습니다.id,title,content,createdAt를 조회하도록 구현했습니다.title,content에 포함된 단어로 검색할 수 있도록 구현했습니다.댓글
content를 입력하여 댓글을 등록하도록 구현했습니다.id,content,createdAt를 조회하도록 구현했습니다.