쥬스메이커 [STEP 3] 제이티, Ari #135

leeari95 · 2021-11-02T12:15:04Z

PR

안녕하세요 흰!
STEP 3 코드 작성하여 PR 보내드립니다.
이전 피드백 때 오버스펙으로 구현된 코드는 삭제 처리하여 반영하였습니다! 감사합니다!
그리고 이번엔 고민했던 부분을 상세히 적어보았는데 보시기 편할지 모르겠네요...ㅎㅎ 🤔
이번 피드백도 잘 부탁드리겠습니다! 혹시나 저희가 놓친 부분이 있다면 더 짚어주신다면 감사하겠습니다! 😊

고민했던 것

재고 수정 화면으로 전환시 재고 관련 Label들이 업데이트 되는 부분을 고민했습니다. 따로 재고수정 화면에서 재고를 조회하고 Label에 반영해주는 방법도 있겠지만 같은 일을 두번 반복하는 것이 좋지않아보였고, JuiceMakerViewController의 Label의 값들을 다음 화면에다가 넘겨주면 좋겠다고 생각하여 prepare메소드를 이용하여 넘겨주는 방식을 선택했습니다.
Controller 내부 가독성을 어떻게 하면 향상 시킬 수 있는지 고민해보았습니다. 저번 피드백때 말씀드려보았던 extension을 이용하여 Controller를 분리해보았는데요. 실제로 가독성도 좋아지는 것 같은 효과[?]를 보았습니다.
초기에는 Stepper가 터치될 때 (Event가 Touch Up Inside일 때) FruitStore의 과일 갯수 저장 프로퍼티 값이 바뀌도록 IBAction Method 메소드를 작성해 구현했습니다. 해당 방식으로 구현하면 Stepper의 값이 변화하지 않았을 때에도 IBAction Method를 사용하게 되어 메소드 내부에서 수동으로 값의 변화를 체크해야 했습니다. 자동으로 값의 변화를 체크해주는 방법을 애플에서 제공해주지 않을까 생각이 들었고 Event를 Touch Up Inside에서 Value Changed로 바꾸어주어 해결했습니다.
Label과 Button이 Dynamic Type 크기에 따라 커지고 작아질 때마다 실시간으로 반영될 수 있도록 adjustsFontForContentSizeCategory 옵션을 true로 주어보았습니다.
Label과 Button이 Dynamic Type 크기에 따라 폰트 크기가 변화할 때마다 글씨가 잘리는 현상을 발견했습니다. 그래서 버튼 크기 너비에 맞게 크고 작아질 수 있도록 AdjustsFontSizeToFitWidth 옵션을 true로 주어보았습니다.

궁금했던 것 / 조언을 얻고 싶은 부분

화면 전환시 Label을 넘겨줄때 prepare 메소드를 이용하고 있는데요. FruitStoreViewController에서 따로 임시로 값을 담을 프로퍼티를 만들고 거기에 값을 담아서 Label.text에 값을 전달해주는 방식이 좋은건지 아니면 지금처럼 loadViewIfNeeded 메소드를 이용하여 미리 View를 load 시켜주고 값을 전달하는 방식이 적절한건지 고민되었습니다. loadViewIfNeeded 메소드를 적절하지 못한 타이밍에 사용하게 되면 로드될 필요가 없는 View가 로드되는 부작용이 발생할 것이라고 생각이 들었지만 저희는 적절한 타이밍에 사용했다고 생각했는데, 흰의 의견이 궁금합니다.
Step 2를 구현할 때 FruitStore 타입에 Singleton 패턴을 적용하여 코드를 구현 했었습니다. 오늘 의존 모둠간의 논의에서 Singleton으로 구현한 것이 Step 1의 요구 사항 "JuiceMaker는 FruitStore를 소유하고 있습니다"를 어긴 것같다는 의견이 나왔습니다. 저희가 작성한 코드에서 JuiceMaker 내부에 fruitStore 프로퍼티가 있고 해당 값을 singleton의 값으로 초기화해주고 있는데 이걸 JuiceMaker가 FruitStore를 소유하고 있다고 말할 수 있을까요?
```
struct JuiceMaker {
    private let fruitStore: FruitStore
    
    init(fruitStore: FruitStore = FruitStore.shared) {
        self.fruitStore = fruitStore
    }
		// 그 외 코드...
}
```

- JuiceMakerViewController에 prepare 메소드 구현 - 현 재고의 갯수를 재고수정 화면에도 연동 기능 추가 - FruitStoreViewController 내부에 Stepper setup 관련 메소드 3개 구현

- stepperTapped 내부에 계산하는 로직 추가 - 계산과 관련된 메서드 3개 추가 (fruitStockChange, chooseCalculator, calculateFruitStock)

…(of:) 추가

… 오류 수정

daheenallwhite

뷰를 처음 구현해보는건데 고생 많으셨어요👏👏👏

리마인드

method 네이밍 신경써주세요.

고민에 대한 답변

둘다 데이터를 어떻게 관리할지에 대한 내용인 걸로 보이는데 한번 같이 의논하는 자리가 있었으면 좋겠네요. 지금 구현된 방식에 대한 이야기 먼저 나눠야 적절한 답변이 가능할 것 같아요

daheenallwhite · 2021-11-03T11:36:35Z

JuiceMaker/JuiceMaker/Controller/FruitStoreViewController.swift

+ currentStockStepperValueUpdate(fruit: .mango, stepper: mangoStockStepper)
+ }
+
+ func currentStockStepperValueUpdate(fruit: Fruit, stepper: UIStepper) {


method 는 동사로 시작하는게 좋아요. update로 시작하면 어떨까요?

앗... 리마인드 해야겠네요.. 메소드는 동사로 🥲
수정완료했습니다!

daheenallwhite · 2021-11-03T11:37:09Z

JuiceMaker/JuiceMaker/Controller/FruitStoreViewController.swift

+
+// MARK: Stepper Operation
+extension FruitStoreViewController {
+ func fruitStockChange(fruit: Fruit, value: Double) {


여기도 네이밍 동사로 변경하면 어떨까요

수정완료했습니다!! 감사합니다!

daheenallwhite · 2021-11-03T11:38:07Z

JuiceMaker/JuiceMaker/Controller/FruitStoreViewController.swift

+ }
+
+ func chooseCalculator(fruit: Fruit, oldStockValue: Int, newStockValue: Int) {
+ switch oldStockValue < newStockValue {


이 로직은 if 로 해도 되지 않나요? switch 를 사용한 이유는요?

switch로 사용한 이유는 if보다 좀더 좋아보여서 사용했는데, 다시 생각을 나눠보니까 if로 해도 상관없다고 판단되서 수정해주었습니다!

daheenallwhite · 2021-11-03T11:39:07Z

JuiceMaker/JuiceMaker/Controller/JuiceMakerViewController.swift

+
+// MARK: - Setup Label and Button
+extension JuiceMakerViewController {
+ func buttonLabelFontSizeFix() {


method 이름 동사로 시작 안함

해당 동작은 storyboard 에서도 할 수있지 않나요? 코드로 작업한 이유는요?

해당 동작은 스토리보드에서 하는 방법을 찾지 못했어요. 코드로 밖에 설정할 수 없겠다고 판단하고 코드로 작업해주었습니다. 🥲

daheenallwhite · 2021-11-03T11:40:41Z

JuiceMaker/JuiceMaker/Controller/JuiceMakerViewController.swift

+}
+
+// MARK: - Transition
+extension JuiceMakerViewController {
 private func presentFruitStoreViewController(_ action: UIAlertAction) {


파라미터로 action 은 왜 받는건가요?

let okAction = UIAlertAction(title: Text.ok.title, style: .default, handler: presentFruitStoreViewController) private func presentFruitStoreViewController(_ action: UIAlertAction) { // 내부 코드 구현 }

UIAlertAction의 이니셜라이저는 handler로 받는 클로저가 UIAlertAction 파라미터를 받도록 되어 있습니다. handler로 presentFruitStoreViewController 메소드를 전달해줄 때 해당 메소드에서 action을 파라미터로 받지 않으면 문법적 오류가 발생하여 이렇게 구현해주었습니다!

presentFruitStoreViewController 메소드에 action 파라미터를 받지 않도록 구현하려면 아래와 같이 구현할 수도 있습니다.

let okAction = UIAlertAction(title: Text.ok.title, style: .default) { action in presentFruitStoreViewController() } private func presentFruitStoreViewController() { // 내부 코드 구현 }

daheenallwhite · 2021-11-03T11:42:08Z

JuiceMaker/JuiceMaker/Controller/JuiceMakerViewController.swift

+ }
+
+ func setupNextViewLabel(of nextViewController: FruitStoreViewController) {
+ nextViewController.loadViewIfNeeded()


loadViewIfNeeded method 가 무슨 역할을 하나요?

왜 외부에서 해당 viewcontroller 를 설정해주나요?

- FruitStore에서 NotificationCenter에 전달하는 object를 기존의 fruit에서 fruit과 stock을 함께 보내도록 수정 - JuiceMaker의 fruitStore에 붙어 있는 private access control을 제거 - JuiceMakerViewController에서 Notification을 통해 받아오는 object의 값이 수정됨에 적합하게 코드 수정 - updateCurrentStockLabel 메소드 내부의 singleton 사용 코드 제거 - updateFruitLabels 메소드 내부에서 stock 값을 직접 받아 사용하도록 수정

- FruitStore 내부 싱글톤 구현을 제거함 - JuiceMaker 내부 init을 싱글톤이 아닌 FruitStore init() 호출로 수정 - JuiceMakerViewController 내부 화면 전환 로직 수정 - FruitStoreViewController 내부 JuiceMaker 인스턴스를 저장하는 프로퍼티 추가 - FruitStoreViewController 내부에서 Singleton을 활용했던 부분 전체적으로 수정

leeari95 · 2021-11-05T06:30:16Z

안녕하세요. 흰!

오늘 오전부터 일찍 디코에서 코드리뷰 해주셔서 정말 감사했습니다. 덕분에 많은 깨달음을 얻었어요.

이번 개선으로 인해서 추가적으로 궁금한 부분이 생겨서 코멘트 남겨드립니다. 😊 🙏🏻

개선사항

말씀해주셨던 첫번째 뷰컨에서 두번째 뷰컨의 프로퍼티의 값을 관리하고 있었던 부분을 수정하여 juiceMaker 인스턴스를 넘겨주는 방식으로 수정했습니다. 수정하는 과정에서 싱글톤 패턴은 서로 의논하여 없애기로 결정하였고 싱글톤을 활용해서 데이터를 가져왔던 부분들을 전체적으로 개선해주었습니다.
UIButton의 titleLabel을 설정해주는 부분을 extension을 활용하여 메소드를 구현하고 해당 메소드를 통해서 설정하도록 개선해보았습니다.

궁금했던 것 / 조언을 얻고 싶은 부분

저희가 현재 FruitStoreViewController와 JuiceMakerViewController에서 중복되는 메서드가 몇개 존재하는데요. 해당 부분을 어떤 방식으로 개선해나가면 좋을지 조언해주셨으면 합니다. 조언을 얻기전에 저희끼리 생각해봤던건 따로 클래스를 만들어서 상속을 통해 해결해줄 수도 있을 것 같다는 생각을 나누어보았습니다.
그리고 FruitStoreViewController에서 현재 juicemaker가 옵셔널인 상태여서 인스턴스를 사용하는 곳에서 모두 옵셔널 바인딩 처리를 해주고 있습니다. 이것 또한 중복으로 일처리를 한다는 느낌이 있어서 개선해주고 싶은데 어떤 방식으로 개선해봐야할지 감이 오지 않습니다. 고민해본 것은 메소드로 따로 옵셔널 바인딩한 인스턴스를 반환해주는 방식과 연산 프로퍼티를 이용하여 반환해주는 방법을 고민해보았는데 둘다 좋지못하다는 생각이 들어서 흰의 조언이 필요합니다. 🥲

지금 Stock을 한꺼번에 변수에 담아서 모든 Label을 셋팅해주고 있는 메서드인데요. 저랑 제이티가 서로 의견이 달라서 흰의 의견도 궁금해서 여쭤봅니다.

제이티: 한꺼번에 셋팅해서 하나라도 try를 실패하게 되면 모든 레이블이 실패하도록 로직을 구성

func updateFruitLabels() {
    guard let fruitStore = juiceMaker?.fruitStore else { return }
    do {
        let strawberryStock = try fruitStore.stock(fruit: .strawberry)
        let bananaStock = try fruitStore.stock(fruit: .banana)
        let pineappleStock = try fruitStore.stock(fruit: .pineapple)
        let kiwiStock = try fruitStore.stock(fruit: .kiwi)
        let mangoStock = try fruitStore.stock(fruit: .mango)
        
        updateCurrentStockLabel(stock: strawberryStock, label: strawberryStockLabel)
        updateCurrentStockLabel(stock: bananaStock, label: bananaStockLabel)
        updateCurrentStockLabel(stock: pineappleStock, label: pineappleStockLabel)
        updateCurrentStockLabel(stock: kiwiStock, label: kiwiStockLabel)
        updateCurrentStockLabel(stock: mangoStock, label: mangoStockLabel)
    } catch {
        guard let requestError = error as? RequestError else { return }
        showNotificationAlert(message: requestError.errorDescription)
    }
}

아리: 코드의 재사용성을 생각해서 파라미터를 전달 받아 do-try-catch를 하나씩 실행하도록 로직을 구성.. 말로 설명하기가 어려워서 예제코드를 첨부합니다.

func updateFruitLabel(fruit: Fruit, label: UILabel) {
    guard let fruitStore = juiceMaker?.fruitStore else { return }
    do {
        updateCurrentStockLabel(stock: fruitStore.stock(fruit: fruit), label: label)
    } catch {
        guard let requestError = error as? RequestError else { return }
        showNotificationAlert(message: requestError.errorDescription)
    }
}
func updateFruitLabels() {
    updateFruitLabels(fruit: .strawberry, label: strawberryStockLabel)
    updateFruitLabels(fruit: .banana, label: bananaStockLabel)
    updateFruitLabels(fruit: .pineapple, label: pineappleStockLabel)
    updateFruitLabels(fruit: .kiwi, label: kiwiStockLabel)
    updateFruitLabels(fruit: .mango, label: mangoStockLabel)
}

어떤 로직이 더 좋다고 판단하기가 어렵더라구요.. 서로가 주장하는 논리에 동의를 하는 상황인데, 둘중에 뭐가 더 좋은지 딱 판단이 서질 않아서 조언을 구해봅니다.

leeari95 and others added 16 commits November 1, 2021 17:18

feat: FruitStore에서 Label과 Stepper의 값 연동 및 Stepper 터치 시 Label 값 변환 구현

1e85463

feat: Stepper를 클릭시 재고가 계산되도록 기능 구현

df7531a

- stepperTapped 내부에 계산하는 로직 추가 - 계산과 관련된 메서드 3개 추가 (fruitStockChange, chooseCalculator, calculateFruitStock)

fix: FruitStore의 잘못된 조건문 수정

23a4bdc

fix: 재고 부족시 재고수정으로 넘어갈 때 Label 반영 안되는 버그 수정

febd113

refactor: JuiceMakerViewController의 중복 코드를 빼주어 메소드 setupNextViewLabel…

bd2ac42

…(of:) 추가

refactor: stepperTapped 내부 가독성을 위해 fruitLabelText 메소드 추가하여 리팩토링

9b545cf

fix: Storyboard에서 JuiceMakerViewController Scene의 Content Priority 충돌…

e71fa4f

… 오류 수정

fix: Button titleLabel이 버튼의 너비에 fit되도록 설정, 글꼴 자동 업데이트 설정

b9d4c03

refactor: JuiceMakerViewController의 코드를 extension을 사용해 분리

4864c3c

refactor: FruitStoreViewController 내부 extension으로 분리

93bf54e

fix: Storyboard의 Stepper와 연결된 stepperTapped 메소드의 Sent Events 변경

484a0a1

chore: 의미없는 줄바꿈과 프로퍼티 삭제

2b397db

chore: 기존 코드에서 오버 스펙으로 구현한 부분 제거

1fec18c

docs: README.md 파일 내부에 STEP 3 관련 내용 추가

da64fc6

docs: README.md 파일 수정하다가 빠진 STEP 2 내용 다시 추가

ccb8a10

daheenallwhite reviewed Nov 3, 2021

View reviewed changes

HoneyCoding and others added 9 commits November 4, 2021 14:52

refactor: 메소드 네이밍 동사로 시작하도록 수정

ae7582e

docs: READMD.md 파일 내용 추가

3a3c801

refactor: chooseCalculator 메소드 내에서 switch 문 사용을 if 문 사용으로 수정

82c2aa5

refactor: currentStockLabelUpdate 메소드 이름을 동사로 수정

119b6d6

feat: UIButton에 extension을 통해 메소드 추가 및 기존 코드 일부 수정

ba4de9c

refactor: fruitLabelChanged 메소드 이름을 fruitStockChanged로 수정

6a7ae09

feat: FruitStoreViewController가 deinit될 때 Notification 제거 코드 추가

dc428fe

docs: README.md 내부 개선사항 내용 추가

981c869

leeari95 force-pushed the 4-leeari95 branch from f65d71b to 981c869 Compare November 5, 2021 08:17

docs: README.md 수정

abef023

docs: README.md 내부 수정

827e98b

leeari95 requested a review from daheenallwhite November 7, 2021 04:03

docs: README.md 내부 오타와 UML 이미지 수정

4809668

yagom closed this Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

쥬스메이커 [STEP 3] 제이티, Ari #135

쥬스메이커 [STEP 3] 제이티, Ari #135

leeari95 commented Nov 2, 2021

daheenallwhite left a comment

daheenallwhite Nov 3, 2021

leeari95 Nov 4, 2021

daheenallwhite Nov 3, 2021

leeari95 Nov 4, 2021

daheenallwhite Nov 3, 2021

leeari95 Nov 4, 2021

daheenallwhite Nov 3, 2021

leeari95 Nov 4, 2021 •

edited

daheenallwhite Nov 3, 2021

HoneyCoding Nov 4, 2021

daheenallwhite Nov 3, 2021

leeari95 commented Nov 5, 2021 •

edited

쥬스메이커 [STEP 3] 제이티, Ari #135

쥬스메이커 [STEP 3] 제이티, Ari #135

Conversation

leeari95 commented Nov 2, 2021

PR

고민했던 것

궁금했던 것 / 조언을 얻고 싶은 부분

daheenallwhite left a 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

leeari95 Nov 4, 2021 • edited

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

leeari95 commented Nov 5, 2021 • edited

개선사항

궁금했던 것 / 조언을 얻고 싶은 부분

leeari95 Nov 4, 2021 •

edited

leeari95 commented Nov 5, 2021 •

edited