박스오피스 앱 [STEP 3] Jin #61

wlsdud-dev · 2024-03-20T11:05:47Z

리뷰어: 알라딘 @junbangg

안녕하세요. 알라딘!
드디어 길고 길었던 Step3 PR을 요청 하게 됐습니다.
개인적으로 어려움이 많았던 기간이라 프로젝트 진행이 어려웠습니다.
그래도 약속을 지키지 않으면 안됐는데, 알라딘과의 약속을 지키지 못해서 죄송합니다.

따금한 리뷰 부탁드립니다.
요즘 날씨가 갑자기 추워지는데 감기 조심하세요.
감사합니다.

이번 PR에서 집중한 부분

CodeBase: 스토리보드를 사용하지 않고 코드베이스로 작업을 해보고 싶었습니다. 뷰를 생성하고 추가해서 레이아웃을 잡어주는 부분이 아직은 많이 어색하지만 구현을 할 수 있어 좋았습니다.

데이터전달: API로 요청한 데이터 값을 cell의 어떻게 넣어줘야 하는지 고민 하여 프로퍼티 감시자를 활용해 데이터 전달을 구현 했습니다.

스타트로딩뷰: 앱실행시 데이터를 받아오기 전 까지 로딩화면을 띄워주는 방식을 화면 전체를 가리지 않고 cell 부분만 가려주는 방식으로 네비게이션으로 뷰를 이동할 때 좀 더 자연스럽다고 생각해 구현했습니다.

고민되는 점

앱 첫 실행시 데이터를 가지고 오기 전 까지 보여지는 로딩 뷰를 어디서 숨겨 줘야 할까요?
현재는 requstBoxOfficeData 에서 로딩뷰를 숨겨주는 명령이 실행되는데 데이터를 요청하는 메서드가 이 책임까지 하는게 맞는지 고민이 됩니다. 그래서 프러퍼티 감시자를 활용해 구현을 해보았지만 적절한것 같지 않아 커밋한 형태로 구현 했습니다.

private var boxOfficeData: BoxOfficeDTO? {
        didSet {
            loadingView.isLoading = boxOfficeData != nil ? false : true
        }
    }

MVC 패턴에서 view가 데이터를 가지고 계산을 하는것이 옳은가요?
MVC 패턴에서 view는 화면에 그리기만 해야하고 데이터를 알면 안된다고 알고 있습니다. cell객체는 뷰에 해당한다고 생각하는데요.
하지만 현재 코드에서 formatMovieLabel 메서드가 controller가 넘겨준 값을 가지고 분기로 어떻게 화면에 그릴지 연산을 합니다.
cell에 넘겨주기 전에 계산을 전부 하고 넘겨주는게 맞는걸까요?
DTO와 Entity?
DTO와 Entity의 개념이 안잡혀서 그런것 같습니다.
제가 이해한 DTO와 Entity는 DTO는 서비스에서 요청한 데이터와 1:1로 매핑되는 객체이고 Entity는 DTO로 받은 데이터를 가공하여 뷰에 사용하는 객체로 이해를 했습니다. 저는 Entity가 불필요한 작업이라 생각해 DTO를 그대로 사용 했는데 Entity를 사용하는 프로젝트도 많은것 같습니다. 현재 프로젝트에서 Entity를 활용하면 어떤점이 좋을까요?

BoxOffice/Extensions/String+.swift

junbangg · 2024-03-22T15:05:40Z

BoxOffice/Extensions/String+.swift

+        formatter.numberStyle = .decimal
+        formatter.locale = Locale.current
+        if let number = Double(self) {
+            return formatter.string(from: NSNumber(value: number)) ?? self


여기도 옵셔널 처리를 하고있네요?? 17째줄에서 함께 언래핑 하는건 어떨까요?

같이 수정 했습니다.

junbangg · 2024-03-22T15:06:15Z

BoxOffice/Extensions/String+.swift

+    /// 숫자가 세 자리 이상 넘어가면 ,를 활용하는 메서드
+    func formatNumberString() -> String {


물론 주석이 필요한 경우도 있지만, 메서드명을 통해서 표현을 해보는건 어떨까요?

메서드 명을 변경해 수정했습니다.
아직은 익숙하지 않아서 그런지 모르겠지만 주석을 추가하는 것이 메서드의 기능을 이해하는데 더 명확하다고 생각하는데
알라딘 의견은 어떤지 궁금합니다.

주석 없이 자체로 충분히 설명이 될만큼 읽기 좋은 코드를 추구하긴 합니다 개인적으로는!
하지만 필요에 따라서 충분히 주석이 필요한 경우도 있더라고요~

테스트 코드 설명

TODO/ FIX-ME 주석

코드만으로는 작성 의도가 파악이 도저히 안되는 코드에 대한 부연 설명..!

이정도일것 같아요~ 근데 충분히 메서드나 변수명으로 목적이 표현 가능한 코드에 대해서는 네이밍과 단일책임원칙으로 충분히 가독성을 높일 수 있다고 생각합니다!

junbangg · 2024-03-22T15:07:03Z

BoxOffice/Extensions/UILabel+.swift

+    /* 텍스트 구간 색상 변경 */
+    func setTextColor(_ color: UIColor, range: NSRange) {


setTextColor <- 이미 메서드명을 충분히 잘 지어주셔서, 주석은 없애도 되겠는데요?!👍👍

junbangg · 2024-03-22T15:07:27Z

BoxOffice/Extensions/UILabel+.swift

+    /* AttributedString이 설정되어있지 않으면 생성하여 반환한다. */
+    private func mutableAttributedString() -> NSMutableAttributedString? {


요 주석 또한 최대한 코드로 읽히게 하면 좋을것 같습니다~

주석은 제가 했지만, 메서드명을 어떤 식으로 바꿔야 적절한지 찾지 못했습니다.

junbangg · 2024-03-22T15:19:38Z

BoxOffice/Present/BoxOfficeViewController.swift

+    private func requstBoxOfficeData() {
+        let date = Date.yesterdayFormatted
+
+        NetworkManager.shared.fetchDailyBoxOffice(for: date) { [weak self] result in


윅! 👍👍

junbangg · 2024-03-22T15:20:21Z

BoxOffice/Present/BoxOfficeViewController.swift

+                }
+
+                if self?.isAppStartLoading == true {
+                    self?.callLoadingView { [weak self] in


요것도 결국 DispatchQueue.main.async 안에 넣어주는거네요???
요거 왜 필요한건가요?!

junbangg · 2024-03-22T15:20:39Z

BoxOffice/Present/BoxOfficeViewController.swift

+
+        requstBoxOfficeData()
+
+        DispatchQueue.main.asyncAfter(deadline: .now() + 0.7) {


asyncAfter(deadline: .now() + 0.7 를 하신 이유가 있을까요??

밑으로 스와이프 하여 새로고침을 할때 새로고침뷰와 컬렉션셀이 겹쳐지는 현상이 발생해 부자연스러운 동작을 합니다.
새로고침뷰가 조금 더 위에 머물 수 있도록 해당 코드를 넣어주었습니다.
적절한 방법이 맞는지 의문이 들어 다른 방법을 찾아보았지만 찾지 못하였습니다.

junbangg · 2024-03-22T15:21:42Z

BoxOffice/Present/LoadingView.swift

+
+    var isLoading = false {
+        didSet {
+            self.isHidden = !self.isLoading


개취 일 수도 있지만, 그냥 self.isLoading == false 도 나쁘지 않은 가독성에 기여를 하는것 같아요!

생각 못했던 방법인데 좋은것 같습니다.
한가지 더 배웠습니다!

junbangg · 2024-03-22T15:27:01Z

BoxOffice/Present/LoadingView.swift

+        return label
+    }()
+
+    var isLoading = false {


isLoading 의 값을 바꾸면, 그 값에 따라서 인디케이터가 돌기 시작하는 그런 동작 구조 이군요?!
아주 창의적이고 좋은것 같습니다~👍👍
다만, 저는 개인적으로 명시적으로 메서드를 통해 돌렸다 멈췄다 할 수 있게 하는게 더 사용하기 좋은 코드가 되지 않을까 싶어요..!
isLoading 은 말그대로 해당 뷰의 상태를 체크할 수 있는 프로퍼티 같아보여요🤔
제 말이 다 옳은건 아니니, 이건 한번 고민 해보시면 좋을것 같아요!

감사합니다, 좀 더 고민해보도록 하겠습니다!

junbangg · 2024-03-22T15:37:31Z

너무 너무 고생 많으셨습니다~ 👏👏👏👏 고민의 흔적이 여기저기 보입니다! 멋저요!👍

코멘트 주신거에 대한 제 개인적인 답변을 드리자면

앱 첫 실행시 데이터를 가지고 오기 전 까지 보여지는 로딩 뷰를 어디서 숨겨 줘야 할까요?

요건 제가 해당 코드에 코멘트 남겼는데 한번 확인해주세요!

MVC 패턴에서 view가 데이터를 가지고 계산을 하는것이 옳은가요?

음.. MVC 패턴에서도 Model-View-Controller 로 나눠져 있기 때문에, 말씀하신 계산/비즈니스 로직은 Controller 에서 담당을 하는게 맞지 않을까 싶네요! 물론 UIKit 에서는 ViewController 라는 친구가 View+Controller의 역할을 갖고 있기도 하지요ㅎㅎ
말씀하신 뷰가 어떤걸까요?? 뷰컨을 말씀하시는거라면! MVC에서는 ViewController의 책임이 되기는 할것 같네요!

DTO와 Entity?

음.. 지금 말씀하신 각각의 정의는 조금 혼동이 생겼을 수도 있을것 같아요..!
DTO는 Data-Transfer-Object 로, 소프트웨어의 여러 레이어 (저희가 만드는 IOS 앱에서는 보통 모듈이 될것 같네요) 를 넘나드는 데이터를 캡슐화한 객체가 될것 같습니다! 지금 저희 프로젝트에서는 사실 모듈화가 되어 있지 않아서..! 전통적인(?) 개념과 맞지는 않을것 같습니다,..!
Entity도 마찬가지인데요! Entity는 저희가 Core Data 에 저장하는 클래스를 말하기도 하고요..! 또 다양한 아키텍쳐에서 쓰이는 용어로, 앱에서 전반적으로 사용되는 비즈니스 규칙과 로직을 캡슐화한 객체를 Entity 라고 하기도 합니다 ~
클린 아키텍쳐 한번 고민해보시면 요런 용어들이 나오는데 이해에 큰 도움이 될것 같아요..!

wlsdud-dev added 13 commits March 19, 2024 21:40

feat: collection view 화면 배치

6dd7f22

feat: collection view cell 화면 레이아웃 구현

fe7cdf2

refactor: 박스오피스 개체 수정

67147c9

refactor: 네트워크 수정

6e8d689

feat: collection view 데이터 연결

19f2139

feat: 네비게이션바 타이틀 날짜 연결

9cda8d9

feat: uilabel extension 텍스트 색상 변경 기능 구현

26a0a51

feat: string 익스텐션 숫자 세자리 , 추가 기능 구현

a622e0e

영화 랭크 변경 라벨 기능 추가

15e82f1

feat: 로딩 뷰 구현

5709ee7

feat: 로딩 뷰 구현

ec8f920

feat: 박스오피스컨트롤러 시작 로딩뷰 적용

8429e7d

refactor: 코드 컨벤션 수정

d405c19

junbangg requested changes Mar 22, 2024

View reviewed changes

wlsdud-dev added 9 commits March 25, 2024 17:55

refactor: 숫자 변환 string 익스텐션 수정

37db680

refacotor: UIlabel 익스텐션 주석 제거

9d3620f

refactor: 상속이 불필요한 클래스 final 키워드 추가

827fe1a

refactor: BoxOfficeListCell 수정

9d9af8a

refactor: BoxOfficeListCell formatMovieLabel 메서드 수정

be26a51

불필요한 주석 제거

6b4e3bc

refactor: BoxOfficeController 로딩뷰 호출 방식 수정

0c52589

refactor: 로딩뷰 수정

c57f80e

refactor: BoxiofficeViewController 데이터 요청 메서드 수정

d9e129d

wlsdud-dev requested a review from junbangg March 26, 2024 04:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

박스오피스 앱 [STEP 3] Jin #61

박스오피스 앱 [STEP 3] Jin #61

wlsdud-dev commented Mar 20, 2024

junbangg Mar 22, 2024

wlsdud-dev Mar 25, 2024

junbangg Mar 22, 2024

wlsdud-dev Mar 25, 2024

junbangg Apr 4, 2024

junbangg Mar 22, 2024

junbangg Mar 22, 2024

wlsdud-dev Mar 25, 2024

junbangg Mar 22, 2024

junbangg Mar 22, 2024

junbangg Mar 22, 2024

wlsdud-dev Mar 26, 2024

junbangg Mar 22, 2024

wlsdud-dev Mar 25, 2024

junbangg Mar 22, 2024

wlsdud-dev Mar 25, 2024

junbangg commented Mar 22, 2024 •

edited

Loading

		/// 숫자가 세 자리 이상 넘어가면 ,를 활용하는 메서드
		func formatNumberString() -> String {

		/* 텍스트 구간 색상 변경 */
		func setTextColor(_ color: UIColor, range: NSRange) {

		/* AttributedString이 설정되어있지 않으면 생성하여 반환한다. */
		private func mutableAttributedString() -> NSMutableAttributedString? {


		requstBoxOfficeData()

		DispatchQueue.main.asyncAfter(deadline: .now() + 0.7) {

박스오피스 앱 [STEP 3] Jin #61

Are you sure you want to change the base?

박스오피스 앱 [STEP 3] Jin #61

Conversation

wlsdud-dev commented Mar 20, 2024

이번 PR에서 집중한 부분

고민되는 점

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junbangg commented Mar 22, 2024 • edited Loading

junbangg commented Mar 22, 2024 •

edited

Loading