Skip to content
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

refactor : EventDetail 코드 리팩토링 #282

Merged
merged 11 commits into from
Aug 11, 2023
2 changes: 1 addition & 1 deletion android/2023-emmsale/app/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
android:name=".presentation.ui.onboarding.OnboardingActivity"
android:exported="true" />
<activity
android:name=".presentation.eventdetail.EventDetailActivity"
android:name=".presentation.ui.eventdetail.EventDetailActivity"
android:exported="false" />
<activity
android:name=".presentation.ui.login.LoginActivity"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
package com.emmsale.data.eventdetail

import java.time.LocalDateTime

data class EventDetail(
val id: Long,
val name: String,
val status: String,
val location: String,
val startDate: String,
val endDate: String,
val startDate: LocalDateTime,
val endDate: LocalDateTime,
Comment on lines -8 to +11
Copy link
Collaborator

Choose a reason for hiding this comment

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

데이터 모델에서 날짜 표현을 위해 String에서 LocalDateTime으로 타입을 바꾼 것 좋네요 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

감사요

val informationUrl: String,
val tags: List<String>,
val imageUrl: String?,
Copy link
Member

Choose a reason for hiding this comment

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

imageUrl 대신에 posterImageUrl은 어떤가요?
imageUrl이라고 하면 어떤 이미지의 Url인지 파악하기가 어려울 수 있을 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

인정

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ package com.emmsale.data.eventdetail
import com.emmsale.data.common.ApiResult

interface EventDetailRepository {
suspend fun fetchEventDetail(eventId: Long): ApiResult<EventDetail>
suspend fun getEventDetail(eventId: Long): ApiResult<EventDetail>
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ class EventDetailRepositoryImpl(
private val eventDetailService: EventDetailService,
) : EventDetailRepository {

override suspend fun fetchEventDetail(eventId: Long): ApiResult<EventDetail> {
override suspend fun getEventDetail(eventId: Long): ApiResult<EventDetail> {
return handleApi(
execute = { eventDetailService.fetchEventDetail(eventId) },
execute = { eventDetailService.getEventDetail(eventId) },
mapToDomain = EventDetailApiModel::toData,
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import retrofit2.http.Path

interface EventDetailService {
@GET("events/{eventId}")
suspend fun fetchEventDetail(
suspend fun getEventDetail(
@Path("eventId") eventId: Long,
): Response<EventDetailApiModel>
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package com.emmsale.data.eventdetail.dto
import com.emmsale.data.eventdetail.EventDetail
import kotlinx.serialization.SerialName
import kotlinx.serialization.Serializable
import java.time.LocalDateTime
import java.time.format.DateTimeFormatter

@Serializable
data class EventDetailApiModel(
Copy link
Collaborator

Choose a reason for hiding this comment

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

ApiModel을 다른 레이어의 모델로 변경하는 작업은 Mapper에서 하기로 컨벤션을 결정했던 것 같습니다! 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 맞네요 ㅎㅎ깜빡했어요 감사요

Expand Down Expand Up @@ -34,12 +36,17 @@ data class EventDetailApiModel(
name = name,
status = status,
location = location,
startDate = startDate,
endDate = endDate,
startDate = startDate.toLocalDateTime(),
endDate = endDate.toLocalDateTime(),
informationUrl = informationUrl,
tags = tags,
imageUrl = imageUrl,
remainingDays = remainingDays,
type = type,
)

private fun String.toLocalDateTime(): LocalDateTime {
val format = DateTimeFormatter.ofPattern("yyyy:MM:dd:HH:mm:ss")
return LocalDateTime.parse(this, format)
}
}
Original file line number Diff line number Diff line change
@@ -1,16 +1,13 @@
package com.emmsale.presentation.eventdetail
package com.emmsale.presentation.ui.eventdetail

import android.content.Context
import android.content.Intent
import android.os.Bundle
import androidx.activity.viewModels
import androidx.appcompat.app.AppCompatActivity
import com.emmsale.R
import com.emmsale.databinding.ActivityEventDetailBinding
import com.emmsale.presentation.common.extension.showToast
import com.emmsale.presentation.ui.eventdetail.EventDetailFragmentStateAdpater
import com.emmsale.presentation.ui.eventdetail.EventDetailViewModel
import com.emmsale.presentation.ui.eventdetail.EventTag
import com.emmsale.presentation.ui.eventdetail.uistate.EventDetailUiState
import com.google.android.material.tabs.TabLayoutMediator

class EventDetailActivity : AppCompatActivity() {
Expand All @@ -24,28 +21,27 @@ class EventDetailActivity : AppCompatActivity() {
super.onCreate(savedInstanceState)
setUpBinding()
setUpEventDetail()
setBackPress()
onBackPressButtonClick()
viewModel.fetchEventDetail(eventId)
}

private fun setUpBinding() {
binding = ActivityEventDetailBinding.inflate(layoutInflater)
binding.lifecycleOwner = this
binding.vm = viewModel
Comment on lines +29 to +30
Copy link
Member

Choose a reason for hiding this comment

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

이 코드는 setContentView 이후에 호출해주는 것도 좋을 것 같아요.
viewModel이 초기화되면서 실행되는 로직이 있다면 버그가 발생할 수 있습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

확인이요

setContentView(binding.root)
}

private fun setUpEventDetail() {
viewModel.eventDetail.observe(this) { eventDetailUiState ->
when (eventDetailUiState) {
is EventDetailUiState.Success -> {
binding.eventDetail = eventDetailUiState
addTag(eventDetailUiState.tags)
initFragmentStateAdapter(
eventDetailUiState.informationUrl,
eventDetailUiState.imageUrl,
)
}

else -> showToast("행사 받아오기 실패")
if (eventDetailUiState.isError) {
showToast(getString(R.string.eventdetail_fetch_eventdetail_error_message))
} else {
addTag(eventDetailUiState.tags)
Copy link
Member

Choose a reason for hiding this comment

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

어떤 태그를 추가하는 메서드인지 구체적으로 명명해주시면 좋을 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

확인이요

initFragmentStateAdapter(
eventDetailUiState.informationUrl,
eventDetailUiState.imageUrl,
)
}
}
}
Expand All @@ -55,9 +51,14 @@ class EventDetailActivity : AppCompatActivity() {
EventDetailFragmentStateAdpater(this, eventId, informationUrl, imageUrl)
TabLayoutMediator(binding.tablayoutEventdetail, binding.vpEventdetail) { tab, position ->
when (position) {
INFORMATION_TAB_POSITION -> tab.text = "상세 정보"
COMMENT_TAB_POSITION -> tab.text = "댓글"
RECRUITMENT_TAB_POSITION -> tab.text = "같이가요"
INFORMATION_TAB_POSITION ->
tab.text =
getString(R.string.eventdetail_tab_infromation)

COMMENT_TAB_POSITION -> tab.text = getString(R.string.eventdetail_tab_comment)
RECRUITMENT_TAB_POSITION ->
tab.text =
getString(R.string.eventdetail_tab_recruitment)
}
}.attach()
binding.vpEventdetail.isUserInputEnabled = false
Expand All @@ -71,7 +72,7 @@ class EventDetailActivity : AppCompatActivity() {
text = tag
}

private fun setBackPress() {
private fun onBackPressButtonClick() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

위로 가기 버튼을 클릭했을 때 실행되어야 할 작업 모음을 정의하는 함수의 이름이라면 onBackPressButtonClick이라는 이름이 괜찮다고 생각합니다! 하지만 위 함수는 위로 가기 버튼이 클릭 이벤트가 발생했을 때 처리하는 일을 하는 OnClickListener를 변경하므로 initBackPressOnClickListener 이렇게 이름을 짓는 것이 더 정확하다고 생각합니다:)
저라면 initUpButton 혹은 setupUpButton 이렇게 지을 것 같습니다 :)

Copy link
Member

Choose a reason for hiding this comment

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

저는 특정 View에 ClickListener를 설정할 때 initXxxClickListener() 라는 메서드명을 자주 사용합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

인정이요 반영할게요

binding.ivEventdetailBackpress.setOnClickListener {
finish()
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,47 +1,58 @@
package com.emmsale.presentation.ui.eventdetail

import androidx.lifecycle.LiveData
import androidx.lifecycle.MutableLiveData
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import com.emmsale.data.common.ApiError
import com.emmsale.data.common.ApiException
import com.emmsale.data.common.ApiSuccess
import com.emmsale.data.eventdetail.EventDetail
import com.emmsale.data.eventdetail.EventDetailRepository
import com.emmsale.data.member.MemberRepository
import com.emmsale.presentation.KerdyApplication
import com.emmsale.presentation.common.ViewModelFactory
import com.emmsale.presentation.common.livedata.NotNullLiveData
import com.emmsale.presentation.common.livedata.NotNullMutableLiveData
import com.emmsale.presentation.ui.eventdetail.uistate.EventDetailUiState
import kotlinx.coroutines.launch

class EventDetailViewModel(
private val eventDetailRepository: EventDetailRepository,
private val memberRepository: MemberRepository,
) : ViewModel() {

private val _eventDetail: MutableLiveData<EventDetailUiState> =
MutableLiveData<EventDetailUiState>()
val eventDetail: LiveData<EventDetailUiState>
private val _eventDetail: NotNullMutableLiveData<EventDetailUiState> =
Copy link
Member

Choose a reason for hiding this comment

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

_eventDetail의 타입을 명시해주지 않으면 한 줄에 깔끔하게 작성할 수 있어요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저는 무조건 타입을 명시해야 한다고 생각합니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

어떤 타입인지 명확하게 적는 것이 실수를 방지한다고 생각해요

NotNullMutableLiveData(EventDetailUiState())
val eventDetail: NotNullLiveData<EventDetailUiState>
get() = _eventDetail
Copy link
Member

Choose a reason for hiding this comment

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

다음과 같이 작성해보는 것은 어떠신가요?
val eventDetail: NotNullLiveData<EventDetailUiState>= _eventDetail

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네^^


fun fetchEventDetail(id: Long) {
setLoadingState(true)
viewModelScope.launch {
when (val result = eventDetailRepository.fetchEventDetail(id)) {
is ApiSuccess -> _eventDetail.postValue(
EventDetailUiState.from(result.data),
)

is ApiError -> _eventDetail.postValue(EventDetailUiState.Error)
is ApiException -> _eventDetail.postValue(EventDetailUiState.Error)
when (val result = eventDetailRepository.getEventDetail(id)) {
is ApiSuccess -> fetchSuccessEventDetail(result.data)
is ApiError -> changeToErrorState()
is ApiException -> changeToErrorState()
}
}
}

private fun setLoadingState(state: Boolean) {
_eventDetail.value = _eventDetail.value.copy(isLoading = state)
}

private fun fetchSuccessEventDetail(eventDetail: EventDetail) {
_eventDetail.value = EventDetailUiState.from(eventDetail)
}

private fun changeToErrorState() {
_eventDetail.value = _eventDetail.value.copy(
isError = true,
isLoading = false,
)
}

companion object {
val factory = ViewModelFactory {
EventDetailViewModel(
eventDetailRepository = KerdyApplication.repositoryContainer.eventDetailRepository,
memberRepository = KerdyApplication.repositoryContainer.memberRepository,
)
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,30 +1,28 @@
package com.emmsale.presentation.ui.eventdetail.uistate

import com.emmsale.data.eventdetail.EventDetail
import java.time.LocalDate
import java.time.LocalDateTime
import java.time.LocalTime
import java.time.format.DateTimeFormatter

sealed class EventDetailUiState {
data class Success(
val id: Long,
val name: String,
val status: String,
val location: String,
val startDate: String,
val endDate: String,
val informationUrl: String,
val tags: List<String>,
val imageUrl: String?,
) : EventDetailUiState()

object Error : EventDetailUiState()
data class EventDetailUiState(
val id: Long = DEFAULT_ID,
val name: String = "",
val status: String = "",
val location: String = "",
val startDate: String = "",
val endDate: String = "",
val informationUrl: String = "",
val tags: List<String> = listOf(),
val imageUrl: String? = "",
val isError: Boolean = false,
val isLoading: Boolean = false,
) {

companion object {
fun from(eventDetail: EventDetail): Success {
private const val DEFAULT_ID = -1L
fun from(eventDetail: EventDetail): EventDetailUiState {
return with(eventDetail) {
Success(
EventDetailUiState(
id = id,
name = name,
status = status,
Expand All @@ -34,20 +32,15 @@ sealed class EventDetailUiState {
informationUrl = informationUrl,
tags = tags,
imageUrl = imageUrl,
isError = false,
isLoading = false,
)
}
}

private fun getGeneralDateFormat(dateString: String): String {
val formatter = DateTimeFormatter.ofPattern("yyyy:MM:dd:HH:mm:ss")
val dateTime = LocalDateTime.parse(dateString, formatter)

val targetDate = LocalDate.of(dateTime.year, dateTime.month, dateTime.dayOfMonth)
val targetTime = LocalTime.of(dateTime.hour, dateTime.minute)
val targetDateTime = LocalDateTime.of(targetDate, targetTime)

private fun getGeneralDateFormat(dateTime: LocalDateTime): String {
val resultFormatter = DateTimeFormatter.ofPattern("yyyy.M.d HH:mm")
return targetDateTime.format(resultFormatter)
return dateTime.format(resultFormatter)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import com.emmsale.presentation.common.extension.getParcelableExtraCompat
import com.emmsale.presentation.common.extension.showToast
import com.emmsale.presentation.common.views.FilterTag
import com.emmsale.presentation.common.views.filterChipOf
import com.emmsale.presentation.eventdetail.EventDetailActivity
import com.emmsale.presentation.ui.eventdetail.EventDetailActivity
import com.emmsale.presentation.ui.main.event.conference.recyclerview.ConferenceRecyclerViewAdapter
import com.emmsale.presentation.ui.main.event.conference.recyclerview.ConferenceRecyclerViewDivider
import com.emmsale.presentation.ui.main.event.conference.uistate.ConferencesUiState
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ class NotificationBoxViewModel(

private suspend fun getConferenceNameAsync(conferenceId: Long): Deferred<String?> =
viewModelScope.async {
when (val conference = eventDetailRepository.fetchEventDetail(conferenceId)) {
when (val conference = eventDetailRepository.getEventDetail(conferenceId)) {
is ApiSuccess -> conference.data.name
is ApiException,
is ApiError,
Expand Down
Loading