Skip to content

[FIX] 코스 발견 / 내가 그린 코스 업로드 #323

Merged
leeeha merged 11 commits intodevelopfrom
feature/fix-discover-course-upload-pick
Feb 14, 2024
Merged

[FIX] 코스 발견 / 내가 그린 코스 업로드 #323
leeeha merged 11 commits intodevelopfrom
feature/fix-discover-course-upload-pick

Conversation

@leeeha
Copy link
Copy Markdown
Member

@leeeha leeeha commented Feb 10, 2024

📌 개요

✨ 작업 내용

  • UiStateV2 사용
  • BaseResponse 사용
  • 리사이클러뷰 감싸고 있는 네스티드 스크롤 뷰 삭제
  • 아이템 클릭 리스너: 인터페이스 대신에 고차함수 사용
  • 데이터 클래스 자체를 번들 데이터로 넘기도록 (Parcelable)
  • 한번 올린 코스는 재업로드 불가능하다는 문구 추가
  • 리스트가 비어있는 경우에 대한 처리

✨ PR 포인트

📸 스크린샷/동영상

스크린샷 2024-02-10 오후 5 38 07 스크린샷 2024-02-10 오후 5 39 21

@leeeha leeeha added 하은 🐰 하은 담당 FIX 💥 버그 및 오류 해결 REFACTOR 🧹 코드 리팩토링 labels Feb 10, 2024
@leeeha leeeha self-assigned this Feb 10, 2024
Copy link
Copy Markdown
Collaborator

@unam98 unam98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니다~!

return remoteCourseDataSource.getMyCourseLoad().data.privateCourses.map { it.toData() }
.toMutableList()
}
override suspend fun getMyCourseLoad(): Result<List<DiscoverUploadCourse>?> =
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

통신 응답 받을 때 Result로 받을 수도 있고 Response로도 받을 수 있는데 Result로 받으신 이유가 있나요?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기존 코드를 보니까 RepositoryImpl 클래스에서 Response 타입으로 응답을 받더라도, 뷰모델에서 runCatching 블럭 이용하여 Result 타입으로 결과 처리 하더라구요! 저는 그 부분을 뷰모델 말고 RepositoryImpl 에서 미리 구현했다고 보시면 될 거 같습니다!

Copy link
Copy Markdown
Member Author

@leeeha leeeha Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예전 프로젝트에서 Response 타입으로 응답을 받으면 isSuccessful 속성으로 성공, 실패를 구분하여 처리했는데, Result 타입을 이용하면 onSuccess, onFailure 이렇게 블록을 나눠서 처리할 수 있다는 게 가독성 면에서 좋다고 생각했습니다! 그리고 getOrNull(), getOfThrow() 등 유용한 확장함수들도 제공한다는 게 좋은 거 같아요.

Comment on lines +6 to +12
@Parcelize
data class DiscoverUploadCourse(
val id: Int = -1,
val imageUrl: String = "",
val departure: String = "",
val distance: String = ""
) : Parcelable
Copy link
Copy Markdown
Collaborator

@unam98 unam98 Feb 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기에 기본값을 주신 것은 null 처리 목적일까요?
만약 그렇다면 변수들의 타입을 nullable하게 바꿔주시는 게 어떠실까요? 이 기본값들이 어떤 목적으로 할당된 것인지 의도 전달/파악에 더 도움이 될 것 같습니다.

그리고 최근에 개인적으로 null 처리를 할 때 기준을 가지고 하면 좋을 것 같다는 생각이 들었습니다.
왜냐하면 나중에 null 처리가 제대로 되어있나 확인하려면 연결돼있는 logic들을 다 찾아봐야 해서 까다롭더라구요

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

responseDTO에서는 웬만하면 다 타입을 nullable하게 맞추고 기본값 지정 등 null 처리는 mapper에서 하는 것이 어떨까요?

이게 항상 정답은 아니지만, 이렇게 기준을 정해놓으면 나중에 "여기는 null 처리가 어떻게 돼있지?" 확인하고 싶을 때 mapper부터 보면 되니까 덜 헤매도 돼서 좋다는 생각이 들었습니다. 그리고 이렇게 하면 mapper도 단순히 responseDTO에서 내가 원하는 property들만 빼내는 역할이 아니라 null 처리의 역할도 수행하게 함으로써 굳이 mapping이라는 중간 절차를 추가한 것에서 효용을 더 얻어낼 수 있는 방향이라는 생각도 들었습니다.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗 여기서 디폴트 값을 준 것은 기존 코드와 비슷하게 로직을 유지하다가 그랬던 거 같아요!
현재는 불필요한 것으로 판단되어 수정하도록 하겠습니다!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nullable한 속성에 대한 널처리는 mapper 함수에서 하는 거 좋은 거 같습니다! 일관성있게 기준을 잡는 습관 들여야겠어요 👍

Comment on lines 3 to 5
interface OnCourseUploadItemClick {
fun selectCourse(id:Int, img: String, departure: String,distance:String)
fun selectCourse(id:Int, img: String, departure: String, distance:String)
} No newline at end of file
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사용하지 않는 파일 삭제해주시면 좋을 것 같습니다!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인 감사드려요!

Copy link
Copy Markdown
Member

@dongx0915 dongx0915 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다~

DiscoverUploadCourse(
id = course.id,
imageUrl = course.image,
departure = course.departure.region + ' ' + course.departure.city,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kotlin Performance Tuning: 20 Best Practices You Should Know

이전에 본 글인데 문자열 연결 연산자를 사용하지 않는게 성능에 도움이 된다는 글이 있었습니다.

아주 사소한 부분이지만 알아두면 좋을 것 같아요~!

Copy link
Copy Markdown
Member Author

@leeeha leeeha Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오호! 이런 디테일 부분에서도 성능을 고려하는 습관을 들이는 것이 좋겠다는 생각이 드네요! 글 추천 감사합니다 :)

@leeeha leeeha merged commit 2df094d into develop Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

FIX 💥 버그 및 오류 해결 REFACTOR 🧹 코드 리팩토링 하은 🐰 하은 담당

Projects

Development

Successfully merging this pull request may close these issues.

[REFACTOR] 코스 발견 / 내가 그린 코스 업로드 리팩토링

3 participants