코드리뷰용 PR 생성 #186
코드리뷰용 PR 생성 #186
Conversation
[feature] 완료버튼 기능 및 글자수 제한 기능 추가 #41
[bug] navigation 경로 수정, + 버튼 색상 바뀌지 않도록 수정
hotfix: 서버 에러 수정, 오타 수정, playlist?.id로 옵셔널 체이닝(타입오류 해결하기 위해)
[Style] SignIn Page 퍼블리싱 #54
[Style] Splash Page 퍼블리싱 #55
Feature/ 프로필페이지 UI 구현 #60
[Setting] 파이어베이스 인증 관련 코드 리팩토링 #61
[feature] 북마크 UI 구현
[style] globalStyles 파일 수정, 스타일 적용 import 수정, navigation 작동 방식 변경, SavedPlaylistDetail 파일 생성 및 라우트 연결
[Style] 플레이리스트 상세보기 페이지 퍼블리싱 및 버그 수정
|
안녕하세요. playwright test가 깨져서 failed 상태인 것 같은데요. |
|
제가 코멘트 드리는 방식 간단히 소개 드립니다. 우선순위P1: 반드시 바꿔주세요. 바꾸지 않으면 머지 불가 리뷰 예시실제 리뷰에서 아래처럼 작성할 예정입니다. 우선순위 감안하여 반영하시거나 논의 이어가면 좋을 것 같습니다. |
[Refactoring] 리팩토링 코드리뷰 수정 #194
| import { db } from '@/api/firebaseApp' | ||
| import { doc, getDoc, updateDoc, setDoc, deleteDoc } from 'firebase/firestore' | ||
|
|
||
| export const fetchLikes = async (playlistId: string) => { |
There was a problem hiding this comment.
P2;
제일 아래 else문 내의 return에서 userLikes를 빈 객체로 반환하다 보니 캡쳐한 사진처럼 fetchLikes에서 any 타입으로 반환한다고 뜹니다.
userLikes가 있다면 어떤 구조가 되나요? 별도로 타입을 지정해주면 좋을 것 같습니다.
함수가 return하는 인자가 any이면 함수를 가져다 사용하는 쪽에서 어떤 타입일지 알지 못해 방어하는 코드가 늘어납니다.
또, 함수는 재사용가능성이 높은데요. 여러 사람이 협업하는 환경에서는 내가 짠 함수를 다른 사람이 사용하는 경우도 흔합니다.
함수가 any를 반환하면 어떤 값을 기대해야할지 알지 못해 함수의 구현을 직접 하나하나 읽어봐야 하게 됩니다. 불편하죠.
객체지향적인 코드를 짜는 원리도 인터페이스만 공개하고 구현부는 은닉하는 것이 중요한 만큼, 함수는 아래와 같은 원칙을 가지고 만들면 좋겠습니다.
- 함수의 인자(input parameter)와 return type을 명시적으로 둔다. (any 금지)
- 함수의 이름은 최대한 직관적으로 작성해 이름만 봐도 그 함수가 어떤 일을 하는지 알 수 있게 한다.
- 가능하다면 unit-test를 작성한다. (비동기로직 등 복잡하지 않다면.) 이로써 다른 작업자는 함수의 사용 예시를 테스트코드로 확인 가능하다.
return { likeCount: 0, userLikes: {} }| if (newLikeCount > 0) { | ||
| await updateDoc(docRef, { | ||
| likeCount: newLikeCount | ||
| }) | ||
| } else { | ||
| await deleteDoc(docRef) | ||
| } | ||
| } | ||
| } else { | ||
| if (isLiked) { | ||
| newLikeCount = 1 | ||
| await setDoc(docRef, { | ||
| likeCount: newLikeCount, | ||
| userLikes: { [userId]: true } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
P4;
조금 위에서 early-return에 대해 말씀드렸는데,
괄호 depth가 복잡해지면 아무래도 개발자가 이해하다 실수할 가능성이 높아지죠. 그래서 depth가 깊을 수록 early-return으로 불필요한 괄호의 depth를 줄여주는 것도 좋은 것 같습니다.
실제 제가 있었던 회사 중에서 괄호를 닫는 위치를 잘못 설정하여, 고객에게 나가면 안될 메시지가 대량으로 발송된 사례도 있었습니다.
(꼭 고치라는 얘기는 아닙니다. P4이니까 토의해보고 결정하면 좋을 것 같습니다)
There was a problem hiding this comment.
if (!docRef) return;
if (newLikeCount === 0) {
await deleteDoc(docRef);
return;
}
await updateDoc(docRef, {
likeCount: newLikeCount
});
이렇게 고쳐봤는데 혹시 맞는걸까요?
- newLikeCount === 0일 경우, deleteDoc을 실행하고 함수 종료
- newLikeCount > 0일 경우에는 await updateDoc만 실행
There was a problem hiding this comment.
일단 좀 수정 범위가 커지는 주제인 것 같은데요.
- updateLikes의 비동기 로직이 실패하면 어떻게 될까요?
updateDoc이나 deleteDoc이 실패하게 되면 DB에 기록된 likeCount와 사용자가 웹에서 보는 likeCount와 달라질 수도 있을 것 같은데요 (맞을까요??)
- updateLikes가 deleteDoc, updateDoc을 호출한 뒤, 성공하였다면 변경된 likeCount를 반환하고, updateLikes를 사용하는 훅 에서는 그 likeCount를 받아 상태를 업데이트하면 어떨까요? (저장소와 사용자가 보는 화면 간 데이터 동기화)
- updateLikes 및 그 함수를 사용하는 훅 또는 컴포넌트에 에러 대응을 하는 로직이 추가되면 좋을 것 같습니다.
만약 updateLikes가 반환값을 갖게 된다면 if-else 조건문 안에서 적절히 return 을 함으로써 early-return이 가능해질 것 같아요.
| getDocs | ||
| } from 'firebase/firestore' | ||
|
|
||
| const ITEMS_PER_PAGE = 10 |
There was a problem hiding this comment.
P3;
상수를 전역으로 상수로 선언한 것은 아주 잘한 점입니다. 자주 사용되는 값은 좀 더 이해 가능한 상수명으로 지금처럼 선언하는게 맞습니다.
다만, fetch하는 item의 갯수는 상수로 두더라도 함수를 사용하는 측에서 조정할 수 있게 하면 좋을 것 같습니다.
- fetchPlaylists를 사용하는 주체가 불러오려는 갯수를 결정할 수 있어야 재사용성이 극대화됩니다. 예를들어, mypage를 새로 만들었는데 거기서는 무한스크롤을 하지 않고 5개씩 pagination해 보여줄지도 모릅니다.
- fetchPlaylists가 items per page를 인자로 받지 않기 때문에, fetchPlaylists 함수를 사용하는 개발자는 이 함수가 몇개의 결과를 반환하는지 알지 못합니다. 이를 알고 싶으면 fetchPlaylists 함수의 실제 구현을 하나하나 읽어봐야 합니다.
There was a problem hiding this comment.
아하 재사용성을 위해서는 이렇게 하면 안 되는 거였군요 감사합니다!
There was a problem hiding this comment.
고생 많으셨습니다.
스타일(css) 관련된 부분 제외하고 리뷰 다 남겼습니다.
코드리뷰
코드리뷰에 대해 첨언을 드리자면, 리뷰어가 지적하고 리뷰이가 수정하는 일방소통은 아니라고 생각합니다.
제가 질문을 좀 남기기도 했고, 각자 의견이 갈리는 부분도 있을텐데 적극적으로 본인의 code를 defense해주시면 좋을 것 같습니다.
후배 개발자를 뽑아서 코드리뷰를 하다 보면, defense를 잘 못하는 경우를 종종 보는 데, 이런 경우 자기주도성이나 논리적 판단역량에 의문을 품게 되는 것 같습니다.
defense를 하려다 보면 내용도 찾아보게 되고, 그러면서 공부도 된다고 생각합니다. 적극적으로 defense해주세요.
| src={userData?.photoURL || 'https://example.com/default-profile.png'} | ||
| alt={userData?.displayName || 'User'} |
There was a problem hiding this comment.
P4;
userData는 Playlist 컴포넌트를 사용하는 상위 컴포넌트에서 넣어주는 것 같은데요.
userData를 fetch하는 상위 컴포넌트나, fetch하는 함수에서 undefined, null 값을 대응해주는 것이 가능하다면 그렇게 해주면 좋을 것 같습니다.
| try { | ||
| const { comment, playlistId } = payload | ||
| const db = getFirestore() | ||
| const user = auth.currentUser | ||
| const coll = collection(db, 'Comments') | ||
| if (!user) return | ||
| await addDoc(coll, { | ||
| user: { | ||
| userId: user.uid, | ||
| displayName: user.displayName, | ||
| photoURL: user.photoURL | ||
| }, | ||
| content: comment, | ||
| createdAt: new Date().toISOString(), | ||
| playlistId | ||
| }) | ||
| } catch (error) { | ||
| console.error('Failed to add comment:', error) | ||
| } |
There was a problem hiding this comment.
P3;
onError 메서드를 사용하는 것은 어떤가요? (onSuccess처럼요)
https://tanstack.com/query/v4/docs/framework/react/reference/useMutation
| : Playlists | ||
|
|
||
| // playlistId와 bookmarkTimestamp를 이용하여 북마크 시점으로 최신 정렬 | ||
| const filteredPlaylists = filteredData |
There was a problem hiding this comment.
P3;
전반적으로 로직이 복잡하고, optional한 데이터를 많이 다루고 있기 때문에 이렇게 메소드 체이닝을 이용하지 않고 별도로 sort를 해주는 메서드를 만들면 어떨까요?
const sortPlaylistsByBookmarkTimestamp = (playlist, bookmarkmap): sortedPlaylists => {
// bookmarkmap이 없는 경우 등 optional 케이스 대응
// sort 로직
}| ?.filter((pl): pl is PlaylistType => { | ||
| return ( | ||
| typeof pl.id === 'string' && | ||
| typeof pl.title === 'string' && | ||
| Array.isArray(pl.urls) && | ||
| typeof pl.createdAt === 'string' && | ||
| typeof pl.userId === 'string' && | ||
| Array.isArray(pl.categories) && | ||
| selectedCategories.some(cat => pl.categories.includes(cat)) | ||
| ); | ||
| }) |
There was a problem hiding this comment.
P3;
filter 메서드에 넣어줄 익명함수를 별도 함수로 분리해도 좋을 것 같습니다.
There was a problem hiding this comment.
보통 이런 함수들이 unit test가 필요한 함수이기도 합니다.
There was a problem hiding this comment.
오오 이 부분은 팀원들과 함께 이야기해 보겠습니다! 감사합니다!
|
|
||
| const filteredPlaylists = data | ||
| ?.filter(pl => pl.userId === user?.uid) | ||
| ?.sort( |
There was a problem hiding this comment.
| ?.sort( | |
| .sort( |
요렇게 해도 괜찮을까요?
There was a problem hiding this comment.
의견주신 부분 찾아보니
?.filter(pl => pl.userId === user?.uid) 부분에 data가 반드시 존재한다고 확신할 때에는 옵셔널 체이닝을 사용하지 않아도 된다는 것을 알게 되었습니다. 다만 data가 null 혹은 undefined일 경우 sort 부분이 호출되지 않기 때문에 조금 더 안전하다고 생각했습니다.
다른 부분에서 예상치 못한 에러 등으로 인해 null이나 undefined가 발생할 경우를 생각해서 옵셔널 체이닝을 넣어주는 방식은 개발함에 있어서 어떻게 평가될까요 ?
감사합니다.
There was a problem hiding this comment.
optional chainning 자체는 문제가 되진 않는다고 생각하는데요.
제가 실제로 코드를 동작시켜본 것은 아닌데, data는 옵셔널이니까 data?.filter가 맞을 것 같은데, filter부터는 data가 있을 때만 실행되니 filter의 결과에 다시 ?.sort를 붙일 필요는 없지 않나 생각했고, 그 부분을 여쭤본 것입니다.
예를들어, filter 메서드는 filter조건에 해당되는 값만 배열로 반환할텐데, 조건을 만족하는 값이 없다면 빈 배열을 반환할 것이고, sort 메서드는 빈 배열을 sort할 것이니 sort 메서드가 받는 배열은 옵셔널이 아니지 않냐는 의미였습니다.
const filteredPlaylists = data
?.filter(pl => pl.userId === user?.uid) // data는 옵셔널임
.sort( // sort 로직)There was a problem hiding this comment.
더불어, 저는 사실 이런 로직들이 unit test 되면 좋겠다는 생각을 하는데요.
지금 보시다 시피, 저도, 수현님도 data 값이 없으면 어떻게되지? data는 있는데 filter한 값이 없으면 어떻게 되지? 에 대해 명확히 인지하지 못하고 있는데요. 이런 케이스를 테스트 케이스로 만들어 테스트해 본다면 개발자는 확신을 가지고 개발할 수 있습니다.
안녕하세요 멘토님 ! 테스트를 제가 진행했었는데요, 테스트 코드 짜는 것을 발표 전날 밤부터 시작했던 것이라 시간이 부족해서 가장 간단한 로그인을 진행했었습니다. 혹시 이렇게 화면은 잘 넘어가는데 url이 그대로인 것으로 나오는 경우 제가 예상한 것 처럼 라우팅 부분에 문제가 있는 것일까요 ? |
|
@suhyun9892 안녕하세요. 일단 테스트가 실패하게된 github action의 실패로그가 아래인 것 같은데요. 만약 그렇다면 아예 테스트에서 localhost:5173으로 연결 자체를 못한 것 같습니다. 만약 로컬에서도 테스트가 실패하고 계시다면 다른 이유일 수도 있을 것 같은데, 제가 firebase 권한이 없고 따로 config 값 같은 것도 없어서 로컬에서는 앱 실행 자체가 안되는 것 같습니다. 그래서 테스트가 실패하는 이유를 제 쪽에서는 확인하기 어려운 것 같습니다. 우선은 테스트를 했을 때 발생하는 에러메시지를 공유해 주시면 도움이 될 것 같습니다. |
에러 메세지 공유 드립니다 ! |
[Fix] 1차 코드리뷰 중 수정 사항 반영
[Refectoring] 중복 코드 삭제 및 스타일 변경
[refactoring] 1차 코드리뷰 일부 수정 완료
| const newCount = newLikeFilled ? likeCount + 1 : likeCount - 1 | ||
| setLikeCount(newCount) | ||
|
|
||
| await updateLikes(playlistId, userId, newLikeFilled) |
코드리뷰용 PR 생성하였습니다. 감사합니다 ! :)