-
Notifications
You must be signed in to change notification settings - Fork 1
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
[BSVR-128/FEAT] 홈, 시야기록 서버 통신 구현 #36
Conversation
; Conflicts: ; app/src/main/java/com/depromeet/spot/di/DataSourceModule.kt ; app/src/main/java/com/depromeet/spot/di/RepositoryModule.kt ; app/src/main/java/com/depromeet/spot/di/ServiceModule.kt
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.
api 연결이랑 코드 짜느라 고생많았어~! 🦾
data/src/main/java/com/depromeet/data/intercepter/AuthInterceptor.kt
Outdated
Show resolved
Hide resolved
companion object { | ||
fun ResponseMySeatRecordDto.toMySeatRecordResponse() = MySeatRecordResponse( |
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.
이렇게 구현한 이유가 동반 객체로 감싸면서 외부 접근이 필요없는 매퍼 함수를 private 으로 정보은닉하기 위한 의도인건가?
그렇다고 하기에는 다른 data class 는 private 을 사용하지 않아서 뭐징!
아니면 어떤 의도로 이렇게 구현했는 지 궁금한 부분! 궁금하다 궁금해.. 👀
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.
우선 companion object로 감싼 이유는mapper를 따로 패키지로 관리하는게 뭔가 한번 더 참고하는 불필요한 행위라고 판단되서 진행을 했어!
동반 객체로 감싸면서 외부 접근이 필요없는 매퍼 함수를 private 으로 정보은닉하기 위한 의도인건가?
맞아! 그런 의도를 가지고 진행을 했어
homeDataSource.getMySeatRecordData(mySeatRecordRequest.toMySeatRecordRequestDto())
.toMySeatRecordResponse()
우리가 서버와의 통신을 response로 전달하는 과정에서 toMySeatREcordResponse()인 mapper를 제외하고는 따로 접근을 허용을 하는 것이 불필요하다고 판단되서 우선 이렇게 진행해봤어!
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.
그렇다면 data class 마다 매핑 함수를 별도로 두는 것에 대해서는 어떻게 생각해!!?
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.
ㅎㅎ 아뉭.. 나도 민주꺼 보고 저거 좋아보이는데? 했다고 형꺼 보고 형꺼도 좋아보이는데? 해서 물어봤어 ㅋㅋㅋㅋ
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.
앗 그렇다면 나는 뭔가 데이터는 데이터 mapper는 mapper로 따로 한번에 보는게 보기 편해보여서 이런 방식으로 하긴 했는데 관희껏도 리뷰쪽 data class마다 매핑함수 하면 조금 어지러울거 같은 기분..?ㅋㅋㅋㅋㅋㅋㅋㅋ
presentation/src/main/java/com/depromeet/presentation/home/HomeActivity.kt
Outdated
Show resolved
Hide resolved
private val _events = MutableSharedFlow<ProfileEvents>( | ||
replay = 0, | ||
extraBufferCapacity = 1, | ||
onBufferOverflow = BufferOverflow.DROP_OLDEST | ||
) | ||
val events: SharedFlow<ProfileEvents> = _events.asSharedFlow() |
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.
sharedFlow 사용한 이유가 있을까?? 🤔
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.
음 그때 성식과 이야기를 나눴을 때 전체의 이벤트를 하나의 sharedflow로 관리하면 어떨까 싶어서 처음에 설계를 하고 잠시 킵해둔 상황입니다..😂
fun uploadProfileEdit() { | ||
viewModelScope.launch { | ||
if ((profileImageJob?.isActive == true) || (profilePresignedJob?.isActive == true)) { | ||
profileImageJob?.join() |
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.
코루틴을 job으로 처리한 이유가 있을까? 요렇게 설계한 이유가 궁금해! 😮
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.
앞서 토의했던 내용을 이야기 해보자면
프로필 수정 버튼 클릭시 :
presigned url 받아오기 -> 이미지 업로드 -> 프로필수정 를 한번에 진행을 했었다고 이야기를 했었잖아?
근데 이번에는
프로필 이미지 선택시 : presignedurl 받아오기 -> 이미지 업로드
프로필 수정 버튼 클릭시 : 프로필 수정 서버 통신
을 의도 했는데 만약 사용자가 아직 이미지 업로드가 되지 않았는데 프로필 수정 버튼을 클릭했을 시 상황을 고려해서 job을 끝내고 한번에 presignedurl을 업로드하는 방식을 채택해봤어..!
그래서 앞서 말했던 sharedflow도 잠시 이야기 해보자면
이벤트 관리를 한번에 해주고 싶다.. 라는 생각에 의도를 했는데 이쪽이 많이 부실해보이긴하네😂
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.
SharedFlow 를 일단 킵해놨다고 말해줘서 코멘트 달지 않았지만 위 형식에서 파라미터의 replay 값을 0으로 한 이유가 궁금했었어!
flow 를 수집하기 전에 앱이 백그라운드로 갔다면 데이터 유실이 있을텐데 이에 대한 방안책이 없어서 물어봤던거!_!
그리고 대부분 단일성 이벤트 (ex. 회원가입시 반복되는 아이디가 틀렸습니다) 를 SharedFlow 로 처리할 수도 있지만 여러 방법이 있으니 목적에 따라서 구현해보면 좋을 것 같아!
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.
코드가 섬세하신 것 같아요 !! API 연결 수고하셨습니다~!! 😊
domain/src/main/java/com/depromeet/domain/entity/request/home/MySeatRecordRequest.kt
Show resolved
Hide resolved
when (data.review.baseReview.images.size) { | ||
0 -> { | ||
tvHomeRecentRecordDate.visibility = View.GONE | ||
tvHomeRecentRecordName.visibility = View.GONE |
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.
tvHomeRecentRecordName.visibility = View.GONE | |
tvHomeRecentRecordName.visibility = GONE |
요렇게 적으면 훨씬 깔끔하다고 생각하는데 어떠신가요 ?!
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.
와 여태 안드로이드하면서 처음안 사실이야.. 전체적으로 수정하두고 올리고 커밋 내역 남겨놓을게!
@@ -14,6 +16,7 @@ android { | |||
|
|||
testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" | |||
consumerProguardFiles("consumer-rules.pro") | |||
buildConfigField("String", "S3_URL", getApiKey("s3.base.url")) |
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.
저는 바로 업로드를 진행하려고 했는데 혹시 S3_URL을 따로 buildConfigField에 추가하신 이유가 궁금합니다.. !
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.
이 경우 interceptor를 확인해보시면 presigned Url에 업로드 할 시 access token header를 따로 첨부하지 않습니다.
그래서 url을 따로 buildConfigField에 추가하는것이 맞다고 판단하였습니다!
val today = LocalDate.now() | ||
return today.year | ||
} | ||
|
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.
잘못적음
💻주요 작업 내용
홈
프로필
내 시야 기록 진입
시야기록 상세
디자인 수정
🎞리뷰 요청 사항
!!😁😁😁interceptor같은 경우 성식과 겹치는 부분이 있어 잠시 /api/v1/members 부분 false 임시 처리해놨습니다