-
Notifications
You must be signed in to change notification settings - Fork 1
refactor: 가게 세부 화면 기능 분리 #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: hoya
Are you sure you want to change the base?
Conversation
private val storeRepository = StoreRepository() | ||
private val storeInfoRepository = StoreInfoRepository() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StoreRepository와 StoreInfoRepository를 나눠둔 이유가 있을까요? 둘 다 메서드 하나씩밖에 없는 것 같아서요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기능 분리에 너무 집중해서 과도하게 나눈 것 같습니다. StoreRepository 하나로 통일했습니다!
try { | ||
val collegeCode = collegeCodeMap[college] ?: throw IllegalArgumentException("Unknown college name: $college") | ||
val degreeCode = degreeCodeMap[degree] ?: throw IllegalArgumentException("Unknown degree name: $degree") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try 구문 내에서 throw를 하는 것보다는 명시적으로 리턴 시켜주는 건 어떨까요? 코틀린의 Result
클래스를 활용해보는 것도 좋을 것 같아요
하나의 커밋에 너무 여러 개의 변경사항이 들어있는 것 같아요. 이렇게 되면 리뷰어가 변경사항을 트래킹하기 어렵습니다 😢 |
return storeItems.map { store -> | ||
store.copy(isFavorite = favoriteItems.any { favorite -> favorite.storeId == store.id && favorite.isFavorite }) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isFavorite
필드를 매번 계산해주는 게 아니라 어딘가에 저장해두었다 재사용하면 더 효율적일 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
조금 헷갈리는 부분이 있습니다..! 현재 isFavorite이 1. 가게 리스트가 변경될 때 2. 즐겨찾기 상태가 변경될 때 업데이트 되는 것으로 보이는데, 혹시 제가 놓친 부분이 있을까요?
해서, 지금은 storeItems.map 내부에서 favoriteItems.any로 isFavorite을 찾고 있는데,
favoriteItems를 Map으로 변환한 후 storeItems에서 isFavorite를 조회하는 건 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음음 뭔가 제가 의도한 바가 잘 전달되지 않은 것 같네요..! 다음에 모각코 오시게 되면 설명드릴게욥
#13 과 변경사항이 충돌하는 것 같은데 맞을까요? 순서대로 진행하는 게 좋을 것 같습니다 |
Summary
Describe your changes