gidalim / newFanLetters

MIT License
0 stars 0 forks source link

박강토님 코드리뷰 #1

Open JaeSang1998 opened 8 months ago

JaeSang1998 commented 8 months ago

https://github.com/gidalim/newFanLetters/blob/ae83dd7688ddd4cfccb71d0b5c777d5c3611c52a/src/axios/api.js#L46

불필요한 코드 주석은 삭제해주세요~!

https://github.com/gidalim/newFanLetters/blob/ae83dd7688ddd4cfccb71d0b5c777d5c3611c52a/src/redux/modules/authSlice.js#L82-L85

이 부분에 대한 공통 로직이 많은데 util 함수로 분리하는게 더 좋아보입니다.

https://github.com/gidalim/newFanLetters/blob/ae83dd7688ddd4cfccb71d0b5c777d5c3611c52a/src/redux/modules/modalSlice.js#L15

이 부분에서 isDivVisible 보다는 modal 이라는 이름을 쓰는게 좋습니다. isModalOpen 과 기능이 중복되어 보이는데, 하나의 불리언으로 관리되면, 혹은 더 명시적인 이름을 가진다면 더 좋을 것 같아요.

https://github.com/gidalim/newFanLetters/blob/ae83dd7688ddd4cfccb71d0b5c777d5c3611c52a/src/components/units/homeComponent/CommentCreator.jsx#L8

CommentCreator 라는 이름이 와닿지는 않는것 같아요. 다른 이름을 고려해봅시다. CommentRenderer Creator 와 Renderer 의 차이점이 어떤게 있는지도 파일명을 통해 유추가 어렵습니다.

https://github.com/gidalim/newFanLetters/blob/ae83dd7688ddd4cfccb71d0b5c777d5c3611c52a/src/components/units/homeComponent/CommentRenderer.jsx#L6

getFanLetterId 라고 되어있는데 내부 로직은 navigate 에요. 이 부분은 수정이 필요해보입니다.

https://github.com/gidalim/newFanLetters/blob/ae83dd7688ddd4cfccb71d0b5c777d5c3611c52a/src/components/units/homeComponent/CommentRenderer.jsx#L10

if else 보다는 삼항연산자를 쓰거나 혹은 else 로직이 없는 early return 로직이 더 깔끔해 보일 것 같아요.

https://github.com/gidalim/newFanLetters/blob/ae83dd7688ddd4cfccb71d0b5c777d5c3611c52a/src/pages/Login/Login.jsx#L59 https://github.com/gidalim/newFanLetters/blob/ae83dd7688ddd4cfccb71d0b5c777d5c3611c52a/src/pages/Login/Login.jsx#L73

폼을 쓰고있다면 비제어 컴포넌트 형태로 사용해보시는걸 추천드려요. 불필요한 랜더링 프로세스를 굳이 가져갈 필요는 없습니다.

thunk 부분은 꼼꼼하게 잘 작성해주셨네요. 비지니스 로직을 전개하는 건 굉장히 익숙해 지신것 같습니다. 다만 디테일한 부분들을 신경써보면 좋을 것 같네요. 고생하셨습니다 강토님 :)

gidalim commented 8 months ago

사실은 이번엔 조금.. 아니, 많이 모자라서 더 많이 배우고 다 갈아엎고, 깔끔하게 수정하고 싶은 마음이 가득입니다. 리뷰해주셔서 감사합니다. 더 좋은 개발자가 되도록 노력하겠습니다