-
Notifications
You must be signed in to change notification settings - Fork 164
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.
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
쥬스메이커 [STEP 2] HAMZZI, Danny #360
base: ic_11_danny
Are you sure you want to change the base?
Conversation
Conflicts: JuiceMaker/JuiceMaker/Source/Controller/JuiceMakerViewController.swift JuiceMaker/JuiceMaker/Source/View/Base.lproj/Main.storyboard
Conflicts: JuiceMaker/JuiceMaker/Source/Controller/JuiceMakerViewController.swift JuiceMaker/JuiceMaker/Source/View/Base.lproj/Main.storyboard
@@ -26,7 +26,6 @@ class FruitStore { | |||
return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FruitStore과 관련해서 아래 질문들 고민해보시고 답변달아주세요~!
1️⃣ 왜 FruitStore의 타입은 class인가요?
2️⃣ 20번째줄의 함수 네이밍이 show보다는 과일재고를 확인하는 동작을 하고 있기때문에, 네이밍을 다시 고민해보면 좋을 거 같아요!
3️⃣ Error Handling의 내용을 보시면 do-try-catch 순서로 가는데 현재 저희 코드는 그렇게 동작하지 않고 있어요
왜 그런걸까요? showFruitQuantity 함수는 Bool 타입보다는 재고가 없을 수 있다는 Error를 체크해볼 수 있을 거 같아요
이부분 고민해서 현재 FruitStore에서 발생 가능한 Error 타입도 만들어 보시고, 이를 ViewController에서 잡을 수 있도록 해보면 좋을 거 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1️⃣ 왜 FruitStore의 타입은 class인가요?
▷ 좀 더 고민 후에 이어서 답글 작성하도록 하겠습니다.
2️⃣ 20번째줄의 함수 네이밍이 show보다는 과일재고를 확인하는 동작을 하고 있기때문에, 네이밍을 다시 고민해보면 좋을 거 같아요!
▷ FruitStore 클래스 20번째줄 메소드의 성격 상 과일재고 확인하는 동작이므로, checkFruitQuantity로 변경하였습니다.
3️⃣ Error Handling의 내용을 보시면 do-try-catch 순서로 가는데 현재 저희 코드는 그렇게 동작하지 않고 있어요
왜 그런걸까요? showFruitQuantity 함수는 Bool 타입보다는 재고가 없을 수 있다는 Error를 체크해볼 수 있을 거 같아요
이부분 고민해서 현재 FruitStore에서 발생 가능한 Error 타입도 만들어 보시고, 이를 ViewController에서 잡을 수 있도록 해보면 좋을 거 같아요!
▷ checkFruitQuantity 메소드에는 FruitResultError 타입을 활용해서 Error Handling을 구성했습니다.
[FruitStore]
func showFruitQuantity(fruitsStock: [Fruit: Int], amount: Int) throws -> Bool {
for (fruit, useQuantity) in fruitsStock {
let requestFruit = useQuantity * amount
let storeFruit = fruitStorage[fruit] ?? 0
if requestFruit > storeFruit {
throw FruitResultError.outOfStock
}
}
return true
}
[JuiceMaker]
func makeJuice(juiceMenu: JuiceMenu, amount: Int) -> Result<String, FruitResultError> {
var checkResult: Bool
do {
try checkResult = fruitStore.showFruitQuantity(fruitsStock: juiceMenu.ingredients, amount: amount)
} catch FruitResultError.outOfStock {
checkResult = false
} catch {
checkResult = true
}
if checkResult {
let message = deductFruit(requestJuiceName: juiceMenu.rawValue, requestFruits: juiceMenu.ingredients, requestJuiceAmount: amount)
return .success(message)
} else {
return .failure(.outOfStock)
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3️⃣ Error Handling의 내용을 보시면 do-try-catch 순서로 가는데 현재 저희 코드는 그렇게 동작하지 않고 있어요
왜 그런걸까요? showFruitQuantity 함수는 Bool 타입보다는 재고가 없을 수 있다는 Error를 체크해볼 수 있을 거 같아요
이부분 고민해서 현재 FruitStore에서 발생 가능한 Error 타입도 만들어 보시고, 이를 ViewController에서 잡을 수 있도록 해보면 좋을 거 같아요!
다시 재구성해봤습니다.!!
▷ checkFruitQuantity 메소드에는 FruitResultError 타입을 활용해서 Error Handling을 구성했습니다.
[FruitStore]
func checkFruitQuantity(fruitsStock: [Fruit: Int], amount: Int) throws {
for (fruit, useQuantity) in fruitsStock {
let requestFruit = useQuantity * amount
let storeFruit = fruitStorage[fruit] ?? 0
if requestFruit > storeFruit {
throw FruitResultError.outOfStock
}
}
}
[JuiceMaker]
func makeJuice(juiceMenu: JuiceMenu, amount: Int) -> Result<String, FruitResultError> {
do {
try fruitStore.checkFruitQuantity(fruitsStock: juiceMenu.ingredients, amount: amount)
} catch FruitResultError.outOfStock {
return .failure(.outOfStock)
} catch {
print("알 수 없는 Error가 발생했습니다.")
}
let message = deductFruit(requestJuiceName: juiceMenu.rawValue, requestFruits: juiceMenu.ingredients, requestJuiceAmount: amount)
return .success(message)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error Handling적용하신 부분 너무 좋습니다! 다만 catch를 JuiceMaker에서 하는 이유가 있을까요? catch를 ViewController에서 하면 어떤 차이가 생길 수 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JuiceMaker 내에서 에러를 처리하면 뷰컨트롤러에서는 단순히 성공 또는 실패 메시지만 처리하게 되네요. 뷰컨트롤러에서 catch 블록을 사용하면 JuiceMaker는 예외를 던지고 예외 처리는 뷰컨트롤러에 위임하여 역할이 확실하게 구분될 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FruitStore 내부에서 과일 재고를 저장하는 변수(fruitStoreage)가 있고, 쥬스메이커 앱에서는 fruitStorage를 자주 참조하고 값을 변경하거나 보는 일이 많습니다. 이러한 특성이 있어 class 타입으로 이용합니다.
@@ -9,6 +9,10 @@ import Foundation | |||
struct JuiceMaker { | |||
private var fruitStore: FruitStore = FruitStore(initialStock: [.strawberry: 10, .banana: 10, .pineapple: 10, .kiwi: 10, .mango: 10]) | |||
|
|||
func viewFruitStock(fruitName: Fruit) -> String { | |||
return String(fruitStore.fruitStorage[fruitName] ?? 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
만약 일치하는 과일이 없다면 이도 Error가 될 수 있지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 Furit 열거형에 명시되어있지 않기 때문에, Error가 발생합니다. 이러한 Error를 방지하기 위해, Error Handling을 활용하는 방법도 생각할 수 있을 것 같습니다.
멘토링을 진행하다보니 Step1 코드와 관련하여 추가 질문을 드리게 됐어요 1️⃣ 파일의 Created by 뒤에 팀명으로 수정해주세요! |
1️⃣ 파일의 Created by 뒤에 팀명으로 수정해주세요! 2️⃣ FruitResultError는 왜 Error를 채택했을까요? 3️⃣ Fruit과 JuiceMenu의 케이스 네이밍이 유사합니다! 구분이 필요할 거 같아요 4️⃣ 계산프로퍼티와 저장프로퍼티의 차이가 무엇일까요? |
1️⃣ 파일의 Created by 뒤에 팀명으로 수정해주세요! 아래 코드로 예시를 들어보겠습니다. struct Rectangle {
var width: Double // 저장 프로퍼티
var height: Double // 저장 프로퍼티
// 계산 프로퍼티
var area: Double {
return width * height
}
}
let myRectangle = Rectangle(width: 10, height: 5)
print(myRectangle.area) // 50을 출력합니다. |
안녕하세요 HAMZZI, Danny! 고민된 부분 및 조언을 얻고 싶은 부분에 대한 코멘트 드리오니 참고해주세요!
2️⃣ 해당 프로젝트의 거의 모든 UIComponent를 JuiceMakerViewController의 @IBOutlet 변수로 선언했습니다. @IBAction은 사용되나, @IBOutlet을 굳이 사용하지 않는다고 느껴지는 몇몇의 변수가 있는데, 꼭 선언을 해야할 지 조언을 얻고 싶습니다. 간단하게 @IBOulet은 변수 선언, @IBAction은 함수 선언 정도로 생각해볼 수 있을 거 같습니다! 추가적으로 @의 의미는 아래 문서를 참고하시면 보다 이해하실때 도움이 될 거 같습니다!
3️⃣ @IBOutlet 변수를 선언할 때, Strong타입과 Weak타입을 사용하는 시기를 잘 모르겠습니다. Weak와 Strong은 결국 Reference Counting(RC)와 관련이 있습니다. 그래서 정해진 거보단 상황에 따라 고민하시고 사용하시면 됩니다. @IBOutlet 의 기본이 weak인 이유는 ViewController는 View를 이미 강한 참조를 하고 있고, 이안에 들어오는 View의 컴포넌트들을 strong으로 참조할 경우 생길 수 있는 문제점들이 존재하기 때문입니다. 4️⃣ orderJuicFailedAlert() 메소드 내부에 confirmAction 상수에 UIAlertAction를 초기화하는 과정에서 "클로저"를 활용할 수 있었습니다. 클로저에 대한 이해가 부족하여, 좀 더 이해를 하고자 하는데요. 클로저를 활용한 방법외에 다른 방법으로 FruitStockViewController로 전환하는 방법이 있을지 궁금하며, 조언을 얻고 싶습니다.! 가능합니다! instantiate를 사용하여 ViewController를 present하는 함수를 별도로 만들어서 Alert의 handler에 넣어주면 원하는 버튼을 눌렀을 때 다시 화면을 전환 시키는 식의 흐름을 짜보실 수 있습니다! 5️⃣ Class와 Struct 선택 이유
✅ 사실 제가 고민하셨으면 하는 부분은 과연 FruitStore와 JuiceMaker 모두 struct로 할 수는 없었을까요? 만약 불가능하다면 왜일까요? 이때 어떤 객체가 class로 선언이 되어야하는지 객체지향적으로 이유를 찾아보시면 좋을 거 같다고 생각했습니다! |
self.navigationItem.leftBarButtonItem = closeButton | ||
} | ||
|
||
@objc func closeModal() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네이밍으로 코드적인 네이밍보단 보다 객체지향적인 네이밍을 해주시면 좋을 거 같습니다!😊
만약 화면이 여러개면 어떤 Modal을 닫는건지 알수 없으니까요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
closeModal -> closeButtonFruitStockModal 로 변경하였습니다.
// Copyright © yagom academy. All rights reserved. | ||
// | ||
|
||
import UIKit | ||
|
||
class JuiceMakerViewController: UIViewController { | ||
private var juiceMaker: JuiceMaker = JuiceMaker() | ||
|
||
|
||
@IBOutlet var modifiedFruitStockButton: UIBarButtonItem! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
삭제해도 동작이 가능할까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 버튼을 삭제하면 해당 버튼과 연결된 동작을 더 이상 수행할 수 없게 됩니다
@IBOutlet weak var strawbananaJuiceButton: UIButton! | ||
@IBOutlet weak var mangkiJuiceButton: UIButton! | ||
@IBOutlet weak var strawberryJuiceButton: UIButton! | ||
@IBOutlet weak var bananaJuiceButton: UIButton! | ||
@IBOutlet weak var pineappleJuiceButton: UIButton! | ||
@IBOutlet weak var mangoJuiceButton: UIButton! | ||
@IBOutlet weak var kiwiJuiceButton: UIButton! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
삭제해줘도 동작이 가능할까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IBOutlet으로 연결된 UI 요소들을 삭제하면 뷰컨트롤러의 뷰와 연결되어 화면에 표시되고 있기 때문에 동작이 불가능해집니다.
@IBAction func orderStrawberryJuiceButtonClicked(_ sender: UIButton) { | ||
showJuiceHandleResult(juiceMenu: .strawberry) | ||
} | ||
|
||
@IBAction func orderBananaJuiceButtonClicked(_ sender: UIButton) { | ||
showJuiceHandleResult(juiceMenu: .banana) | ||
} | ||
|
||
@IBAction func orderPineappleJuiceButtonClicked(_ sender: UIButton) { | ||
showJuiceHandleResult(juiceMenu: .pineapple) | ||
} | ||
|
||
@IBAction func orderKiwiJuiceButtonClicked(_ sender: UIButton) { | ||
showJuiceHandleResult(juiceMenu: .kiwi) | ||
} | ||
|
||
@IBAction func orderMangoButtonClicked(_ sender: UIButton) { | ||
showJuiceHandleResult(juiceMenu: .mango) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 반복되는 코드에 대해 tag를 공부해보신 거로 알고 있습니다!
일전 말씀드린대로 tag를 사용하면 코드에서 가독성이 떨어지고, 에러 발생 시 오류를 찾기 힘들기 때문에 개인적으로 지향하는 방법은 아닙니다
다만 tag의 넘버를 별도의 enum으로 만들어서 Int rawvalue값을 활용하여 관리하는 방법도 있다는 점 참고해보시면 좋을 거 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네. 맞습니다. tag를 사용하는 방법은 가독성이 떨어질 수 있고, 코드 유지 보수가 어려울 수 있다고 생각합니다. 세레나가 말씀하신 tag 넘버 사용에 대해서 참고하여 보겠습니다.
case .success(let message): | ||
print(message) | ||
orderJuiceSucceedAlert(message: message) | ||
showFruitStockLabel() | ||
case .failure(.outOfStock): | ||
print("재고가 없습니다") | ||
orderJuiceFailedAlert() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result타입을 적절하게 활용하신 거 같습니다👍
func orderJuiceSucceedAlert(message: String) { | ||
let alert = UIAlertController(title: "쥬스메이커", message: message, preferredStyle: UIAlertController.Style.alert) | ||
|
||
let confirmAction = UIAlertAction(title: "확인", style: UIAlertAction.Style.default, handler: nil) | ||
|
||
alert.addAction(confirmAction) | ||
present(alert, animated: false, completion: nil) | ||
} | ||
|
||
func orderJuiceFailedAlert() { | ||
let alert = UIAlertController(title: "쥬스메이커", message: "재료가 모자랍니다. 재고를 수정할까요?", preferredStyle: UIAlertController.Style.alert) | ||
let confirmYesAction = UIAlertAction(title: "예", style: UIAlertAction.Style.default) { _ in | ||
self.openFruitStockViewController() | ||
} | ||
let confirmNoAction = UIAlertAction(title: "아니오", style: UIAlertAction.Style.default) | ||
alert.addAction(confirmYesAction) | ||
alert.addAction(confirmNoAction) | ||
present(alert, animated: false, completion: nil) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
order로 시작하면 alert이라는 느낌보단 주문하는 느낌이 강한 거 같아요ㅜ
보여주는 것이기 때문에 show는 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
showAlert / showErrorAlert / showOutOfStockAlert 이러한 형태로 변경하였습니다
return | ||
} | ||
|
||
let navigationController = UINavigationController(rootViewController: fruitStockViewController) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체 코멘트에 드린 내용 참고하셔서 수정해보시면 좋을 거 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리팩토링 후, 다시 PR을 해보겠습니다!
…ResultError 에 message 프로퍼티 추가, FruitStore 를 싱글턴 패턴으로 변경, 에러 핸들링 수정, JuiceMenu 케이스 네이밍 변경
안녕하세요 세레나(@serena0720 )
현재 업데이트된 풀 리퀘스트에 대하여 더 있을 수 있는 코멘트가 있다면 기다리겠습니다. |
안녕하세요. @serena0720 😆
쥬스메이커 1조 HAMZZI, Danny 입니다. 쥬스메이커 [STEP2]가 완료되어 PR 요청드립니다. 🙏
1. 고민되었던 점 🤔
재고수정 화면으로 전환할 때 방식을 어떻게 적용해야할 지 고민 했었습니다. 1가지는 NavigationController를 이용한 방식, 나머지 1가지는 Modal 방식을 사용하는 것이었습니다.
각각의 화면 방식의 특징을 확인해보니, 아래와 같았습니다.
1️⃣ NavigationController 방식의 경우, 화면의 전환이 계층 구조를 이루면서 보여줄 수 있는 방식이었습니다. (ex. 설정 앱의 화면전환 형태)
2️⃣ Modal 방식의 경우, 화면의 전환이 일시적으로 화면을 띄워줌으로써 사용자가 어떤 행위를 하게 하고 화면이 사라지는 형태의 방식이었습니다.
그래서, 재고수정 화면 기능을 봤을 때(화면이 계층 구조를 이루면서 전환이 될 필요는 없다고 생각이 되었습니다.), Modal 방식으로의 화면 전환이 더 적절할 것 같았으나, 저희 1조 구성원이 의견을 나눈 결과 2가지 방식을 사용해보기로 했습니다.
2가지 방식을 하기로 한 이유는
더불어, 화면 전환 방식에 관련해서 꼭 이러한 사항은 지켜야 하는 부분이 있으시다면 첨언 부탁드릴게요! 🙏
추가로 금일 오전에 좀 더 의견을 나눈결과, UINavigationController의 이니셜라이저를 이용해서 rootViewController를 재고수정화면으로 넣어줘서 화면전환을 이루도록 했습니다. 그런데, 화면전환형태가 모달 방식처럼 Pop-Over가 되는 것 같아 왜 그런지 확인해봐야할 것 같습니다.
2. 조언을 얻고 싶었던 점 🙏
1️⃣ 해당 프로젝트의 거의 모든 UIComponent를 JuiceMakerViewController의 @IBOutlet 변수로 선언했습니다. @IBAction은 사용되나, @IBOutlet을 굳이 사용하지 않는다고 느껴지는 몇몇의 변수가 있는데, 꼭 선언을 해야할 지 조언을 얻고 싶습니다.
2️⃣ @IBOutlet 변수를 선언할 때, Strong타입과 Weak타입을 사용하는 시기를 잘 모르겠습니다. 참고 자료를 공유해주신다면 꼭 확인하겠습니다.
3️⃣ orderJuicFailedAlert() 메소드 내부에 confirmAction 상수에 UIAlertAction를 초기화하는 과정에서 "클로저"를 활용할 수 있었습니다.
클로저에 대한 이해가 부족하여, 좀 더 이해를 하고자 하는데요. 클로저를 활용한 방법외에 다른 방법으로 FruitStockViewController로 전환하는 방법이 있을지 궁금하며, 조언을 얻고 싶습니다.!
3. FruitStore Class로 한 이유는 ?, JuiceMaker Structure 한 이유는? 🤔
1️⃣ FruitStore Class로 한 이유
2️⃣ JuiceMaker Struture로 한 이유