-
Notifications
You must be signed in to change notification settings - Fork 70
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
[북마크] 북마크한 세션 목록 화면 구현 #220
[북마크] 북마크한 세션 목록 화면 구현 #220
Conversation
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.
편집 기능도 고려하여 완성도 높은 기능을 구현해주셨네요 👏
몇 가지 코멘트 확인을 부탁드립니다.
feature/bookmark/src/main/java/com/droidknights/app2023/feature/bookmark/BookmarkViewModel.kt
Outdated
Show resolved
Hide resolved
feature/bookmark/src/main/java/com/droidknights/app2023/feature/bookmark/BookmarkViewModel.kt
Outdated
Show resolved
Hide resolved
feature/bookmark/src/main/java/com/droidknights/app2023/feature/bookmark/BookmarkViewModel.kt
Outdated
Show resolved
Hide resolved
feature/bookmark/src/main/java/com/droidknights/app2023/feature/bookmark/BookmarkViewModel.kt
Outdated
Show resolved
Hide resolved
...re/bookmark/src/main/java/com/droidknights/app2023/feature/bookmark/listitem/BookmarkCard.kt
Outdated
Show resolved
Hide resolved
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.
북마크 화면을 한번에 구현을 해주셨네요. 고생 많으셨습니다
여러 로직을 한번에 수정 요청드리기 보다는 부분적으로 나눠서 드리면 좋을 것 같네요.
또한 UI 구현의 관점보다는 기능 관점에서의 피드백을 중점적으로 드렸습니다.
feature/bookmark/src/main/java/com/droidknights/app2023/feature/bookmark/text/RoomText.kt
Outdated
Show resolved
Hide resolved
...re/bookmark/src/main/java/com/droidknights/app2023/feature/bookmark/listitem/BookmarkCard.kt
Outdated
Show resolved
Hide resolved
...re/bookmark/src/main/java/com/droidknights/app2023/feature/bookmark/listitem/BookmarkCard.kt
Outdated
Show resolved
Hide resolved
feature/bookmark/src/main/java/com/droidknights/app2023/feature/bookmark/BookmarkUiState.kt
Outdated
Show resolved
Hide resolved
feature/bookmark/src/main/java/com/droidknights/app2023/feature/bookmark/BookmarkUiState.kt
Outdated
Show resolved
Hide resolved
feature/bookmark/src/main/java/com/droidknights/app2023/feature/bookmark/BookmarkUiState.kt
Outdated
Show resolved
Hide resolved
feature/bookmark/src/main/java/com/droidknights/app2023/feature/bookmark/BookmarkViewModel.kt
Outdated
Show resolved
Hide resolved
...re/bookmark/src/main/java/com/droidknights/app2023/feature/bookmark/listitem/BookmarkCard.kt
Outdated
Show resolved
Hide resolved
feature/bookmark/src/main/java/com/droidknights/app2023/feature/bookmark/BookmarkScreen.kt
Outdated
Show resolved
Hide resolved
feature/bookmark/src/main/java/com/droidknights/app2023/feature/bookmark/BookmarkViewModel.kt
Outdated
Show resolved
Hide resolved
리뷰를 반영하여 컴포저블 함수에서 지양하는 return문 제거
- BookmarkItemUiState의 프로퍼티들이 Session과 유사하며 값들을 풀어서 소유하고 있을 때보다는 session자체를 가지는 경우, 로직 작성이 더 수월하다고 판단하여 수정합니다.
- 'listitem' 패키지를 제거하고 존재하던 파일들을 상위 패키지로 이동합니다 - BookmarkItemUiState를 파일로 분리합니다
- BookMark된 Session을 가져오는 로직을 개선합니다. - 북마크된 세션들을 가져오는 GetBookmarkedSessionsUseCase를 생성합니다. - GetBookmarkedSessionsUseCase에 대한 테스트코드를 추가합니다
- 기존 Session 모듈과 Bookmark모듈은 공통적인 컴포넌트를 가지고 있어 core:ui 모듈에 통합합니다. - 각 모듈에 존재하는 중복 코드들은 제거합니다
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.
많은 코멘트 반영하시느라 고생 많으셨습니다 🙇 👏
|
||
Then("북마크된 Id에 해당하는 세션을 반환한다") { | ||
bookmarkedSessions.size shouldBe 2 | ||
bookmarkedSessions.map { it.id } shouldContainAll expected |
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.
정렬 로직도 테스트하면 어떨까요?
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.
넵 정렬로직도 중요한 부분이니 테스트 하는게 맞네요
dd18b76 에서 추가했습니다!
그런데 한가지 질문이 있습니다!
위 커밋에서는 테스트를 Then을 하나 더 추가하도록 구현을 했는데
만약 리스트의 순서가 섞인것을 가정한 경우를 추가적으로 테스트하고 싶다면 아래의 방법이 맞는지 궁금합니다!
- 순서가 섞인 suffledList(mock 데이터)를 하나 더 만든다
- FakeRepo, UseCase를 하나 더 생성한다
- GWT 테스트 케이스를 하나 더 만든다
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.
@toastmeister1 정렬 로직의 테스트 코드를 작성하는 방법은 여러 가지가 있겠지만, 가장 중요한 것은 개발자에게 얼마나 심리적 안정감을 줄 수 있는가? 인 것 같아요. 제시해주신 방법도 좋지만 단순히 현재 작성된 데이터셋의 순서를 변경하고, 이게 잘 정렬되었는지 테스트하는 것만으로도 충분하다고 생각합니다.
지금 작성하신 코드도 충분히 심리적 안정감을 준다면 좋습니다 👍
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.
어떤 맥락인지 이해가 가네요!
답변 감사합니다!
- 북마크된 세션들이 시간순으로 반환되는지에 대한 테스트케이스를 추가합니다
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.
고생 많으셨습니다.
@wisemuji
아래 커멘트 확인되셨으면 병합 부탁드립니다.
#220 (comment)
도와드린것보다 두분께 많은 도움을 받았네요 😅 컨퍼런스 성공적으로 마치시길 바랄게요 :) |
Issue
Overview (Required)
북마크한 세션 목록들을 화면에 표시하는 기능을 구현합니다
Links
Screenshot
empty.mp4
list.mp4