쥬스메이커 [STEP 2] 제이티, Ari #119

leeari95 · 2021-10-26T09:10:18Z

@codingJT @leeari95
@daheenallwhite

안녕하세요 흰!
STEP 2 코드 작성하여 PR 보내드립니다.

그리고.. 저희가 실수로 Merge되기 전에 커밋을 해버려서.. 3개 커밋이 같이 merge 되어있습니다. 죄송합니다!
혹시 저희가 놓친 부분이 있다면 번거롭더라도 한 번만 더 짚어주신다면 감사하겠습니다!

이번 피드백도 잘 부탁드립니다...! 😁

고민했던 것

Singleton 패턴을 활용
화면 이동시 이동 방식에 대한 고민
- 이유 네비게이션을 따라서 화면을 이동하는 방식이 적절하지 못하다고 생각했기 때문이다. 임시적으로 화면에 들어가서 재고를 수정하는 용도의 View라는 생각이 들었다. 따라서 Push 보다는 Present가 적합하다고 생각했다.
Human Interface Guidelines에 따른 Alert 버튼 구성
- Yes, No의 사용은 하지 말라고 되어있다.
- 단순 수락시 OK, 취소는 Cancel
- Cancel 버튼은 왼쪽에 위치해야 한다.
Alert을 메서드에 구현하여 재사용성을 고려
재고 수량이 바뀔 때마다 화면에 반영시키는 방식
- NotificationCenter를 활용
하드코딩 개선방법

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

전체적으로 네이밍을 잘 하였는지 궁금합니다.
UIButton이나 UILabel이나 네이밍을 할 때 끝에 Label이나 Button을 넣어주는 것이 널리 쓰이는 방법같아서 저희도 적용해보았습니다. 이 부분에 대한 네이밍 컨벤션에 대한 흰의 생각이 궁금합니다.
코드 부분에서 "알 수 없는 에러가 발생했습니다." 라는 문구로 Alert을 띄우는 부분들이 많이 보이는데, 이 부분을 개선할 방법을 찾지 못했어요. 조언이 필요합니다.
NotificationCenter를 이용하여 Label을 변경해주고있는데, 적절하게 잘 사용한건지 궁금합니다.
ViewController 클래스를 구현할 때 접근제어를 사용해도 되는지 궁금합니다. private 키워드를 다 붙여줘도 상관 없어보이는데, 실제로 프로젝트 할 때 개발자들이 이 부분에 대해서 고민하고 접근제어를 하는지가 궁금했습니다.
지금 viewDidLoad() 메서드에서 addObserver를 해주고 있는데요. 나중에 removeObserver도 해줘야할탠데, 그 부분은 저희끼리 고민해보았을 때 아직 배우지는 않았지만 Application life cycle에서 applicationWillTerminate 시점에서 removeObserver를 호출해주면 된다고 한번 생각을 해보았는데, 이 부분도 맞게 고민해본건지 여쭤보고 싶습니다.

- Juice 타입 rawValue 추가후 CustomStringConvertible 프로토콜 채택 - JuiceMakerViewController 클래스 내부에 orderJuiceButtonTapped 메서드 구현 - JuiceMakerViewController 클래스 내부에 juiceCompletionAlret 메서드 구현

- JuiceMakerViewController 클래스에 juiceMaker 프로퍼티 생성 - orderJuiceButtonTapped 메소드 로직 수정 - notificationAlert 메소드를 통해 버튼이 하나인 Alert를 보여주도록 구현 - outOfStockAlert 메소드를 통해 버튼이 2개인 Alert를 보여주도록 구현 - juiceCompletionAlert 메소드 삭제 (notificationAlert에 통합)

- Notification.Name changedFruitStockNotification 추가 - FruitStore 타입 내부에 changeAmount 메서드에 Notification 발송 기능 추가 - JuiceMakerViewController 내부에 Label들을 업데이트 할 수 있도록 여러 메서드 추가

@IBOutlet

- orderJuiceButtonTapped 메서드 내부에 do-try-catch문을 분리하여 mixFruit 메서드 추가 - text로 하드코딩 되어있던 switch문을 @IBOutlet을 활용하여 개선

daheenallwhite

많은 고민이 드러나는 코드네요 고생하셨습니다~👏👏👏👏

질문에 대한 답변

전체적으로 네이밍을 잘 하였는지 궁금합니다.
→ 어색한 네이밍은 없는걸로 보여요
UIButton이나 UILabel이나 네이밍을 할 때 끝에 Label이나 Button을 넣어주는 것이 널리 쓰이는 방법같아서 저희도 적용해보았습니다. 이 부분에 대한 네이밍 컨벤션에 대한 흰의 생각이 궁금합니다.
→ 이렇게 많이 쓰이죠. 길더라도 해당 ui component 가 어떤 일을 하는지 최대한 녹이는 네이밍이 좋다고 생각해요.
코드 부분에서 "알 수 없는 에러가 발생했습니다." 라는 문구로 Alert을 띄우는 부분들이 많이 보이는데, 이 부분을 개선할 방법을 찾지 못했어요. 조언이 필요합니다.
→ 해당 작업을 하는 method 로 빼면 어떨까요?
NotificationCenter를 이용하여 Label을 변경해주고있는데, 적절하게 잘 사용한건지 궁금합니다.
→ 코멘트 언급된 부분 확인해주세요
ViewController 클래스를 구현할 때 접근제어를 사용해도 되는지 궁금합니다. private 키워드를 다 붙여줘도 상관 없어보이는데, 실제로 프로젝트 할 때 개발자들이 이 부분에 대해서 고민하고 접근제어를 하는지가 궁금했습니다.
→ 접근제어 당연히 고려하고요, 뷰컨 내부의 method 에 private을 붙이는 부분 말하시는거 맞죠?
지금 viewDidLoad() 메서드에서 addObserver를 해주고 있는데요. 나중에 removeObserver도 해줘야할탠데, 그 부분은 저희끼리 고민해보았을 때 아직 배우지는 않았지만 Application life cycle에서 applicationWillTerminate 시점에서 removeObserver를 호출해주면 된다고 한번 생각을 해보았는데, 이 부분도 맞게 고민해본건지 여쭤보고 싶습니다.
→ 해당 view 에서만 쓰이는건데도 앱 종료 시점에 삭제해주는게 적절한가요? 지금은 화면이 한두개지만 여러 view controller 가 필요한 앱에서도 해당 시점이 적절한지 고민해보면 어떨까요?

daheenallwhite · 2021-10-27T07:17:04Z

JuiceMaker/JuiceMaker/Controller/JuiceMakerViewController.swift

+
+ @objc func fruitLabelChanged(notification: Notification) {
+ guard let fruit = notification.object as? Fruit else {
+ showNotificationAlert(message: "알 수 없는 에러가 발생했습니다.")


코드 전반에 걸쳐서 "알수없는~" 이 문장이 반복해서 사용되는데 따로 관리할 수 있는 법은 없나요?

daheenallwhite · 2021-10-27T07:19:46Z

JuiceMaker/JuiceMaker/Controller/JuiceMakerViewController.swift

+ let alert = UIAlertController(title: nil, message: "재료가 모자라요. 재고를 수정할까요?", preferredStyle: .alert)
+ let cancelAction = UIAlertAction(title: "Cancel", style: .cancel, handler: nil)
+ let okAction = UIAlertAction(title: "OK", style: .default) { _ in


여기 message 나 title에 들어가는 문자열들을 따로 관리할 방법이 없나요?

enum으로 따로 분리하여 관리하도록 개선하였습니다! 감사합니다!

daheenallwhite · 2021-10-27T07:21:46Z

JuiceMaker/JuiceMaker/Controller/JuiceMakerViewController.swift

+ func currentStockLabelUpdate(fruit: Fruit, label: UILabel) {
+ do {
+ let stock = try FruitStore.shared.stock(fruit: fruit)
+ label.text = String(stock)


CustomStringConvertible 을 채택해서 String(stock) 으로 표현해서 label.text 에서 사용한 이유가 있나요? stock.descriptionText 같은 스타일로 사용할 수도 있지 않나요?

daheenallwhite · 2021-10-27T07:29:55Z

JuiceMaker/JuiceMaker/Model/FruitStore.swift

 class FruitStore {
-
 static let shared: FruitStore = FruitStore()


JuiceMaker에 FruitStore 객체를 외부에서 주입해주지 않고 singleton 으로 넣은 이유가 있나요?

daheenallwhite · 2021-10-27T07:35:08Z

JuiceMaker/JuiceMaker/Controller/JuiceMakerViewController.swift

+ }
+ }
+
+ func fruitlabel(of fruit: Fruit) -> UILabel {


이 메소드는 어떤 역할을 하는 메소드인가요?

daheenallwhite · 2021-10-27T07:36:00Z

JuiceMaker/JuiceMaker/Model/FruitStore.swift

- case kiwi
- case mango
- }
-
 private var fruitBasket: [Fruit: Int]

 init(count: Int = defaultFruitCount) {


defaultFruitCount 가 FruitStore 내에서만 쓰이는데 안에서 관리하는게 낫지 않을까요?

daheenallwhite · 2021-10-27T07:50:42Z

JuiceMaker/JuiceMaker/Model/FruitStore.swift

@@ -49,6 +54,8 @@ class FruitStore {
 }
 let newFruitCount = calculator(oldFruitCount, count)
 fruitBasket[fruit] = newFruitCount
+
+ NotificationCenter.default.post(name: .changedFruitStockNotification, object: fruit)


NotificationCenter 를 사용해보셨군요
근데 stock 이 줄어들 때는 이벤트 알림을 안받나요?

아뇨 줄어들때도 알림을 받습니다!
subtractFruitStock, addFruitStock 메서드가 changeAmount 메서드를 활용하여 stock을 추가하거나 빼는 일을 하고 있습니다😁

daheenallwhite · 2021-10-27T07:52:06Z

JuiceMaker/JuiceMaker/Controller/JuiceMakerViewController.swift

+ NotificationCenter.default.addObserver(self, selector: #selector(fruitLabelChanged(notification:)), name: .changedFruitStockNotification, object: nil)
+ }
+
+ @IBAction func orderJuiceButtonTapped(_ sender: UIButton) {


모든 button 의 action method 를 개별로 구현할 때와 이렇게 하나에 구현할 때의 장단점은 무엇인가요?

daheenallwhite · 2021-10-27T07:53:27Z

JuiceMaker/JuiceMaker/Controller/JuiceMakerViewController.swift

+ let alert = UIAlertController(title: nil, message: "재료가 모자라요. 재고를 수정할까요?", preferredStyle: .alert)
+ let cancelAction = UIAlertAction(title: "Cancel", style: .cancel, handler: nil)
+ let okAction = UIAlertAction(title: "OK", style: .default) { _ in
+ guard let viewController = self.storyboard?.instantiateViewController(withIdentifier: "FruitStoreView") else { return }


재고 화면 view controller 는 왜 이름이 FruitStoreview 인가요?

해당 함수를 따로 빼면 어떤가요?

해당 부분도 피드백 반영하여 개선해보았습니다 😄
오늘 시간내서 코드리뷰 해주셔서 정말 감사합니다! 많은 도움이 되었어요!!!!! 😊🙏🏻

- Juice 타입에 채택되어있는CustomStringConvertible 프로토콜 제거 - Juice 타입 내부 프로퍼티명 description에서 name으로 수정 - Text 타입 이름을 Message로 변

…를 외부로 분리

daheenallwhite · 2021-11-02T08:46:52Z

JuiceMaker/JuiceMaker/Model/Text.swift

+ private func localizedTitle(key: String) -> String {
+ let bundle = Bundle.init(for: UIButton.self)
+ let title = bundle.localizedString(forKey: key, value: nil, table: nil)
+ return title
+ }


이 메소드는 무슨 역할을 하나요?

Apple에서 제공하는 다국어 지원 String을 사용하고자 해당 메소드를 구현했습니다!

해당 메소드를 사용하는 방법은 key-value 방식으로, 특정 키를 제공하면 해당 키에 맞는 title이 나오게 됩니다. 이 title은 localizing이 된 텍스트입니다.

예를 들어 localizedTitle(key: "Cancel") 코드가 실행되면 현재 기기의 언어가 영어로 설정되었을 때에는 "Cancel", 한국어로 설정되었을 때에는 "취소"를 리턴해줍니다.

한국어를 지원하기 위해 프로젝트의 Localization에 Korean을 추가해 주었습니다. 추가해주지 않으면 언어가 영어에서 한국어로 변경되어도 해당 사항이 적용되지 않습니다.

Apple에서 제공하는 다국어 지원 String을 사용하기 위해 Bundle의 initialilzer를 사용했습니다. Bundle이 Directory를 추상화한 개념으로 알고 있는데, 아직 Bundle의 구체적인 의미는 확인하지 못했습니다. 혹시 해당 사항 관련해서 Bundle의 구체적인 개념이 어떤 건지 여쭈어볼 수 있을까요?

NotificationCenter를 사용하지 않고 해당 기능을 구현한 것으로 인해 앱을 실행한 뒤, 기기의 언어를 변경하고 앱에 다시 돌아가면 언어가 제대로 변경되지 않는 현상이 발생할 수 있는지도 궁금합니다!

leeari95 and others added 12 commits October 25, 2021 17:08

refactor: HIG 지침에 따라 Alert의 선택지 문구 수정

293f811

refactor: Fruit타입을 FruitStore 내부에서 외부로 이동

a560ab2

refactor: JuiceMakerViewController의 UILabel 프로퍼티 이름 수정

57031d7

refactor: JuiceMakerViewController 내부 메서드명 수정

eb48bef

refactor: JuiceMakerViewController 내부 메소드 순서 변경

0331594

refactor: orderJuiceButtonTapped 메서드 분리 및 리팩토링

8edd150

- orderJuiceButtonTapped 메서드 내부에 do-try-catch문을 분리하여 mixFruit 메서드 추가 - text로 하드코딩 되어있던 switch문을 @IBOutlet을 활용하여 개선

chore: 불필요한 주석 제거

b28fab2

chore: mixFruit 메서드 내부에 에러 문구를 수정

5d1c1da

docs: README.md 파일 내용 추가

f7bb20f

daheenallwhite reviewed Oct 27, 2021

View reviewed changes

HoneyCoding and others added 5 commits October 28, 2021 15:11

refactor: 문자열을 모아놓은 enum Text 추가 및 숫자를 문자열로 형변환 하는 코드 수정

8bfece8

refactor: Juice 타입 리팩토링 및 Text 타입 이름 수정

5f09d5c

- Juice 타입에 채택되어있는CustomStringConvertible 프로토콜 제거 - Juice 타입 내부 프로퍼티명 description에서 name으로 수정 - Text 타입 이름을 Message로 변

refactor: Alert 버튼의 타이틀을 따로 Text 타입으로 구현

d252b60

refactor: FruitStore 내부에 전역변수를 enum Const 타입으로 리팩토링

0484cd1

refactor: JuiceMakerViewController 내부의 showOutOfStockAlert 메소드의 코드 일부…

494468c

…를 외부로 분리

leeari95 requested a review from daheenallwhite November 1, 2021 04:21

daheenallwhite reviewed Nov 2, 2021

View reviewed changes

daheenallwhite merged commit 5b90c88 into yagom-academy:4-leeari95 Nov 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

쥬스메이커 [STEP 2] 제이티, Ari #119

쥬스메이커 [STEP 2] 제이티, Ari #119

leeari95 commented Oct 26, 2021

daheenallwhite left a comment

daheenallwhite Oct 27, 2021

daheenallwhite Oct 27, 2021

leeari95 Oct 28, 2021

daheenallwhite Oct 27, 2021

daheenallwhite Oct 27, 2021

daheenallwhite Oct 27, 2021

daheenallwhite Oct 27, 2021

daheenallwhite Oct 27, 2021

leeari95 Oct 27, 2021

daheenallwhite Oct 27, 2021

daheenallwhite Oct 27, 2021

leeari95 Oct 28, 2021

daheenallwhite Nov 2, 2021

HoneyCoding Nov 2, 2021

HoneyCoding Nov 2, 2021

		class FruitStore {

		static let shared: FruitStore = FruitStore()

쥬스메이커 [STEP 2] 제이티, Ari #119

쥬스메이커 [STEP 2] 제이티, Ari #119

Conversation

leeari95 commented Oct 26, 2021

고민했던 것

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

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

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