박스오피스 앱 [STEP4] Jane, Tommy #59

jane1choi · 2024-03-20T08:22:08Z

버드:

안녕하세요 레드❗제인, 토미입니다!!
아마 이 PR이 마지막 PR이 될 것 같은데, 이슈해결과 공채 기간 등의 이슈로 약속드린 기한 보다 늦어진 점 죄송합니다..🥲
이번 PR도 편하게 피드백 해주시면 감사하겠습니다~!
⭐️ 조언, 제안, 논의 언제나 환영합니다 ⭐️

🔍 What is this PR?

박스오피스 프로젝트 STEP 4의 기능을 구현했습니다.

<구현 사항>

셀 클릭 시 화면 전환 구현
영화 상세 정보 조회, 영화 포스터 이미지 get 통신 진행

✏️ PR Point

사실 이번 PR 구현은 STEP3의 연장선으로 추가 기능을 구현한 것이라,, 따로 새로운 것을 도입하거나 시도해본 것은 없는 것 같아요!
다만, 기존에 네트워킹 과정에서 자잘하게 문제가 있었던 부분을 찾아서 해결하고, Clean-Architecture 구조에 맞춰서 새로운 API 요청을 추가로 처리했습니다. 코드 봐주시고 조언해주실 것들이 있다면 남겨주시면 감사하겠습니다!!!!

문제가 있었던 부분:
옵셔널 바인딩 코드에 실수가 있어 모든 요청에서 .success와 .networkFail이 동시에 발생하는 문제가 있었습니다.
해결 커밋: b49421f38c5283d3c85597636b229b925fba059f

📮 질문

현 상태로는 아래 코드와 같이 화면 전환 시 DI를 해주고 있는데요!

extension BoxOfficeViewController: UICollectionViewDelegate {
    func collectionView(_ collectionView: UICollectionView, didSelectItemAt indexPath: IndexPath) {
        let selectedMovieCode = Observable<String>(boxOfficeList[indexPath.item].movieCode)
        let selectedMovieName = Observable<String>(boxOfficeList[indexPath.item].movieName)
        
        let movieDetailVC = MovieDetailViewController(
            viewModel: MovieDetailViewModel(
                movieUseCase: DefaultMovieUseCase(
                    movieRepository: DefaultMovieRepository(
                        apiService: MovieAPIService(provider: NetworkProvider())
                    )
                ), moviePosterUseCase: DefaultMoviePoserUseCase(movieRepository: DefaultMoviePosterRepository(apiService: MoviePosterAPIService(provider: NetworkProvider())))
            ),
            movieCode: selectedMovieCode, movieName: selectedMovieName
        )
        navigationController?.pushViewController(movieDetailVC, animated: true)
    }
}

지금은 화면 전환이 하나이지만,,, 화면이 늘어날 경우 DI 코드 자체가 길어진다는 문제가 있을 것 같다는 생각이 들어서요!
이럴 경우, 코디네이터 패턴을 사용해 화면전환 코드를 분리 해주면서 코디네이터가 의존성 주입 허브의 역할을 해주도록 만들 수도 있고,
DI객체를 따로 두는 방법도 있다고 생각하는데, 레드는 이런 방법들에 대해 어떻게 생각하시는지, 효과적인 DI 방법이 궁금합니다! 의견 남겨주시면 감사하겠습니다😊😊

cherrishRed

🍀 제인 토미 정말 수고 많으셨습니다!
마지막까지 포기하지 않고 프로젝트를 하신 점 정말 칭찬드립니다! 👏👏👏
저도 리뷰 하면서 정말 즐거웠습니다!

📢 질문에 대한 답변

화면 전환이 늘어날 경우 DI 코드가 길어질 것 같은데 어떻게 관리하면 좋을까요?

코디네이터 패턴을 사용해 DI의 허브 역할을 하게 하는 방법
DI 객체를 따로 두는 방법
코디네이터와 DI를 모두 두는 방법

이건 정말 선택의 문제 인것 같습니다.
모든 방법 다 좋습니다.
지금 프로젝트 규모로 생각했을때는 2개다 오버엔지니어링이라고 생각합니다.ㅎㅎ

만약에 상용하는 앱을 만드는 것이다 라고 판단하면, (기획자가 내가 아니면)
앞으로 기획의 미래를 알 수 없으니, 미래를 위해서 3번이나 1번을 선택할 것 같습니다. (확장성을 위해서요)

지금 프로젝트 기준으로 생각 해본다면, DI 코드가 좀 많은 편이니 2번을 우선 채택하고, 추후에 필요에 의해서 코디네이터를 추가로 구현할 것 같네요.

😉 칭찬 드리고 싶은점

👏 네트워크 구현이 더욱 깔끔해진 것 같다.
👏 tableview를 잘 구현하셨다.
👏 viewModle 구조가 깔끔하다.

⛄️ 개선했으면 좋겠는점

🦋 VC 개선
viewModel 의 구조에 비해, VC에 불필요한 정보가 좀 있는 것 같습니다.
VM에서 바뀌는 정보를 VC가 구독하고 있으니, 어떤 정보를 들고 있는 것이 좋은지 고민해보시고 개선 하시면 좋을 것 같습니다.

🦋 데이터 타입 개선
VC 개선과 마찬가지로, table에 보여줄 정보를 어떤 형태로 저장하는가에 대해서 고민해 볼 필요가 있어보입니다.
특히 item의 개수는 서버에서 오는 정보의 수에 따라 가변 값이 이여야 할 것 같은데, 8 이런식으로 하드 코딩 되어 있는 것은 너무 유연하지 않은 구조 같네요.

나머지 자잘한 사항들은 코멘트에 추가로 남겨 놓았습니다!
이해가 안가거나 궁금한 점 있으시면 편하게 질문 주세요!

cherrishRed · 2024-03-21T10:29:29Z

BoxOffice/Domain/Entities/MovieEntity.swift

+    func getInfoArray() -> [(title: String, info: String)] {
+        return [
+            ("감독:", director),
+            ("제작년도:", productYear),
+            ("개봉일:", openDate),
+            ("상영시간:", showTime),
+            ("관람등급:", watchGrade),
+            ("제작국가:", nation),
+            ("장르:", genres),
+            ("배우:", actors)
+        ]
+    }


이 부분을 튜플로 처리하신 이유가 있을까요?
key value 가 있는 형태라면, dictionary도 괜찮았을 것 같은데,
이런 타입을 채택하신 이유가 궁금합니다🤔

dictionary를 사용할 경우 순서를 지킬수 없기 때문에 순서를 지키기위해 튜플을 채택하였습니다.

cherrishRed · 2024-03-21T10:30:17Z

BoxOffice/Infrastructure/Network/Foundation/NetworkConstants.swift

 enum HeaderType {
    case basic
+    case custom([String: String])
 }


custom 타입을 추가하신 부분 아주 좋습니다

cherrishRed · 2024-03-21T10:32:05Z

BoxOffice/Presentation/BoxOfficeScene/BoxOfficeViewController.swift


+    // TODO: 위치 옮기기
    private func formatDateString(_ dateString: String) -> String {


이 TODO 는 처리가 된 것 일까요?
된 것이라면, 지워주시고 안되었으면 위치를 옮겨주세요!

cherrishRed · 2024-03-21T10:39:51Z

BoxOffice/Presentation/MovieDetailScene/MovieInfoTableViewCell.swift

+    func setUpData(with info: (title: String, info: String)?) {
+         guard let info = info else { return }
+         titleLabel.text = info.title
+         infoLabel.text = info.info
+     }


위에서 본 튜플 타입이 이부분에서 바로 이렇게 쓰이려고 작성하신 거군요.
info.title 의 값을 보면,
"제목:" 이런 형식을 가지고 있는데, 이런경우
"제목" 만 값을 가지고 있고, : 은 이 부분에서 처리를 하는게 어떨까 싶네요.
info 가 의미하는 바가 데이터 묶음인데, : 까지 들고 있는게 좀 어색하기도 하고, 혹시 이부분이 아니라 다른 곳에서도 데이터를 사용한다면 혼란스러울 것 같아요.

info.title 이런 방식을 사용하고 싶으신 거라면, 아예 info 객체를 만드는 것도 한 방법이라고 생각합니다

cherrishRed · 2024-03-21T10:42:42Z

BoxOffice/Presentation/MovieDetailScene/MovieDetailViewController.swift

+        output.movieDetail.subscribe { [weak self] movieEntity in
+            guard let movieEntity = movieEntity else { return }
+            self?.movieDetail = movieEntity
+            self?.setUpNavigationBar(movieName: movieEntity.movieName)
+            self?.movieDetailTableView.reloadData()
+        }
+
+        output.moviePoster.subscribe { [weak self] moviePoster in
+            guard let moviePoster = moviePoster else { return }
+            self?.moviePoster = moviePoster
+            self?.movieDetailTableView.reloadData()
+        }


reloadData 는 모든 정보를 바꿔버리는데,
테이블의 데이터를 전체를 갈지 않고 해결하는데에는 무슨 방법이 있을까요?

cherrishRed · 2024-03-21T10:43:52Z

BoxOffice/Presentation/MovieDetailScene/MovieDetailViewController.swift

+    func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
+        switch section {
+        case 0: return 1
+        case 1: return 8
+        default: return 0
+        }
+    }
+
+    func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
+        guard let imageCell = tableView.dequeueReusableCell(withIdentifier: String(describing: MovieImageTableViewCell.self), for: indexPath) as? MovieImageTableViewCell,
+        let infoCell = tableView.dequeueReusableCell(withIdentifier: String(describing: MovieInfoTableViewCell.self), for: indexPath) as? MovieInfoTableViewCell
+        else {
+            return UITableViewCell()
+        }
+
+        switch indexPath.section {
+        case 0:
+            imageCell.setUpData(with: moviePoster)
+            return imageCell
+        case 1:
+            let info = movieDetail?.getInfoArray()[indexPath.row]
+            infoCell.setUpData(with: info)
+            return infoCell
+        default:
+            return UITableViewCell()


section 0, section 1 에 대해서 명시적인 이름이 존재하면 좋을 것 같네요.

UITableViewDataSource 프로토콜 메서드를 구현하는 방식으로 테이블 뷰의 UI를 구성하고 있습니다!
section에 접근할 수 있는 메서드에서 section이 Int 타입의 파라미터로 존재하기 때문에 section 번호로 접근을 해주었는데, 따로 section의 이름을 지정할 수 있는 방법이 있을까요???

정답이 있는 것은 아니지만 많이 사용하는 방법으로는, Section 이라는 enum 을 만들고, 그 sections: [Section] 이런 식의 형태로 정리하는 경우가 있습니다!

cherrishRed · 2024-03-21T10:45:32Z

BoxOffice/Data/Repositories/DefaultMoviePosterRepository.swift

+                    }
+                } else {
+                    completion(.pathError)
+                }


이 부분은 imagery 이 없을 때, dto 가 MoviePosterDTO 타입이 아닐때,
에러가 발생하는 부분 같은데, pathError 라는 이름은 맞지 않는 것 같아요

확인 감사합니다!
NetworkResult에 .parsingError 케이스 추가 후 해당 부분 수정해주었습니다!
반영 커밋: 7a6b0c6

cherrishRed · 2024-03-21T10:47:17Z

BoxOffice/Presentation/MovieDetailScene/MovieDetailViewController.swift

+    private var movieDetail: MovieEntity?
+    private var moviePoster: MoviePosterEntity?


VC 가 이 두 타입을 알고 있어야 하는 이유는 무엇인가요?
만약 알아야 한다면, MovieEntity 값을 구독하지 않고 moveName 구독한 이유가 궁금합니다.

jane1choi and others added 30 commits March 13, 2024 16:44

add: MovieEntity 추가

9ac6ae6

feat: MovieInfoTableViewCell 기본 UI 구성

9d9a6af

feat: 영화 상세 뷰 기본 UI 구성

81e2537

chore: cell identifier 설정 코드 수정

3dd3e49

chore: 빈 줄 삭제

94ab493

chore: 파일 경로 수정

d35aae1

chore: MovieAPIService 인스턴스 생성 방식 수정

8863c93

feat: moviedto -> entity 변환 메서드 구현

c1e9745

chore: movieCode 추가

aa4f585

feat: MovieRepository 구현

a921b87

feat: MovieUseCase 구현

03078cc

feat: 배열 변환 메서드 구현

f843f8d

feat: MovieDetailViewModel 구현

8f191c6

feat: movieDetailVC 화면전환 메소드 구현

7145e1f

feat: viewmodel 주입

785a5a1

chore: cell 별 데이터 삽입

9f48613

feat: MoviePosterDTO 생성

e174da9

feat: MoviePosterEntity 생성

a4df8ab

refactor: TargetType URL 삭제

60feb91

feat: MoviePosterAPI 생성

f07e7fe

feat: MoviePosterAPIService 생성

a3dec89

feat: PosterRepository 생성

6177f97

feat: MoviePosterUseCase 생성

4e4a95a

feat: DetailViewModel에 PosterUseCase 추가

bfea546

feat: ImageCell 생성

93cc3ef

feat: ImageCell 등록

66abaed

feat: DI 코드 추가

48e2ab3

chore: 셀 설정 코드 강제 옵셔널 추출 코드 수정

59e955d

chore: cell selectionStyle 설정 및 자잘한 코드 수정

7af04a8

fix: 스택뷰 관련 레이아웃이 깨지는 문제 해결

e1a0597

jane1choi added 4 commits March 20, 2024 01:37

chore: 자잘한 코드 수정

db3b894

chore: 이미지 contentMode 수정 및 UI 자잘한 코드 수정

51585f2

chore: 프린트문 삭제

290348f

fix: 모든 요청에서 networkFail이 발생하는 문제

b49421f

jane1choi force-pushed the feat/step4 branch from a95be96 to b49421f Compare March 20, 2024 13:57

cherrishRed reviewed Mar 21, 2024

View reviewed changes

chore: NetworkResult 케이스 추가 및 parsingError 부분 수정

7a6b0c6


		// TODO: 위치 옮기기
		private func formatDateString(_ dateString: String) -> String {

		private var movieDetail: MovieEntity?
		private var moviePoster: MoviePosterEntity?

박스오피스 앱 [STEP4] Jane, Tommy #59

Are you sure you want to change the base?

박스오피스 앱 [STEP4] Jane, Tommy #59

Uh oh!

Conversation

jane1choi commented Mar 20, 2024

🔍 What is this PR?

✏️ PR Point

📮 질문

Uh oh!

cherrishRed left a comment

Choose a reason for hiding this comment

📢 질문에 대한 답변

😉 칭찬 드리고 싶은점

⛄️ 개선했으면 좋겠는점

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants