Skip to content
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] yoshikim, Marco #85

Merged
merged 26 commits into from
Jun 19, 2021

Conversation

Marcorable
Copy link
Member

@jinios

@taeyoung-Kim-KR @keeplo

안녕하세요 젤라! 🤠
날이 많이 더운데... 잘 적응중이신가요? ㅎㅎ;;
젤라에 조언 덕에 많이 고민해보고
Step1 내용도 많이 수정을 거치고 Step2까지 진행하게 되었습니다.

이번 리뷰도 잘 부탁 드립니다..

고민되었던 점 🤯

  • 7개의 버튼이 있지만 모두 같은 동작 -> 1개의 IBAction은 어떨지..
    명확하기 때문에 각각에 버튼이 같은 동작이어도 IBAction을 구현하는게 좋다!
    VS
    다른 앱이면 몰라도 현재 앱은 버튼도 정해져 있고 구분만 잘해주면 된다!
    두 방향을 디코에서 만나는 사람마다 ㅎㅎ 질문을 던져보기도하고 저희도 고민을 해보았는데
    두 사람 다 코드가 재사용 되는 형태로 구현되는 게 마음에 들어서 후자로 구현해보게되었습니다.

  • KVO 와 NotificationCenter의 사용 (공부자료이용해보기)
    KVO를 공부하고 적용시켜보기 전에 FruitStore의 프로퍼티 관리하는 부분에서
    매번 switch 문이 생성되는 부분을 편리하게 하는 방향으로 딕셔너리 프로퍼티에서 과일:재고 형태로 변형을 했었습니다.
    각각 다루는 예제도 이해하고 저희 코드에 적용해보려고 했는데
    딕셔너리 프로퍼티의 변화를 관찰하는 KVO 형태 구현은 생각보다 힘들어서 구현해내지 못하고
    Step1에서 추가된 Error Handling과 함께 성공시 NotificationCenter를 이용한 메세지 전송의 형태로 구현을 하게 되었습니다.
    최대한 MVC 디자인패턴을 적용해보고자 시도해 보았는 데
    NotificationCenter를 이용한 부분이 이런 디자인패턴에 올바르게 적용된건지 궁금합니다.

피드백 받고 싶은 부분 😹

  • guard else처리? error처리?
    특히 ViewController에서 sender를 사용하거나 NotificationCenter의 userInfo를 사용할때
    옵셔널 바인딩을 하는 자료들을 진행할때 다양한 경우에 guard 문이 구현할수 밖에 없었고 그에 맞춰 else 처리가 생겼습니다.
    특별한 에러처리를 해야할지 아니면 지금처럼 간단히 print()를 이용해서 동작에 대한 결과를 얻는 정도면 괜찮을지
    요런 경우 주로 어떻게 처리하는 방향이 좋을지 조언을 구하고 싶습니다.

  • 현재 앱처럼 간단한 UI의 업데이트
    사실 처음 아이디어는 다섯개의 UILabel을 과일의 재고가 변경 될 때마다 동시에 5개를 업데이트 하는 형태로도 구현을 했었는 데.
    왠지 한번에 하는것보다 각각 변화에 따라 업데이트 해야할 것 같아서 switch를 이용해서 나눠보았습니다.
    이런 경우에 꼭 일일이 변화에 대해 나눠줄 필요가 있는지 물어보고 싶습니다.

Marcorable and others added 21 commits June 8, 2021 14:16
Fruit,Juice를 열거형으로 생성, 쥬스 종류와 소모될 재료의 양을 가질 변수 생성
FruitStore의 각 과일(저장프로퍼티)의 현재 재고를 알려주는 currentStock메소드 생성
과일마다의 갯수를 수정하는 메소드 생성
JuiceMaker 구조체 내부에 FruitStore() 인스턴스를 하나 생성
Fruit 와 Juice enum을 이용해서 쥬스를 만드는 makeJuice() 메소드와 재고 확인을 하는 checkStock() 메소드를 구현
FruitStore의 기존 프로퍼티는 switch구문이 길어져서 딕셔너리로 변경하였습니다. 그에따라 함수도 변경하였습니다.:
기존에 튜플 또는 딕셔너리 처리하는 과정에서 호출명에 대한 고민을 for-loop의 인자명으로 해결해봤습니다.
makeJuice 함수가 주스만들기 기능만 초점이 잡히고 재고가 부족한 경우는 에러 상황으로 생각하면서 함수를 구현해봤습니다.
반환값을 옵셔널로 주거나 throw로 에러처리를 주는 방식 보다 재고 부족과 같은 상황으로 처리했습니다.
딸바쥬스 -> 딸기쥬스, 딸바쥬스 -> 망키쥬스 등등 동작에 맞는 버튼Title 변경 및 재고추가 내비게이션 제목추가
메인과 수정하는 뷰컨트롤러 분리. 과일 UI레이블 생성. UI버튼 생성 및 연결
7개의 버튼 IBAction을 하나로 구현하고 동작시 sender를 이용해서 Juice case를 얻는 형태로 구현했습니다.
성공시 alert 생성, 실패시 alert 생성
Fruit, Juice, JuiceMakerError 열거형 타입선언 코드를 따로 EnumTypes.swift 파일로 변환해놓음
성공시 쥬스이름을 키값으로 보내는 Alert 생성, 실패시 값 없이 Post Alert 생성,메인 뷰컨트롤러가 NotificationCenter 옵저버 설정
FruitStore에 과일 갯수가 바뀌는 경우 해당 과일의 수량을 화면에 띄워주는 updateUILable 함수를 구현하고 NotificationCenter의 메세지를 받아 동작하게 만들었습니다.
successAlert -> alertMakingJuiceSuccess
failAlert -> alertMakingJuiceFail
maker -> juiceMaker
store -> fruitStore
등 함수(동사형태), 변수명 명확하게 수정해봤습니다.
case mangoJuice = "망고쥬스 주문"
case kiwiJuice = "키위쥬스 주문"
case pineappleJuice = "파인애플쥬스 주문"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

각 버튼에 프로젝트 디폴트로 들어있던 ex "딸기쥬스 주문" 형태의 titleLable.text 부분을 각 case의 rawValue로 작성해 보았습니다.

} catch {
NotificationCenter.default.post(name: Notification.Name(rawValue: "makeJuiceFail"), object: nil)
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if - else 는 두 가지 상황을 주어쥐고 상황에 맞게 동작하는 느낌이 있는 반면

makeJuice() 는 주스만들기 동작이 주 기능이고 주스만들기 실패(재고없음) 인 경우가 특수한경우 또는 오류의 개념으로 접근해서 do-catch 형태로 구현하고 Error Handling을 적용해 보았습니다.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

에러핸들링을 이용해서 동작의 흐름을 제어하는 구현을 하신 점 좋습니다 👍
다만, notification을 통해서 makeJuice()의 결과를 전달하는 건 좋지 않은 방법같습니다.
성공/실패 결과에 따라서 다음 액션을 해야하는 주체에게 notification이 아닌 방식으로 전달하는 방법을 생각해보세요🧐

func currentStock(_ fruit: Fruit) -> Int {
guard let stock = fruitStocks[fruit] else {
print("재고에 존재하지 않는 과일")
return emptyQuantity
Copy link
Member Author

@Marcorable Marcorable Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분을 에러처리로 할까 고민 했었는데

emptyQuantity 상수 (0)을 리턴해주면 무조건 "재고 없음" 으로 쥬스 만들기가 실패하는 상황이
현재 guard에서 오류로 실패하는 상황과 동일한 결과라고 판단해서 이런 구현을 해보았습니다...

따로 에러처리를 하는 쪽이나 다른 방향으로 고민을 좀더 해보는게 좋을지 이런 처리도 괜찮을지 피드백 받고 싶습니다..

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 구조에서 에러처리까지는 필요 없어보입니다. 오히려 현재 stock을 정확히 알려주는것이고 현재 stock 상황에 따라 다음 액션을 하는 곳은 currentStock()을 호출하는 쪽이어서 에러처리를 한다면 이쪽에서 하는게 맞아 보여요!😃

Comment on lines 21 to 29
NotificationCenter.default.addObserver(self, selector: #selector(alertMakingJuiceSuccess(_:)),
name: Notification.Name(rawValue: "makeJuiceSuccess"), object: nil)
NotificationCenter.default.addObserver(self, selector: #selector(alertMakingJuiceFail),
name: Notification.Name(rawValue: "makeJuiceFail"), object: nil)
NotificationCenter.default.addObserver(self, selector: #selector(updateUILabel(_:)),
name: Notification.Name(rawValue: "updateUILabel"), object: nil)
for fruit in Fruit.allCases {
NotificationCenter.default.post(name: Notification.Name("updateUILabel"), object: nil, userInfo: ["과일종류":fruit])
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

객체 간 이벤트나 정보를 전달하는 방법은 여러가지 있습니다.
대표적으로 델리게이트 패턴, Notification, KVO, 핸들러 방식 정도가 있는데요
각각의 특징이 달라서 구조마다 가장 적합한 방식을 사용하면됩니다.
지금 사용해주신 Notification방식은 1 대 다수의 관계, 그리고 옵저버와 누가 이벤트를 포스팅 하는지 모를 때(서로 이어져있지 않은 관계) 에서 다수의 옵저버가 하나의 이벤트를 받아야 할 때 주로 사용되는 패턴입니다.

fruit 객체에 따라 라벨을 세팅하는 타이밍을 알기 위해 이렇게 구현 하셨을 것 같습니다. 이런 경우는 MainViewController가 fruit의 상태값이 필요할 때 fruit의 상태를 알 수 있으면 라벨의 업데이트가 가능할 것 같습니다.~

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위에서 말씀드린 객체 간 이벤트를 전달하는 방식들에 대해서 각각 특징도 한번 학습해보세요~ 앞으로 iOS를 공부하면서 중요한 부분입니다 🙂

Comment on lines +62 to +67
guard let userInfo = notification.userInfo else {
print("userInfo 에러"); return
}
guard let userInfoValue = userInfo["쥬스이름"], let juiceName = userInfoValue as? String else {
print("userInfoValue 에러"); return
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notification을 통해서 객체의 상태값을 받고있어서 옵셔널 언래핑이나 타입 체크 코드가 필요한 상황으로 보이는데,
notification이 아닌 다른 방식으로 구현한다면 이런 guard문은 없어져도 되는 코드가 되겠네요!

그리고 guard문은 원하는 조건이 아닐 때 해당 블럭을(지금으로 치면 함수겠죠?) 다 타지 않고 리턴해버려야 할 때 주로 쓰게되는데요
현재 구조에서는 userInfo값의 오류때문에 에러처리를 할 필요는 없어보입니다!

} catch {
NotificationCenter.default.post(name: Notification.Name(rawValue: "makeJuiceFail"), object: nil)
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

에러핸들링을 이용해서 동작의 흐름을 제어하는 구현을 하신 점 좋습니다 👍
다만, notification을 통해서 makeJuice()의 결과를 전달하는 건 좋지 않은 방법같습니다.
성공/실패 결과에 따라서 다음 액션을 해야하는 주체에게 notification이 아닌 방식으로 전달하는 방법을 생각해보세요🧐

func currentStock(_ fruit: Fruit) -> Int {
guard let stock = fruitStocks[fruit] else {
print("재고에 존재하지 않는 과일")
return emptyQuantity
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 구조에서 에러처리까지는 필요 없어보입니다. 오히려 현재 stock을 정확히 알려주는것이고 현재 stock 상황에 따라 다음 액션을 하는 곳은 currentStock()을 호출하는 쪽이어서 에러처리를 한다면 이쪽에서 하는게 맞아 보여요!😃

return [.mango:2, .kiwi:1]
}
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주스의 특징을 묶어서 객체화해보면 어떨까요?
주스를 만들때 필요한 재고와 주스의 종류를 나타내는 description처럼 주스의 특징을 나타내는 프로퍼티로 객체화하고 구조적으로 개선하면 재고 수량이나 description을 enum의 switch-case로 관리할 필요가 줄어들것같습니다..!

taeyoung-Kim-KR and others added 4 commits June 18, 2021 16:08
기존 딕셔너리 타입에서 프로퍼티로 다시 생성하고 KVO를 사용하기 위해 코드를 수정 하였습니다.
구조체로 변경된 Juice의 인스턴스 사용하는 형태로 변경
버튼이름을 직접 코드에 넣는 하드코딩 방식을 피하기 위해서 버튼 구별하는 방식을 변경하였습니다.
@jinios
Copy link

jinios commented Jun 18, 2021

@taeyoung-Kim-KR @Keeplo
오늘 금요일이 마지막 날이긴 한데 아직 마지막 스텝도 남았고 이번 스텝도 수정할 부분이 남았으니 주말에도 피드백 드릴게요~! 조금만 더 홧팅해주세요!!!

@Marcorable
Copy link
Member Author

오늘 금요일이 마지막 날이긴 한데 아직 마지막 스텝도 남았고 이번 스텝도 수정할 부분이 남았으니 주말에도 피드백 드릴게요~! 조금만 더 홧팅해주세요!!!

젤라 .. 죄송해요 요렇게 코멘트 남겨주신지 몰랐어요.. 감사합니다...
주말까지.. ㅠㅠ 정말 감사합니다...
저희 step2 이후로 더디지만 하나씩 꼼꼼하게 함께 공부 했는 데 생각보다 어렵고 많이 더뎌서 늦어졌어요 ㅠㅠ

@jinios
Copy link

jinios commented Jun 19, 2021

Step3 PR 올려주신게 프로젝트 완성본같아서 코멘트 대체하였습니다~
이것도 close할게요 :)

@jinios jinios merged commit 472bf71 into yagom-academy:3-marco Jun 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants