[Chore] Change delete/modify/report modal #248
Conversation
Walkthrough이 변경 사항은 AndroidManifest.xml 파일에서 활동 이름을 Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (14)
app/src/main/java/com/eatssu/android/presentation/review/list/ReviewViewModel.kt (1)
Line range hint
256-265: 상태 관리 패턴 개선을 제안합니다.현재 상태 관리가 데이터 클래스로 구현되어 있지만, sealed class를 사용하면 더 안전하고 명확한 상태 관리가 가능합니다.
다음과 같은 구조로 변경하는 것을 고려해보세요:
sealed class ReviewState { object Loading : ReviewState() object Empty : ReviewState() data class Error(val message: String) : ReviewState() data class Success( val reviewInfo: ReviewInfo? = null, val reviewList: List<Review>? = null ) : ReviewState() }이렇게 변경하면:
- 상태 간 전환이 더 명확해집니다
- 상태별 처리가 when 식으로 강제되어 안전성이 향상됩니다
- 불가능한 상태 조합을 컴파일 시점에 방지할 수 있습니다
app/src/main/res/layout/fragment_bottomsheet_others.xml (1)
21-27: 문자열 리소스 사용 권장하드코딩된 텍스트 "리뷰 관리"를 strings.xml로 이동하는 것이 좋습니다.
다음과 같이 수정해주세요:
<TextView style="@style/Subtitle2" android:layout_width="wrap_content" android:layout_height="wrap_content" android:layout_marginBottom="22dp" - android:text="리뷰 관리" + android:text="@string/title_review_management" android:textColor="@color/black" />app/src/main/java/com/eatssu/android/presentation/common/OthersBottomSheetFragment.kt (2)
39-39: 로깅 메시지 영문화 필요국제화 및 일관성을 위해 로깅 메시지를 영어로 작성해주세요.
- Timber.d("넘겨받은 리뷰 정보: $reviewId $menu") + Timber.d("Received review info: reviewId=$reviewId, menu=$menu")
41-48: 리포트 화면 전환 시 로딩 상태 처리 필요ReportActivity로 전환하는 동안 사용자에게 로딩 상태를 표시하는 것이 좋습니다. 또한 화면 전환 실패 시의 예외 처리도 추가해주세요.
예시 구현:
try { binding.progressBar.isVisible = true val intent = Intent(requireContext(), ReportActivity::class.java) intent.putExtra("reviewId", reviewId) startActivity(intent) } catch (e: Exception) { Timber.e(e, "Failed to start ReportActivity") showErrorMessage() } finally { binding.progressBar.isVisible = false dismiss() }app/src/main/java/com/eatssu/android/presentation/mypage/myreview/MyReviewAdapter.kt (2)
41-45: 이미지 로딩 조건문 간소화
data.imgUrl?.isEmpty() == true || data.imgUrl?.get(0).isNullOrEmpty()조건문이 복잡합니다. Kotlin의 함수를 활용하여 가독성을 높일 수 있습니다.다음과 같이 수정하면 가독성이 향상됩니다:
if (data.imgUrl.isNullOrEmpty() || data.imgUrl[0].isNullOrEmpty()) { imageView.visibility = View.GONE } else { Glide.with(itemView) .load(data.imgUrl[0]) .into(imageView) imageView.visibility = View.VISIBLE }
55-57: Timber 로그의 불필요한 문자열 포매팅
Timber.d에서 가변인자를 사용하고 있으나 문자열 내에 포맷 지정자가 없습니다. 이는 불필요한 리소스 소모를 일으킬 수 있습니다.다음과 같이 수정할 수 있습니다:
Timber.d("리뷰 상세 정보 - ID: %d, 메뉴: %s, 내용: %s", data.reviewId, data.menu, data.content)또는 문자열 보간법을 사용할 수 있습니다:
Timber.d("리뷰 상세 정보 - ID: ${data.reviewId}, 메뉴: ${data.menu}, 내용: ${data.content}")app/src/main/java/com/eatssu/android/presentation/review/list/ReviewAdapter.kt (1)
Line range hint
45-49: 이미지 URL 체크 로직 간소화 가능이미지 URL이 비어 있는지 확인하는 로직이 복잡합니다. 가독성을 위해 로직을 간소화할 수 있습니다.
다음과 같이 수정하면 가독성이 향상됩니다:
if (data.imgUrl.isNullOrEmpty() || data.imgUrl[0].isNullOrEmpty()) { binding.ivReviewPhoto.visibility = View.GONE binding.cvPhotoReview.visibility = View.GONE } else { Glide.with(itemView) .load(data.imgUrl[0]) .into(binding.ivReviewPhoto) binding.ivReviewPhoto.visibility = View.VISIBLE binding.cvPhotoReview.visibility = View.VISIBLE }app/src/main/java/com/eatssu/android/presentation/mypage/myreview/MyReviewListActivity.kt (1)
41-47: 익명 객체를 통한 클릭 리스너 설정
OnItemClickListener를 설정하여 클릭 이벤트를 처리하고 있습니다. 그러나 클릭 리스너 설정 부분이 다소 복잡해 보입니다.람다식을 활용하여 코드를 간결하게 만들 수 있습니다:
adapter.setOnItemClickListener { view, reviewData -> onMyReviewClicked(reviewData) }이를 위해
setOnItemClickListener메서드와 인터페이스를 수정하여 람다식을 받을 수 있도록 변경하면 가독성이 향상됩니다.app/src/main/java/com/eatssu/android/presentation/common/MyReviewBottomSheetFragment.kt (2)
23-33: 아키텍처 개선 제안프래그먼트의 구조는 잘 설계되어 있으나, 다음과 같은 개선사항을 고려해보세요:
- 콜백 인터페이스를 별도의 파일로 분리하여 재사용성 향상
- sealed class를 사용하여 이벤트 처리를 더 타입 안전하게 구현
66-80: 데이터 전달 방식 개선 필요Intent를 통한 데이터 전달 방식을 개선하면 좋을 것 같습니다:
- Parcelable 객체를 사용하여 데이터 전달
- 키 상수를 companion object로 분리
+ companion object { + private const val KEY_REVIEW_ID = "reviewId" + private const val KEY_MENU = "menu" + private const val KEY_CONTENT = "content" + private const val KEY_MAIN_GRADE = "mainGrade" + private const val KEY_AMOUNT_GRADE = "amountGrade" + private const val KEY_TASTE_GRADE = "tasteGrade" + }app/src/main/AndroidManifest.xml (1)
Line range hint
173-179: 매니페스트 설정 오류프래그먼트를 액티비티로 선언하는 것은 잘못된 설정입니다:
- BottomSheetFragment는 매니페스트에 선언하지 않아야 함
- 해당 항목을 제거하고 호스트 액티비티에서 프래그먼트를 관리해야 함
- <activity - android:name=".presentation.common.MyReviewBottomSheetFragment" - android:exported="true" - android:theme="@style/Theme.MyDialog"> - <meta-data - android:name="android.app.lib_name" - android:value="" /> - </activity>app/src/main/res/layout/activity_fix_menu.xml (1)
50-53: RatingBar 접근성 개선 필요RatingBar 스타일링이 개선되었으나, 다음 사항들을 고려해보세요:
- contentDescription 속성 추가로 접근성 개선
- 평점 변경 시 TalkBack 피드백 제공
<RatingBar android:id="@+id/rb_main" style="@style/Widget.AppCompat.RatingBar" + android:contentDescription="@string/rating_main_description" android:layout_width="wrap_content" android:layout_height="wrap_content"Also applies to: 108-111, 143-146
app/src/main/java/com/eatssu/android/presentation/review/list/ReviewActivity.kt (2)
122-144: RecyclerView 어댑터를 재사용하여 성능 향상현재
setAdapter()메서드에서 매번 새로운ReviewAdapter인스턴스를 생성하고 있습니다. 어댑터를 클래스 수준에서 선언하고 재사용하면 성능 및 메모리 효율성을 향상시킬 수 있습니다.다음과 같이 어댑터를 클래스 변수로 선언하고 초기화하십시오:
+ private lateinit var adapter: ReviewAdapter override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) // 기존 코드 + adapter = ReviewAdapter() + binding.rvReview.adapter = adapter + binding.rvReview.layoutManager = LinearLayoutManager(this) + adapter.setOnItemClickListener(object : ReviewAdapter.OnItemClickListener { + override fun onMyReviewClicked(view: View, reviewData: Review) { + onMyReviewClicked(review = reviewData) + } + override fun onOthersReviewClicked(view: View, reviewData: Review) { + onOthersReviewClicked(reviewData = reviewData) + } + }) } private fun setAdapter(reviewList: List<Review>) { - val adapter = ReviewAdapter() adapter.submitList(reviewList) - // 레이아웃 매니저 및 클릭 리스너 설정 코드 이동 }
175-209: 중복 코드 리팩토링으로 코드 유지보수성 향상
onMyReviewClicked()와onOthersReviewClicked()메서드에서 유사한 코드를 사용하고 있습니다. 공통 기능을 별도의 메서드로 추출하여 코드 중복을 줄이고 유지보수성을 높일 수 있습니다.다음과 같이 공통 메서드를 생성할 수 있습니다:
private fun showBottomSheet( fragment: DialogFragment, bundle: Bundle ) { fragment.arguments = bundle fragment.setStyle( DialogFragment.STYLE_NORMAL, R.style.RoundCornerBottomSheetDialogTheme ) fragment.show(supportFragmentManager, "Open Bottom Sheet") }각 메서드에서는 다음과 같이 호출합니다:
fun onMyReviewClicked(review: Review) { val bundle = Bundle().apply { putLong("reviewId", review.reviewId) putString("menu", review.menu) putString("content", review.content) putInt("mainGrade", review.mainGrade) putInt("amountGrade", review.amountGrade) putInt("tasteGrade", review.tasteGrade) } val modalBottomSheet = MyReviewBottomSheetFragment().apply { arguments = bundle onReviewDeletedListener = this@ReviewActivity } showBottomSheet(modalBottomSheet, bundle) } fun onOthersReviewClicked(reviewData: Review) { val bundle = Bundle().apply { putLong("reviewId", reviewData.reviewId) putString("menu", reviewData.menu) } val modalBottomSheet = OthersBottomSheetFragment() showBottomSheet(modalBottomSheet, bundle) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
app/src/main/AndroidManifest.xml(1 hunks)app/src/main/java/com/eatssu/android/presentation/common/MyReviewBottomSheetFragment.kt(1 hunks)app/src/main/java/com/eatssu/android/presentation/common/OthersBottomSheetFragment.kt(1 hunks)app/src/main/java/com/eatssu/android/presentation/mypage/myreview/MyReviewAdapter.kt(2 hunks)app/src/main/java/com/eatssu/android/presentation/mypage/myreview/MyReviewListActivity.kt(3 hunks)app/src/main/java/com/eatssu/android/presentation/mypage/myreview/MyReviewViewModel.kt(4 hunks)app/src/main/java/com/eatssu/android/presentation/review/delete/DeleteViewModel.kt(0 hunks)app/src/main/java/com/eatssu/android/presentation/review/delete/MyReviewDialogActivity.kt(0 hunks)app/src/main/java/com/eatssu/android/presentation/review/list/ReviewActivity.kt(5 hunks)app/src/main/java/com/eatssu/android/presentation/review/list/ReviewAdapter.kt(2 hunks)app/src/main/java/com/eatssu/android/presentation/review/list/ReviewViewModel.kt(3 hunks)app/src/main/java/com/eatssu/android/presentation/review/report/OthersReviewDialogActivity.kt(0 hunks)app/src/main/res/drawable/ic_pencil.xml(1 hunks)app/src/main/res/drawable/ic_remove.xml(1 hunks)app/src/main/res/layout/activity_fix_menu.xml(3 hunks)app/src/main/res/layout/activity_my_review_dialog.xml(0 hunks)app/src/main/res/layout/fragment_bottomsheet_my_review.xml(1 hunks)app/src/main/res/layout/fragment_bottomsheet_others.xml(1 hunks)
💤 Files with no reviewable changes (4)
- app/src/main/res/layout/activity_my_review_dialog.xml
- app/src/main/java/com/eatssu/android/presentation/review/report/OthersReviewDialogActivity.kt
- app/src/main/java/com/eatssu/android/presentation/review/delete/MyReviewDialogActivity.kt
- app/src/main/java/com/eatssu/android/presentation/review/delete/DeleteViewModel.kt
✅ Files skipped from review due to trivial changes (2)
- app/src/main/res/drawable/ic_remove.xml
- app/src/main/res/drawable/ic_pencil.xml
🔇 Additional comments (12)
app/src/main/java/com/eatssu/android/presentation/review/list/ReviewViewModel.kt (1)
10-10: 의존성 주입이 올바르게 구현되었습니다!
DeleteReviewUseCase의 의존성 주입이 Hilt 패턴에 맞게 잘 구현되었습니다.
Also applies to: 35-35
app/src/main/res/layout/fragment_bottomsheet_others.xml (1)
2-9: 레이아웃 구성이 적절합니다!
BottomSheetBehavior를 사용한 기본 레이아웃 구성이 잘 되어있습니다.
app/src/main/java/com/eatssu/android/presentation/common/OthersBottomSheetFragment.kt (1)
22-29: 구현이 적절합니다
onCreateView 구현이 안드로이드 모범 사례를 잘 따르고 있습니다.
app/src/main/java/com/eatssu/android/presentation/mypage/myreview/MyReviewAdapter.kt (2)
15-16: ListAdapter로의 변경으로 효율적인 데이터 관리
RecyclerView.Adapter에서 ListAdapter로 변경하여 DiffUtil을 활용한 효율적인 데이터 변경 처리가 가능해졌습니다. 이는 성능 향상과 코드 간소화에 도움이 됩니다.
18-20: OnItemClickListener 인터페이스 추가로 클릭 이벤트 처리 개선
클릭 이벤트 처리를 위한 OnItemClickListener 인터페이스를 도입하여 코드의 결합도를 낮추고 재사용성을 높였습니다.
app/src/main/java/com/eatssu/android/presentation/review/list/ReviewAdapter.kt (2)
14-16: ListAdapter 도입으로 성능 개선
RecyclerView.Adapter에서 ListAdapter로 변경하여 리스트 업데이트 시 DiffUtil을 활용할 수 있게 되었습니다. 이는 성능 향상에 기여합니다.
17-20: OnItemClickListener 인터페이스 추가로 클릭 이벤트 처리 개선
클릭 이벤트 처리를 위한 인터페이스를 도입하여 코드의 유연성과 재사용성을 높였습니다.
app/src/main/res/layout/fragment_bottomsheet_my_review.xml (1)
1-88: 레이아웃 구성 적절
바텀 시트 다이얼로그를 위한 레이아웃이 적절하게 구성되었습니다. 요소들의 배치와 스타일링이 일관성 있게 잘 적용되었습니다.
app/src/main/java/com/eatssu/android/presentation/mypage/myreview/MyReviewListActivity.kt (3)
20-21: MyReviewBottomSheetFragment.OnReviewDeletedListener 구현으로 삭제 이벤트 처리
MyReviewBottomSheetFragment.OnReviewDeletedListener를 구현하여 리뷰 삭제 후 UI 업데이트를 처리할 수 있게 되었습니다.
36-38: 어댑터 생성 후 즉시 submitList 호출
어댑터를 생성하고 데이터를 즉시 설정하는 것으로 보입니다. 데이터가 변경될 때마다 어댑터에 새로운 리스트를 전달하여 UI를 갱신합니다.
81-99: BottomSheetDialogFragment 호출 시 스타일 적용 확인
바텀 시트 다이얼로그 호출 시 스타일을 적용하여 모서리 곡선 등이 제대로 표시되는지 확인하세요.
바텀 시트 다이얼로그의 스타일이 적용되었는지 확인하기 위해 다음 스크립트를 실행하세요:
✅ Verification successful
Let me check the shape drawable definition to ensure the rounded corners are properly defined.
바텀 시트 다이얼로그의 스타일이 올바르게 구성되어 있습니다
스타일 구성이 적절하게 되어 있음을 확인했습니다:
RoundCornerBottomSheetDialogTheme가 themes.xml에 정의되어 있음- 상단 모서리가 30dp로 둥글게 처리되도록 shape_corner_top.xml이 올바르게 구성됨
- 하단 모서리는 0dp로 설정되어 직각 처리됨
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: 스타일 리소스 파일에서 'RoundCornerBottomSheetDialogTheme' 정의 여부 확인
# 예상 결과: 스타일이 정의되어 있어야 합니다.
rg 'RoundCornerBottomSheetDialogTheme' -A 5 -B 2 app/src/main/res/values/
Length of output: 725
Script:
#!/bin/bash
# Description: Check the shape_corner_top drawable definition
rg -g "shape_corner_top.xml" . --files-with-matches
cat app/src/main/res/drawable/shape_corner_top.xml
Length of output: 510
app/src/main/java/com/eatssu/android/presentation/mypage/myreview/MyReviewViewModel.kt (1)
103-103: MyReviewState 데이터 클래스의 isDeleted 초기화 위치 확인 필요
isDeleted 변수를 사용하고 있으나, 이 값이 삭제 이후에 적절히 초기화되는지 확인해야 합니다. 그렇지 않으면 상태가 유지되어 예상치 못한 동작을 유발할 수 있습니다.
isDeleted 변수가 삭제 처리 후에 초기화되는지 확인하기 위해 코드를 검토하세요.
| fun deleteReview(reviewId: Long) { | ||
| viewModelScope.launch { | ||
| deleteReviewUseCase(reviewId).onStart { | ||
| _uiState.update { it.copy(loading = true) } | ||
| }.onCompletion { | ||
| _uiState.update { it.copy(loading = false, error = true) } | ||
| }.catch { e -> | ||
| _uiState.update { | ||
| it.copy( | ||
| error = true, | ||
| // toastMessage = context.getString(R.string.delete_not) | ||
| ) | ||
| } | ||
| Timber.e(e.toString()) | ||
| }.collectLatest { result -> | ||
| Timber.d(result.toString()) | ||
|
|
||
| _uiState.update { | ||
| it.copy( | ||
| // isDeleted = true, | ||
| // toastMessage = context.getString(R.string.delete_done) | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
오류 처리 로직 개선이 필요합니다.
다음과 같은 문제점들이 발견되었습니다:
onCompletion블록에서 항상error = true로 설정되어 있어, 성공적인 삭제 시에도 오류 상태로 표시됩니다.- 성공 케이스에서 error 상태를 초기화하지 않습니다.
- 주석 처리된 토스트 메시지 코드는 사용자 피드백이 누락되었음을 의미합니다.
다음과 같이 수정하는 것을 제안합니다:
fun deleteReview(reviewId: Long) {
viewModelScope.launch {
deleteReviewUseCase(reviewId).onStart {
_uiState.update { it.copy(loading = true) }
- }.onCompletion {
- _uiState.update { it.copy(loading = false, error = true) }
+ }.onCompletion {
+ _uiState.update { it.copy(loading = false) }
}.catch { e ->
_uiState.update {
it.copy(
+ loading = false,
error = true,
)
}
Timber.e(e.toString())
}.collectLatest { result ->
Timber.d(result.toString())
_uiState.update {
it.copy(
+ error = false,
// TODO: 사용자 피드백 구현
)
}
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun deleteReview(reviewId: Long) { | |
| viewModelScope.launch { | |
| deleteReviewUseCase(reviewId).onStart { | |
| _uiState.update { it.copy(loading = true) } | |
| }.onCompletion { | |
| _uiState.update { it.copy(loading = false, error = true) } | |
| }.catch { e -> | |
| _uiState.update { | |
| it.copy( | |
| error = true, | |
| // toastMessage = context.getString(R.string.delete_not) | |
| ) | |
| } | |
| Timber.e(e.toString()) | |
| }.collectLatest { result -> | |
| Timber.d(result.toString()) | |
| _uiState.update { | |
| it.copy( | |
| // isDeleted = true, | |
| // toastMessage = context.getString(R.string.delete_done) | |
| ) | |
| } | |
| } | |
| } | |
| } | |
| fun deleteReview(reviewId: Long) { | |
| viewModelScope.launch { | |
| deleteReviewUseCase(reviewId).onStart { | |
| _uiState.update { it.copy(loading = true) } | |
| }.onCompletion { | |
| _uiState.update { it.copy(loading = false) } | |
| }.catch { e -> | |
| _uiState.update { | |
| it.copy( | |
| loading = false, | |
| error = true, | |
| ) | |
| } | |
| Timber.e(e.toString()) | |
| }.collectLatest { result -> | |
| Timber.d(result.toString()) | |
| _uiState.update { | |
| it.copy( | |
| error = false, | |
| // TODO: 사용자 피드백 구현 | |
| ) | |
| } | |
| } | |
| } | |
| } |
| <LinearLayout | ||
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent" | ||
| android:gravity="center" | ||
| android:orientation="vertical" | ||
| android:paddingBottom="24dp" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintTop_toTopOf="parent" | ||
| tools:ignore="MissingConstraints"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
불필요한 제약 조건 제거 필요
LinearLayout 내부에서 ConstraintLayout 관련 속성(layout_constraintStart_toStartOf, layout_constraintTop_toTopOf)과 tools:ignore="MissingConstraints"는 불필요합니다.
다음과 같이 수정해주세요:
<LinearLayout
android:layout_width="match_parent"
android:layout_height="match_parent"
android:gravity="center"
android:orientation="vertical"
- android:paddingBottom="24dp"
- app:layout_constraintStart_toStartOf="parent"
- app:layout_constraintTop_toTopOf="parent"
- tools:ignore="MissingConstraints">
+ android:paddingBottom="24dp">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <LinearLayout | |
| android:layout_width="match_parent" | |
| android:layout_height="match_parent" | |
| android:gravity="center" | |
| android:orientation="vertical" | |
| android:paddingBottom="24dp" | |
| app:layout_constraintStart_toStartOf="parent" | |
| app:layout_constraintTop_toTopOf="parent" | |
| tools:ignore="MissingConstraints"> | |
| <LinearLayout | |
| android:layout_width="match_parent" | |
| android:layout_height="match_parent" | |
| android:gravity="center" | |
| android:orientation="vertical" | |
| android:paddingBottom="24dp"> |
| <LinearLayout | ||
| android:id="@+id/ll_report" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:orientation="horizontal"> | ||
|
|
||
| <ImageView | ||
| android:layout_width="12dp" | ||
| android:layout_height="12dp" | ||
| android:layout_gravity="center" | ||
| android:src="@drawable/ic_pencil" /> | ||
|
|
||
| <TextView | ||
| style="@style/Body1" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginStart="20dp" | ||
| android:paddingVertical="14dp" | ||
| android:text="신고하기" | ||
| android:textColor="@color/black" /> | ||
|
|
||
| <View | ||
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent" | ||
| android:layout_weight="1" /> | ||
|
|
||
| </LinearLayout> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
접근성 및 유지보수성 개선 필요
신고하기 옵션 섹션에 대한 몇 가지 개선사항이 있습니다:
다음 사항들을 수정해주세요:
- 접근성을 위한 contentDescription 추가
- 하드코딩된 문자열을 strings.xml로 이동
- 고정된 크기값을 dimens.xml로 이동
- paddingVertical 대신 android:padding 사용 (하위 버전 호환성)
<LinearLayout
android:id="@+id/ll_report"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
+ android:contentDescription="@string/desc_report_button"
android:orientation="horizontal">
<ImageView
- android:layout_width="12dp"
- android:layout_height="12dp"
+ android:layout_width="@dimen/icon_size_small"
+ android:layout_height="@dimen/icon_size_small"
android:layout_gravity="center"
+ android:contentDescription="@string/desc_report_icon"
android:src="@drawable/ic_pencil" />
<TextView
style="@style/Body1"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
- android:layout_marginStart="20dp"
+ android:layout_marginStart="@dimen/margin_medium"
- android:paddingVertical="14dp"
+ android:paddingTop="14dp"
+ android:paddingBottom="14dp"
- android:text="신고하기"
+ android:text="@string/action_report"
android:textColor="@color/black" />Committable suggestion skipped: line range outside the PR's diff.
| private var _binding: FragmentBottomsheetOthersBinding? = null | ||
| private val binding get() = _binding!! | ||
|
|
||
| var reviewId = -1L | ||
| var menu = "" | ||
|
|
There was a problem hiding this comment.
메모리 누수 방지를 위한 바인딩 정리 필요
바인딩 객체가 프래그먼트 수명주기 이후에도 유지될 수 있어 메모리 누수가 발생할 수 있습니다. onDestroyView()에서 바인딩을 정리해주세요.
다음과 같이 수정해주세요:
+ override fun onDestroyView() {
+ super.onDestroyView()
+ _binding = null
+ }또한 reviewId와 menu를 데이터 클래스로 캡슐화하는 것을 고려해보세요.
Committable suggestion skipped: line range outside the PR's diff.
| arguments?.let { | ||
| reviewId = it.getLong("reviewId") | ||
| menu = it.getString("menu").toString() | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
인자 검증 로직 개선 필요
arguments에서 reviewId와 menu를 가져올 때 다음과 같은 문제점들이 발견되었습니다:
- reviewId가 -1L인 경우에 대한 검증이 누락되어 있습니다
- menu는 null 체크 없이 toString()을 호출하고 있어 NPE 발생 가능성이 있습니다
- 로그 메시지가 한글로 작성되어 있어 일관성이 떨어집니다
수정이 필요한 부분:
- arguments?.let 블록 내에서 reviewId와 menu 값 검증 추가
- menu는
it.getString("menu")?.toString() ?: ""형태로 안전한 널 처리 필요 - 영문 로그 메시지로 통일
🔗 Analysis chain
인자 유효성 검사 추가 필요
reviewId가 유효하지 않은 경우(-1)에 대한 처리가 필요합니다. 또한 menu가 null이나 빈 문자열일 경우의 처리도 필요합니다.
Let me check how these reviewId values are handled in the related files.
Let me check how these fragments handle their lifecycle and any error cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any null checks for these arguments in other files
rg "reviewId.*-1|menu.*isEmpty" app/src/main/java/
Length of output: 786
Script:
#!/bin/bash
# Check the implementation of these bottom sheet fragments and activities
ast-grep --pattern 'class $_ : BottomSheetDialogFragment {
$$$
reviewId $$$
$$$
}'
# Also check for menu parameter handling
rg "menu.*toString|getString.*menu" app/src/main/java/
Length of output: 1924
Script:
#!/bin/bash
# Check implementation of these bottom sheet fragments
cat app/src/main/java/com/eatssu/android/presentation/common/OthersBottomSheetFragment.kt
cat app/src/main/java/com/eatssu/android/presentation/common/MyReviewBottomSheetFragment.kt
# Check if there's any error handling in parent classes
rg "class.*BottomSheetDialogFragment" app/src/main/java/
Length of output: 5925
| mOnItemClickListener.onMyReviewClicked(v, data) | ||
| } else { | ||
| showMenu(binding.root.context, v, R.menu.menu_other_review, data) | ||
| mOnItemClickListener.onOthersReviewClicked(v, data) |
There was a problem hiding this comment.
클릭 이벤트 처리 시 NullPointerException 방지 필요
mOnItemClickListener가 초기화되지 않았을 경우 NullPointerException이 발생할 수 있습니다. 이를 방지하기 위해 초기화를 보장하거나 클릭 시 null 체크가 필요합니다.
mOnItemClickListener를 nullable로 선언하고 클릭 시 null 체크를 추가하거나, 초기화를 강제하도록 수정하세요.
| fun deleteReview(reviewId: Long) { | ||
| viewModelScope.launch { | ||
| deleteReviewUseCase(reviewId).onStart { | ||
| _uiState.update { it.copy(loading = true) } | ||
| }.onCompletion { | ||
| _uiState.update { it.copy(loading = false, error = true) } | ||
| }.catch { e -> | ||
| _uiState.update { | ||
| it.copy( | ||
| error = true, | ||
| toastMessage = context.getString(R.string.delete_not) | ||
| ) | ||
| } | ||
| Timber.e(e.toString()) | ||
| }.collectLatest { result -> | ||
| Timber.d(result.toString()) | ||
|
|
||
| _uiState.update { | ||
| it.copy( | ||
| isDeleted = true, | ||
| toastMessage = context.getString(R.string.delete_done) | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
deleteReview 메서드의 에러 처리 로직 개선 필요
onCompletion 블록에서 error = true로 상태를 업데이트하고 있는데, 이는 성공적인 완료 시에도 에러 상태로 설정될 수 있습니다. 에러 처리는 catch 블록에서만 수행하고, onCompletion에서는 로딩 상태만 변경하는 것이 적절합니다.
다음과 같이 수정할 수 있습니다:
.deleteReviewUseCase(reviewId)
.onStart {
_uiState.update { it.copy(loading = true) }
}
.catch { e ->
_uiState.update {
it.copy(
loading = false,
error = true,
toastMessage = context.getString(R.string.delete_not)
)
}
}
.collectLatest { result ->
_uiState.update {
it.copy(
loading = false,
isDeleted = true,
toastMessage = context.getString(R.string.delete_done)
)
}
}이렇게 하면 에러 발생 시에는 catch 블록에서 에러 상태를 처리하고, 성공적으로 완료되면 collectLatest에서 로딩 상태를 해제하며 결과를 처리할 수 있습니다.
| var reviewId = -1L | ||
| var menu = "" | ||
| var content = "" | ||
| var mainGrade = -1 | ||
| var amountGrade = -1 | ||
| var tasteGrade = -1 |
There was a problem hiding this comment.
속성 유효성 검사 추가 필요
리뷰 데이터의 유효성 검사가 누락되어 있습니다:
- 평점(mainGrade, amountGrade, tasteGrade)의 범위 검증 필요 (0-5)
- reviewId의 유효성 검사 추가 필요
- var mainGrade = -1
- var amountGrade = -1
- var tasteGrade = -1
+ var mainGrade = -1
+ set(value) {
+ require(value in -1..5) { "평점은 -1에서 5 사이여야 합니다" }
+ field = value
+ }
+ var amountGrade = -1
+ set(value) {
+ require(value in -1..5) { "평점은 -1에서 5 사이여야 합니다" }
+ field = value
+ }
+ var tasteGrade = -1
+ set(value) {
+ require(value in -1..5) { "평점은 -1에서 5 사이여야 합니다" }
+ field = value
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var reviewId = -1L | |
| var menu = "" | |
| var content = "" | |
| var mainGrade = -1 | |
| var amountGrade = -1 | |
| var tasteGrade = -1 | |
| var reviewId = -1L | |
| var menu = "" | |
| var content = "" | |
| var mainGrade = -1 | |
| set(value) { | |
| require(value in -1..5) { "평점은 -1에서 5 사이여야 합니다" } | |
| field = value | |
| } | |
| var amountGrade = -1 | |
| set(value) { | |
| require(value in -1..5) { "평점은 -1에서 5 사이여야 합니다" } | |
| field = value | |
| } | |
| var tasteGrade = -1 | |
| set(value) { | |
| require(value in -1..5) { "평점은 -1에서 5 사이여야 합니다" } | |
| field = value | |
| } |
| binding.llDelete.setOnClickListener { | ||
| AlertDialog.Builder(requireContext()).apply { | ||
| setTitle(R.string.delete) | ||
| setMessage(R.string.delete_description) | ||
| setNegativeButton("취소") { _, _ -> | ||
| activity?.showToast(App.appContext.getString(R.string.delete_undo)) | ||
| } | ||
| setPositiveButton("삭제") { _, _ -> | ||
| viewModel.deleteReview(reviewId) | ||
| lifecycleScope.launch { | ||
| viewModel.uiState.collectLatest { | ||
| if (it.isDeleted) { | ||
| onReviewDeletedListener?.onReviewDeleted() // 콜백 호출 | ||
| dismiss() | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }.create().show() | ||
| } |
There was a problem hiding this comment.
삭제 로직 개선 필요
다음 사항들을 개선하면 좋을 것 같습니다:
- 다이얼로그 버튼의 문자열을 strings.xml로 이동
- 삭제 실패 시 에러 처리 로직 추가
- 코루틴 스코프 취소 처리 추가
- setNegativeButton("취소") { _, _ ->
- activity?.showToast(App.appContext.getString(R.string.delete_undo))
- }
- setPositiveButton("삭제") { _, _ ->
+ setNegativeButton(R.string.cancel) { _, _ ->
+ activity?.showToast(getString(R.string.delete_undo))
+ }
+ setPositiveButton(R.string.delete) { _, _ ->
viewModel.deleteReview(reviewId)
lifecycleScope.launch {
+ try {
viewModel.uiState.collectLatest {
if (it.isDeleted) {
onReviewDeletedListener?.onReviewDeleted() // 콜백 호출
dismiss()
+ } else if (it.error != null) {
+ activity?.showToast(getString(R.string.delete_error))
}
}
+ } catch (e: Exception) {
+ activity?.showToast(getString(R.string.unknown_error))
+ }
}
}Committable suggestion skipped: line range outside the PR's diff.
|
|
||
|
|
||
| override fun onReviewDeleted() { | ||
| lodeData() |
There was a problem hiding this comment.
'lodeData()' 메서드명 오타 수정 필요
현재 메서드명이 lodeData()로 표기되어 있습니다. 올바른 철자는 loadData()이며, 데이터 로드를 의미하므로 메서드명을 수정해야 합니다.
다음과 같이 수정하십시오:
- lodeData()
+ loadData()메서드 정의부와 해당 메서드를 호출하는 모든 부분에서도 동일하게 수정해야 합니다.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| lodeData() | |
| loadData() |
Summary
Describe your changes
Issue
To reviewers
Summary by CodeRabbit
릴리스 노트
새로운 기능
버그 수정
문서화
스타일
기타