Skip to content

[Refactor] 버스 리뉴얼 코드 리팩토링 #497

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

Merged
merged 25 commits into from
Dec 6, 2024
Merged

Conversation

ThirFir
Copy link
Contributor

@ThirFir ThirFir commented Dec 3, 2024

개요

버스 리뉴얼 코드 리팩토링

상세 작업

  • 뷰모델과 컴포저블 분리 - 뷰모델로부터 데이터 받아온 후 필요만 데이터만 하위 컴포저블에 전달
  • 로딩뷰 추가
  • 기타 네이밍, 로직 등 수정

@ThirFir ThirFir added the refactor Code refactoring label Dec 3, 2024
@ThirFir ThirFir self-assigned this Dec 3, 2024
@ThirFir ThirFir requested a review from a team as a code owner December 3, 2024 07:31
Copy link
Collaborator

@Jokwanhee Jokwanhee left a comment

Choose a reason for hiding this comment

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

고생했습니다~~~ 👍👍

Comment on lines +68 to +77
if (placeSelectMode == PlaceSelectMode.DEPARTURE) {
placeSelectMode =
if (arrival.isEmpty()) PlaceSelectMode.ARRIVAL else PlaceSelectMode.NONE
onDepartureSet(context.getString(it.titleRes))
}
else if(placeSelectMode == PlaceSelectMode.ARRIVAL) {
placeSelectMode =
if (departure.isEmpty()) PlaceSelectMode.DEPARTURE else PlaceSelectMode.NONE
onArrivalSet(context.getString(it.titleRes))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분 when 절 사용하면 더 가독성 있을 것 같은데 어떻게 생각해요!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

첨에 when이었는데 PlaceSelectMode.None 일 때 동작도 명시해줘야 해서 if로 바꿨습니다 ㅠ

onCompleteMinDepartureTime: (dateIndex: Int, daytimeIndex: Int, hourIndex: Int, minuteIndex: Int) -> Unit = { _, _, _, _ -> },

) {
var showSelectDialog by remember { mutableStateOf(false) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 당연히 알고있는 부분이겠지만, Saveable하지 않게 remember 사용한 이유가 있나요!? (다른 이유가 있는 건가 싶어서요..)

Copy link
Contributor Author

@ThirFir ThirFir Dec 5, 2024

Choose a reason for hiding this comment

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

saveable아니어도 괜찮을거라 생각했는데 다시 생각해보니 saveable인 게 좋겠네요 바꿨습니다~!

Comment on lines 37 to 38
val searchButtonEnabled by remember(departure, arrival) { derivedStateOf { departure.isNotEmpty() && arrival.isNotEmpty() } }
var placeSelectMode by remember { mutableStateOf(PlaceSelectMode.NONE) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시라도 놓친 부분이라면 여기도 rememberSaveable 달아둘게요~

Copy link
Contributor

@nodobi nodobi left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!
확실히 리팩토링 거치면서 코드 분리가 된게 느껴지는 거 같아요. 👍

개인적인 궁금증인데, 리팩토링 하는 과정에선 어떤 부분들을 고려하셨나요?

Comment on lines +156 to +160
emit(BusTimetableUiState.Success(
shuttleRegions.await(), expressTimetable.await(), cityTimetable.await()
))
}.catch {
emit(BusTimetableUiState.LoadFailed)
Copy link
Contributor

Choose a reason for hiding this comment

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

이런식으로 구성하니까 읽기 좋아보여요! 👍

Comment on lines +95 to +100
sealed interface BusSearchResultUiState {
data class Success(val departureInfos: List<BusDepartureInfoViewState>) : BusSearchResultUiState
data object Loading : BusSearchResultUiState
data object LoadFailed : BusSearchResultUiState
}

Copy link
Contributor

Choose a reason for hiding this comment

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

여기서 sealed class 대신 sealed interface 를 사용하신 이유가 뭔가요?? 🙋‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

별로 의미 두진 않았고 일반적으로 그렇게 하기 때문에 따랐슴니다 ㅎ
혹시 그럼 class로 해야 한다는 이유 같은 것을 생각해보신적 있으신가용??

Copy link
Contributor

Choose a reason for hiding this comment

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

저도 단순히 sealed class 로 많이 했어서 궁금했어요 ㅋㅋㅋ 😅
찾아보니 sealed interface 로 하는게 괜찮아보이네요!!

@ThirFir
Copy link
Contributor Author

ThirFir commented Dec 5, 2024

리팩토링 하는 과정에선 어떤 부분들을 고려하셨나요?

최우선 목표는 뷰모델 선언부와 UI를 분리하는 것이었습니다.
뷰모델의 선언부에 UI 로직을 두니 프리뷰가 간헐적으로 안보이더라구여.

다음은 뷰모델에 상태가 아닌 변수들을 상태화하는 거였어용
그래서 일반 변수였던 것들을 StateFlow화 했습니다.

또, UiState의 형태를 Success, Loading, LoadFailed 3가지 기본 형태로 정형화하려 했습니다.
이렇게만 해도 UI상태 관리에 큰 문제를 겪지 않으면서도 코드도 깔끔해지더라구여~

겸사겸사 불편했던 로직들도 좀 수정해주고여 ㅎ

@ThirFir ThirFir merged commit d739739 into develop Dec 6, 2024
@ThirFir ThirFir deleted the refactor/bus-logic branch December 25, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants