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.

Sign up for GitHub

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

Jump to bottom

🔀 :: (#492) PlaylistDetail을 ReactorKit으로 리팩합니다. #514

Merged
merged 37 commits into from
May 5, 2024
Merged

🔀 :: (#492) PlaylistDetail을 ReactorKit으로 리팩합니다. #514

merged 37 commits into from
May 5, 2024

Conversation

yongbeomkwak
Copy link
Member

@yongbeomkwak yongbeomkwak commented May 3, 2024 •
edited
Loading

💡 배경 및 개요

PR을 하게 된 문제상황, 배경 등 개요에 대해서 작성해주세요!

퍼블리싱의 경우 스크린샷/동영상도 추가해주면 좋아요!

Resolves: #492

📃 작업내용

  • 플레이리스트 디테일 화면을 reactorKit으로 리팩했습니다.

🙋‍♂️ 리뷰노트

구현 시에 고민이었던 점들 혹은 특정 부분에 대한 의도가 있었다면 PR 리뷰의 이해를 돕기 위해 서술해주세요!

또한 리뷰어에게 특정 부분에 대한 집중 혹은 코멘트 혹은 질문을 요청하는 경우에 작성하면 좋아요!

e.g. 작업을 끝내야할 시간이 얼마 없어 확장성보다는 동작을 위주로 만들었어요! 감안하고 리뷰해주세요!

✅ PR 체크리스트

템플릿 체크리스트 말고도 추가적으로 필요한 체크리스트는 추가해주세요!

  • 이 작업으로 인해 변경이 필요한 문서가 변경되었나요? (e.g. XCConfig, 노션, README)
  • 이 작업을 하고나서 공유해야할 팀원들에게 공유되었나요? (e.g. "API 개발 완료됐어요", "XCConfig 값 추가되었어요")
  • 작업한 코드가 정상적으로 동작하나요?
  • Merge 대상 브랜치가 올바른가요?
  • PR과 관련 없는 작업이 있지는 않나요?

🎸 기타

남은 작업

  • songCart 관련 작업
  • 저장후 업데이트 (노티)
  • 로그인이 필요한 커스텀 플리로 테스트

@github-actions GitHub Actions
Copy link

github-actions bot commented May 3, 2024 •
edited
Loading

✅ Successful finished SwiftLint

@github-actions GitHub Actions
Copy link

github-actions bot commented May 3, 2024

✅ Assign 자동 지정을 성공했어요!

@yongbeomkwak

@baekteun baekteun changed the title 492 PlaylistDetail을 ReactorKit으로 리팩합니다. 🔀 :: (#492) PlaylistDetail을 ReactorKit으로 리팩합니다. May 3, 2024
@baekteun baekteun added the ♻️ Refactor 코드 리팩토링 label May 3, 2024
baekteun
baekteun reviewed May 4, 2024
View reviewed changes
Projects/Features/PlaylistFeature/Sources/PlayListDetailViewModel.swift Show resolved Hide resolved
Projects/Features/PlaylistFeature/Sources/Reactors/PlaylistDetailReactor.swift Outdated
Comment on lines 43 to 45
internal let type: PlayListType
private var disposeBag = DisposeBag()
internal let key: String
Copy link
Member

Choose a reason for hiding this comment

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

혹기 type이랑 key는 왜 internal인가요? 외부에서 접근하지고 않고 외부에서 접근하는거도 안좋은 동작이라 private가 적합한거같아서요

Copy link
Member Author

Choose a reason for hiding this comment

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

어 둘다 뷰컨에서 써요

Copy link
Member

Choose a reason for hiding this comment

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

음 그렇군요
일단 저거 private 사이에 넣어놓기보다 둘을 묶어서 따로 빼는게 보기 좋을거같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

반영햇습니다

Projects/Features/PlaylistFeature/Sources/Reactors/PlaylistDetailReactor.swift Outdated
Comment on lines 85 to 88

case .tapEdit:
return updateIsEditing(true)

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
case .tapEdit:
return updateIsEditing(true)
case .tapEdit:
return updateIsEditing(true)

요기 줄바꿈이 이상한거같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

반영했습니다.

Projects/Features/PlaylistFeature/Sources/Reactors/PlaylistDetailReactor.swift Outdated
Comment on lines 215 to 222
// TODO: 로그 찍기 , LogManager.printError("playlist detail datasource is empty")
guard var tmp = currentState.dataSource.first?.items else {
return .empty()
}
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
Member Author

Choose a reason for hiding this comment

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

로그는 병합하고 이용할 수 있지 않나요 ?? LogManager 자체가 지금 제 브랜치에서는 없어요

Copy link
Member

Choose a reason for hiding this comment

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

pull이나 rebase 하셔여할거같아요 develop에 머지됐어요

Copy link
Member Author

Choose a reason for hiding this comment

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

로깅 적용

Projects/Features/PlaylistFeature/Sources/Reactors/PlaylistDetailReactor.swift Outdated Show resolved Hide resolved
Projects/Features/PlaylistFeature/Sources/ViewControllers/PlayListDetailViewController.swift Outdated
Comment on lines 128 to 131
tableView.register(
UINib(nibName: "SongListCell", bundle: BaseFeatureResources.bundle),
forCellReuseIdentifier: "SongListCell"
)
Copy link
Member

Choose a reason for hiding this comment

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

이 친구는 bind보다 configureUI가 더 적절하지 않으려나요.?.?

Copy link
Member Author

Choose a reason for hiding this comment

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

엇 이동하겠습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

반영 완료

Projects/Features/PlaylistFeature/Sources/ViewControllers/PlayListDetailViewController.swift Outdated
return
}
self.activityIndicator.stopAnimating()
let currentState = reactor.state
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let currentState = reactor.state
let currentState = reactor.state.share(replay: 4)

이러면 옵져버블 스트림 하나로 여럿에 공유가능해요
ref : https://reactivex.io/documentation/operators/refcount.html

Copy link
Member Author

Choose a reason for hiding this comment

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

그러니깐 4번을 한 흐름으로 처리한다는 뜻인가요 ?

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
Member Author

Choose a reason for hiding this comment

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

오 신기하다 ...

@yongbeomkwak yongbeomkwak force-pushed the 492-refactory-playlistDetail branch from 8cb5f25 to 893d8df Compare May 4, 2024 08:31
youn9k
youn9k reviewed May 4, 2024
View reviewed changes
Projects/Features/PlaylistFeature/Sources/Reactors/PlaylistDetailReactor.swift
}

// 데이터 업데이트

Copy link
Member

Choose a reason for hiding this comment

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

Before
// 데이터 업데이트

func updateDataSource(_ isBackup: Bool) -> Observable<Mutation> {

After
/// 데이터 업데이트
func updateDataSource(_ isBackup: Bool) -> Observable<Mutation> {

주석 방식이 얘만 달라요!

Copy link
Member Author

Choose a reason for hiding this comment

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

확인!

youn9k
youn9k reviewed May 4, 2024
View reviewed changes
Projects/Features/PlaylistFeature/Sources/ViewControllers/PlayListDetailViewController.swift Outdated
guard let self = self else {
.bind(onNext: { owner, model in

guard let type = owner.reactor?.type else {
Copy link
Member

Choose a reason for hiding this comment

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

Before
guard let type = owner.reactor?.type else {
    return
}
After
guard let type = owner.reactor?.type else { return }

간단한 guard else return 구문은 한줄로 표현하는게 가독성이 좋은것같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

확인했씁니다. 근데 이거 제안 기능으로 바로 올려주실 수 있던걸로 알아요 한번 나중에는 그렇게 해보실래여? 그러면 바로 커밋 가능

Copy link
Member

Choose a reason for hiding this comment

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

어 다시보니 이 type 변수를 안 사용하네요?

Copy link
Member Author

Choose a reason for hiding this comment

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

그것도 수정했씁니다 !

Copy link
Member

Choose a reason for hiding this comment

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

확인했씁니다. 근데 이거 제안 기능으로 바로 올려주실 수 있던걸로 알아요 한번 나중에는 그렇게 해보실래여? 그러면 바로 커밋 가능

아 이건 몰랐네요! 한번 써볼게용

youn9k
youn9k reviewed May 4, 2024
View reviewed changes
Projects/Features/PlaylistFeature/Sources/ViewControllers/PlayListDetailViewController.swift

self.playListImage.kf.setImage(
with: type == .wmRecommend ? WMImageAPI.fetchRecommendPlayListWithSquare(
owner.playListImage.kf.setImage(
Copy link
Member

Choose a reason for hiding this comment

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

// Before
owner.playListImage.kf.setImage(
                    with: owner.reactor?.type == .wmRecommend ? WMImageAPI.fetchRecommendPlayListWithSquare(
                        id: model.image,
                        version: model.version
                    ).toURL : WMImageAPI.fetchPlayList(id: model.image, version: model.version).toURL
// After
let imageURL = owner.reactor?.type == .wmRecommend ? 
WMImageAPI.fetchRecommendPlayListWithSquare(id: model.image, version: model.version).toURL : 
WMImageAPI.fetchPlayList(id: model.image, version: model.version).toURL
owner.playListImage.kf.setImage(
                    with: imageURL,
                    ...
)

코드가 너무 길어서 가독성을 위해 imageURL을 변수로 따로 뺴고 들여쓰기도 수정해봤는데 이런 방식은 어떤가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

오 좋은 것 같아요 ! 반영할께요

youn9k
youn9k reviewed May 4, 2024
View reviewed changes
Projects/Features/PlaylistFeature/Sources/ViewControllers/PlayListDetailViewController.swift Outdated Show resolved Hide resolved
Projects/Features/PlaylistFeature/Sources/ViewControllers/PlayListDetailViewController.swift
moreButton.rx.tap
.withUnretained(self)
.subscribe(onNext: { owner, _ in

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

공백을 제거해요

Projects/Features/PlaylistFeature/Sources/ViewControllers/PlayListDetailViewController.swift
backButton.rx.tap
.withUnretained(self)
.subscribe(onNext: { owner, _ in

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

공백을 제거해요

Projects/Features/PlaylistFeature/Sources/ViewControllers/PlayListDetailViewController.swift
}
)
owner.showPanModal(content: vc)

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

공백을 제거해요

Projects/Features/PlaylistFeature/Sources/ViewControllers/PlayListDetailViewController.swift Outdated Show resolved Hide resolved
Projects/Features/PlaylistFeature/Sources/ViewControllers/PlayListDetailViewController.swift Outdated Show resolved Hide resolved
yongbeomkwak and others added 3 commits May 5, 2024 00:30
@yongbeomkwak @youn9k
Update Projects/Features/PlaylistFeature/Sources/ViewControllers/Play…
bef3cf8 
…ListDetailViewController.swift

Co-authored-by: Youngkyu Song <[email protected]>
@yongbeomkwak @youn9k
Update Projects/Features/PlaylistFeature/Sources/ViewControllers/Play…
4efd213 
…ListDetailViewController.swift

Co-authored-by: Youngkyu Song <[email protected]>
@yongbeomkwak @youn9k
Update Projects/Features/PlaylistFeature/Sources/ViewControllers/Play…
7868c2c 
…ListDetailViewController.swift

Co-authored-by: Youngkyu Song <[email protected]>
youn9k
youn9k approved these changes May 4, 2024
View reviewed changes
Copy link
Member

@youn9k youn9k left a comment

Choose a reason for hiding this comment

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

리액터킷으로 리팩하니까 코드 흐름파악하기가 좋아서 리뷰하기도 편하네요👀👍

@yongbeomkwak yongbeomkwak merged commit db10f06 into develop May 5, 2024
3 checks passed
@yongbeomkwak yongbeomkwak deleted the 492-refactory-playlistDetail branch May 5, 2024 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baekteun baekteun baekteun approved these changes

@youn9k youn9k youn9k approved these changes

@kimdaehee0824 kimdaehee0824 Awaiting requested review from kimdaehee0824 kimdaehee0824 is a code owner

@KangTaeHoon KangTaeHoon Awaiting requested review from KangTaeHoon KangTaeHoon is a code owner

Assignees

@yongbeomkwak yongbeomkwak

Labels
♻️ Refactor 코드 리팩토링
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PlaylistDetail을 ReactorKit으로 리팩합니다.
4 participants
@yongbeomkwak @KangTaeHoon @youn9k @baekteun