[Feat] 리뷰 v2 전체 구현 #384
[Feat] 리뷰 v2 전체 구현 #384
Conversation
Summary of ChangesHello @HI-JIN2, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 앱의 리뷰 기능을 Jetpack Compose를 사용하여 v2 버전으로 완전히 재구현하는 것을 목표로 합니다. 이를 통해 사용자 인터페이스의 현대화와 코드베이스의 유지보수성 향상을 꾀합니다. 기존 XML 레이아웃을 Compose 컴포넌트로 전환하고, 백엔드 API 변경사항에 맞춰 데이터 모델과 리포지토리 로직을 업데이트하며, 새로운 네비게이션 구조를 도입하여 전반적인 리뷰 시스템을 개선했습니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| } | ||
| override suspend fun getImageString(file: File): Flow<BaseResponse<ImageResponse>> = flow { | ||
| override suspend fun getMenuReviewInfo(menuId: Long): ReviewInfo { | ||
| return reviewService.getMenuReviewInfo(menuId).result?.toDomain()!! //non null 하면 안될 것 같은데 |
There was a problem hiding this comment.
non-null assertion(!!) 연산자는 NullPointerException을 발생시킬 수 있어 사용에 주의가 필요합니다. 주석으로도 우려를 표현해주셨듯이, API 응답이 null일 가능성이 있다면 더 안전한 방법으로 처리하는 것이 좋습니다. 반환 타입을 ReviewInfo?로 변경하거나, null일 경우 특정 예외를 던져 호출하는 쪽에서 처리하도록 하는 것을 권장합니다. getMealReviewInfo 함수에도 동일한 문제가 있습니다.
| return reviewService.getMenuReviewInfo(menuId).result?.toDomain()!! //non null 하면 안될 것 같은데 | |
| return reviewService.getMenuReviewInfo(menuId).result?.toDomain() |
| @SerializedName("rating") var rating: Int? = null, | ||
| @SerializedName("menuLikes") var menuLikes: List<MenuLikes> = arrayListOf(), | ||
| @SerializedName("content") var content: String? = null |
There was a problem hiding this comment.
DTO(Data Transfer Object)의 속성은 var 대신 val로 선언하여 불변(immutable)으로 만드는 것이 좋습니다. 데이터 클래스는 일반적으로 상태를 가지지 않는 데이터 컨테이너로 사용되므로, 불변성을 유지하면 코드의 안정성과 예측 가능성을 높일 수 있습니다. 이 패턴은 다른 DTO 파일들(WriteMealReviewRequest.kt, WriteMenuReviewRequest.kt 등)에도 동일하게 적용하는 것을 권장합니다.
| @SerializedName("rating") var rating: Int? = null, | |
| @SerializedName("menuLikes") var menuLikes: List<MenuLikes> = arrayListOf(), | |
| @SerializedName("content") var content: String? = null | |
| @SerializedName("rating") val rating: Int? = null, | |
| @SerializedName("menuLikes") val menuLikes: List<MenuLikes> = arrayListOf(), | |
| @SerializedName("content") val content: String? = null |
There was a problem hiding this comment.
다른 request 확인해보니까 review 작성 관련 request만 var로 선언되어있는데 이유가 있을까요?
There was a problem hiding this comment.
그리고 다른 request는 default값이 없는데 리뷰 작성 request들만 default값이 있는 이유도 궁금합니다!
There was a problem hiding this comment.
오 생각해보니까 request면 디폴트값이나 nullable을 규제하는게 맞겠네여 수정해올게요..!
제가 별 생각 없이 작업한 것 같아요🥹
app/src/main/java/com/eatssu/android/data/dto/response/MealReviewListResponse.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/eatssu/android/data/remote/dto/response/MenuReviewListResponse.kt
Show resolved
Hide resolved
| data class Menu( | ||
| val menuId: Long, | ||
| val name: String, | ||
| val isLike: Boolean, | ||
| ) |
There was a problem hiding this comment.
Review.Menu와 같은 커스텀 객체를 List 형태로 savedStateHandle을 통해 전달할 때는 Parcelable을 구현하는 것이 안전하고 효율적입니다. @Parcelize 어노테이션을 사용하면 Parcelable 구현을 쉽게 자동화할 수 있습니다.
@Parcelize
data class Menu(
val menuId: Long,
val name: String,
val isLike: Boolean,
) : Parcelable| private fun getIntents() { //todo 추후 변경 | ||
| menuType = intent.getStringExtra("menuType").toString() | ||
| itemId = intent.getLongExtra("itemId", 0) | ||
| itemName = intent.getStringExtra("itemName").toString().replace(Regex("[\\[\\]]"), "") |
There was a problem hiding this comment.
replace(Regex("[\[\]]"), "")를 사용하여 문자열을 처리하는 것은 데이터 형식 문제에 대한 임시방편으로 보입니다. 데이터가 Intent에 담기기 전, 즉 데이터의 원천(source)에서부터 불필요한 대괄호를 제거하는 것이 더 근본적이고 안정적인 해결책입니다.
| Text(content, style = EatssuTheme.typography.body3) | ||
|
|
||
| // 이미지가 있는 경우에만 표시 | ||
| if (!imgUrl.isNullOrBlank() && imgUrl != "null") { |
There was a problem hiding this comment.
이미지 URL이 유효한지 확인하기 위해 문자열 "null"과 비교하는 로직이 포함되어 있습니다. 이는 API가 JSON null 대신 문자열 "null"을 반환할 때 발생하는 문제로 보입니다. 이러한 처리는 UI 로직보다는 데이터를 파싱하는 단계(예: DTO Deserializer)에서 처리하여 UI 코드를 더 깔끔하게 유지하는 것이 좋습니다.
app/src/main/java/com/eatssu/android/presentation/mypage/myreview/MyReviewViewModel.kt
Outdated
Show resolved
Hide resolved
|
모든 DTO data class의 필드가 var과 nullable이고 기본값이 지정되어있는 이유가 별도로 있을까요? |
서버에서 오는 값을 non-null로 신뢰할 수 없기 때문입니다. non-null로 했는데, 실제로는 해당 값이 안오면 NPE이 터집니다.. 그래서 nullable로 받고, toDomain() 확장함수를 통해서 domain layer의 데이터 클래스(얘는 val에 non-null입니다) 로 변환해주도록 하고 있습니당 추가로, DTO 코드를 수동으로 짜지 않고 https://json2kt.com/ 를 이용해서 스웨거에서 json을 복붙하는 방식으로 하고 있어염 |
kangyuri1114
left a comment
kangyuri1114
left a comment
There was a problem hiding this comment.
수고하셨습니다!!!
리뷰 확인 부탁드려요
UI 디자인 및 서버 api 업데이트 되고 다시 한번 더 보겠습니다
| @SerializedName("rating") var rating: Int? = null, | ||
| @SerializedName("menuLikes") var menuLikes: List<MenuLikes> = arrayListOf(), | ||
| @SerializedName("content") var content: String? = null |
There was a problem hiding this comment.
다른 request 확인해보니까 review 작성 관련 request만 var로 선언되어있는데 이유가 있을까요?
| @SerializedName("rating") var rating: Int? = null, | ||
| @SerializedName("menuLikes") var menuLikes: List<MenuLikes> = arrayListOf(), | ||
| @SerializedName("content") var content: String? = null |
There was a problem hiding this comment.
그리고 다른 request는 default값이 없는데 리뷰 작성 request들만 default값이 있는 이유도 궁금합니다!
| isLike = menu.isLike ?: false | ||
| ) | ||
| }, | ||
| writerNickname = data.writerNickname ?: "유저", |
There was a problem hiding this comment.
유저 default 보다는 다른 값처럼 빈 스트링이 좋지 않을까요??
왜 얘만 유저라고 두었는지 궁금합니다
There was a problem hiding this comment.
오 그냥 딱히 이유는 없엇서여 ""로 바꾸겠슴다
| return menuList.map { | ||
| (it.menuId ?: -1L) to (it.name ?: "") | ||
| } |
There was a problem hiding this comment.
메뉴 id와 메뉴명을 Pair로만 반환하신 이유가 있을까요?
이전에는 data class 형태로 의미가 명확했는데, Pair만으로는 어떤 값이 어떤 역할인지 파악하기 어려워 보여서 여쭤봅니다.
혹시 단순 매핑용이라면 data class MenuMini(val id: Long, val name: String) 형태로 두는 것도 명시적일 것 같아서요..!
There was a problem hiding this comment.
흠 id랑 string 두 값이라서 굳이 데이터 모델을 만들어야하나? 라는 생각이 들어서 pair로 바꾸었는데, 별로 좋은 방법은 아니었나 봅니다. 다시 바꿔올게유
| one = reviewRatingCount?.oneStarCount ?: 0, | ||
| two = reviewRatingCount?.twoStarCount ?: 0, | ||
| three = reviewRatingCount?.threeStarCount ?: 0, | ||
| four = reviewRatingCount?.fourStarCount ?: 0, | ||
| five = reviewRatingCount?.fiveStarCount ?: 0, |
There was a problem hiding this comment.
[ P2 ]
이 one, two ... 의 의미가 뭔지 모르겠어서 계속 봤는데요
각 별점 점수에 대한 수군요
domain.model 패키지에 있는 ReviewInfo 데이터 클래스들의 이름도 ReviewRatingCount 데이터 클래스 처럼 변수명을 조금 더 명시적으로 바꾸면 어떨까요?
아래 사진에 있는 One, two, three도 oneStarCount 처럼 바꾸자는 말입니다!
| @Composable | ||
| fun ReviewItem( | ||
| isWriter: Boolean, | ||
| modifier: Modifier = Modifier, |
There was a problem hiding this comment.
아시다시피
modifier: Modifier = Modifier,
위치는 optional 값 위니까
isWriter: Boolean,
writeName: String,
writeDate: String,
content: String,
rating: Int,
modifier: Modifier = Modifier,
menuList: List<Review.Menu>? = null,
imgUrl: String? = null,
onMoreClick: () -> Unit = {},|
|
||
| @Composable | ||
| fun ReviewItem( | ||
| isWriter: Boolean, |
There was a problem hiding this comment.
내가 쓴 리뷰인지에 대한 UI로직은 아직 없는거죠?
There was a problem hiding this comment.
근데 저 onClickMore로 넘겨서 isWriter 파라미터 값으로 내가 쓴 리뷰인지 구분하지 않아가지구 해당 파라미터 빼도 될 것 같어요
| modifier: Modifier = Modifier, | ||
| viewModel: ModifyViewModel = hiltViewModel(), | ||
| reviewId: Long, | ||
| initialRating: Int = 0, | ||
| initialContent: String = "", | ||
| menuList: List<Review.Menu> = emptyList(), | ||
| onBack: () -> Unit = {}, |
There was a problem hiding this comment.
reviewId: Long,
modifier: Modifier = Modifier,
viewModel: ModifyViewModel = hiltViewModel(),
initialRating: Int = 0,
initialContent: String = "",
menuList: List<Review.Menu> = emptyList(),
onBack: () -> Unit = {},There was a problem hiding this comment.
생각해보니 initialRating은 없을 수가 없어서 이렇게 바꿨어염~
reviewId: Long,
initialRating: Int,
modifier: Modifier = Modifier,
viewModel: ModifyViewModel = hiltViewModel(),
initialContent: String = "",
menuLikeInfoList: List<Review.MenuLikeInfo> = emptyList(),
| /** Route */ | ||
| @Composable | ||
| fun WriteReviewScreen( |
There was a problem hiding this comment.
어차피 오버로딩때문에 상관은 없지만 이 컴포저블 함수를 WriteReviewRoute로 명명해도 좋겠어용
이건 어떻게 할건지 논의해서 컨벤션으로 정해야 할거같네요
| "메뉴에 대한 상세한 리뷰를 작성해주세요", | ||
| style = EatssuTheme.typography.body2 |
There was a problem hiding this comment.
placeholder 색 지정 빠졌어요
color = Gray400
WriteReviewScreen 도 색 지정 빠져있어요
# Conflicts: # app/src/main/java/com/eatssu/android/data/remote/dto/request/WriteMealReviewRequest.kt # app/src/main/java/com/eatssu/android/data/remote/dto/request/WriteMenuReviewRequest.kt # app/src/main/java/com/eatssu/android/data/remote/dto/request/WriteReviewRequest.kt # app/src/main/java/com/eatssu/android/data/remote/dto/response/MealReviewListResponse.kt # app/src/main/java/com/eatssu/android/data/remote/dto/response/MenuResponse.kt # app/src/main/java/com/eatssu/android/data/remote/dto/response/MenuReviewListResponse.kt # app/src/main/java/com/eatssu/android/data/remote/dto/response/MyReviewListResponse.kt # app/src/main/java/com/eatssu/android/data/remote/dto/response/MyReviewResponse.kt # app/src/main/java/com/eatssu/android/data/remote/dto/response/ReviewListResponse.kt # app/src/main/java/com/eatssu/android/data/remote/service/ReviewService.kt # app/src/main/java/com/eatssu/android/data/repository/MealRepositoryImpl.kt # app/src/main/java/com/eatssu/android/data/repository/ReviewRepositoryImpl.kt # app/src/main/java/com/eatssu/android/data/repository/UserRepositoryImpl.kt # app/src/main/java/com/eatssu/android/data/service/MealService.kt # app/src/main/java/com/eatssu/android/data/service/UserService.kt # app/src/main/java/com/eatssu/android/di/ServiceModule.kt # app/src/main/java/com/eatssu/android/domain/repository/MealRepository.kt # app/src/main/java/com/eatssu/android/domain/repository/ReviewRepository.kt # app/src/main/java/com/eatssu/android/domain/repository/UserRepository.kt # app/src/main/java/com/eatssu/android/domain/usecase/menu/GetMenuNameListOfMealUseCase.kt # app/src/main/java/com/eatssu/android/domain/usecase/review/DeleteReviewUseCase.kt # app/src/main/java/com/eatssu/android/domain/usecase/review/GetImageUrlUseCase.kt # app/src/main/java/com/eatssu/android/domain/usecase/review/GetMealReviewInfoUseCase.kt # app/src/main/java/com/eatssu/android/domain/usecase/review/GetMealReviewListUseCase.kt # app/src/main/java/com/eatssu/android/domain/usecase/review/GetMenuReviewInfoUseCase.kt # app/src/main/java/com/eatssu/android/domain/usecase/review/GetMenuReviewListUseCase.kt # app/src/main/java/com/eatssu/android/domain/usecase/review/GetMyReviewsUseCase.kt # app/src/main/java/com/eatssu/android/domain/usecase/review/ModifyReviewUseCase.kt # app/src/main/java/com/eatssu/android/domain/usecase/review/WriteReviewUseCase.kt # app/src/main/java/com/eatssu/android/presentation/cafeteria/menu/MenuFragment.kt # app/src/main/java/com/eatssu/android/presentation/cafeteria/menu/MenuViewModel.kt # app/src/main/java/com/eatssu/android/presentation/cafeteria/review/list/ReviewViewModel.kt # app/src/main/java/com/eatssu/android/presentation/cafeteria/review/modify/ModifyReviewActivity.kt # app/src/main/java/com/eatssu/android/presentation/cafeteria/review/modify/ModifyViewModel.kt # app/src/main/java/com/eatssu/android/presentation/cafeteria/review/write/ReviewWriteRateActivity.kt # app/src/main/java/com/eatssu/android/presentation/cafeteria/review/write/ReviewWriteViewModel.kt # app/src/main/java/com/eatssu/android/presentation/cafeteria/review/write/menu/VariableMenuViewModel.kt # app/src/main/java/com/eatssu/android/presentation/common/MyReviewBottomSheetFragment.kt # app/src/main/java/com/eatssu/android/presentation/intro/IntroActivity.kt # app/src/main/java/com/eatssu/android/presentation/intro/IntroViewModel.kt # app/src/main/java/com/eatssu/android/presentation/mypage/myreview/MyReviewAdapter.kt # app/src/main/java/com/eatssu/android/presentation/mypage/myreview/MyReviewViewModel.kt
| @@ -0,0 +1,6 @@ | |||
| package com.eatssu.android.domain.model | |||
|
|
|||
| sealed class Result { | |||
There was a problem hiding this comment.
WriteReviewViewModel과 WriteReviewUseCase에서 사용하는 Result 타입에 대해서 이야기해보아야할 것 같아요!
저는 진님께서 작성해주신 코드처럼 리뷰 작성에 성공했다, 실패했다를 Caller에게 전달하려는 목적이라면 간단하게 boolean를 쓰는게 좋다고 생각해요.
물론 sealed class와 Result라는 타입을 토입함으로써 부가적인 message, 나아가 오류 대응에 필요한 비즈니스 로직 관련 데이터를 같이 담을 수 있지만 현재 케이스에는 불필요하다고 생각해요!
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| Result.Failure(e.message ?: "리뷰 작성에 실패했습니다.") |
There was a problem hiding this comment.
비슷한 맥락으로 타 use-case에서 오류 대응 없이 caller에게 오류 대응을 runCatching이든 try-catch를 사용하게끔 하는 것처럼 이 catch 블럭을 아예 없애도 될 것 같아요.
복잡한 여러 오류 케이스가 발생할 수 있는 상황이라면 GetTodayMealUseCase.kt의 MealException처럼(지금은 사용하지 않는 클래스인 것 같네요) 발생할 수 있는 오류에 대한 sealed class를 정의해서 그걸 use-case에서 리턴하는게 좋다고 생각합니다!
| class ReviewComposeActivity : ComponentActivity() { | ||
|
|
||
| private lateinit var menuType: String | ||
| private var itemId by Delegates.notNull<Long>() |
There was a problem hiding this comment.
이 부분 제외하고도 Intent 값을 가져오는 것 때문에 Activity, Fragment 내부에 선언되어있는 var, lateinit var 등은 추후에 제가 올릴 type-safe intent 구현하면서 개선할 수 있을 것 같아요!
| fun provideMealService(retrofit: Retrofit): MealService { | ||
| return retrofit.create(MealService::class.java) |
There was a problem hiding this comment.
저도 이 부분 수정된 것 보고 의아했어요. 확인 부탁드립니다!
Summary
Compose로 새로 구현
XML -> Compose 연결 부분이 3군데 있습니다.
Describe your changes
Screen_recording_20251003_172017.webm
Issue
To reviewers
ApiResult sealed class를 도입하려다.. 아직 식단 쪽 서버 호출 로직이 레거시라서 한번에 도입이 안되어서 냅뒀습니다
내 리뷰에서 닉네임이 api에서 오지 않아서 로컬값을 넣는데 요 부분 더 나은 방법 있으면 말씀해주시면 감사하겠습니다!