쥬스 메이커 [STEP 1] 제이티, Ari #105

leeari95 · 2021-10-19T06:59:18Z

안녕하세요 흰!
생각보다 STEP 1을 빨리 구현해서 코드에 허점이 많지 않을까 걱정이 들지만, 저희끼리 충분히 고민해서 고쳐볼 수 있는 부분을 고민하고 고쳐서 PR 보내드립니다.
저희가 놓친 부분이 있다면 번거롭더라도 한 번만 더 짚어주신다면 감사드리겠습니다.

잘 부탁드립니다...! 😁

고민했던 것

과일과 주스를 사용자 정의 타입 enum으로 구현하는 것
초기 재고 10개를 어떻게 기본값으로 초기화 해줄 것인지에 대한 고민
- Dictionary의 init(uniqueKeysWithValues:), zip
각 주스 제조에 필요한 과일의 갯수를 저장하는 방법에 대한 고민 (레시피)
- typealias Recipe
- Dictionary 활용
Juice 타입을 파라미터로 받아서 레시피에 있는 과일의 갯수를 차감해주는 방법에 대한 고민
- 레시피 내부에 과일 종류와 과일 갯수를 받아서 과일의 재고가 있는지 확인하는 것
- 재고가 부족하다면 예외처리를 하도록 구현

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

전체적으로 네이밍을 잘 하였는지 궁금합니다.
에러처리를 해주었는데, do-catch를 활용하여 에러를 처리해주는 부분은 구현하지 않았습니다. 나중에 Controller에서 사용할 때 처리해주어야 한다고 생각이 들었기 때문인데 맞는거겠죠? 아니면 Model에서 미리 구현을 해놔야하는 건지.. 헷갈립니다.
Nested Type을 잘 활용하였는지에 관해 궁금합니다.
- 현재 구현한 코드에서는 FruitStore 내부에 Fruit 타입을, JuiceMaker 내부에 Juice 타입을 구현해주었습니다. 둘다 연관된 타입이라고 생각해서 내부에 구현을 해줬는데요.
- 해당 코드대로면 예시로 Controller에서 Juice 타입의 값을 활용하고 싶을 때 JuiceMaker.Juice.strawberry와 같은 형태로 작성해야 하는데, 이게 가독성 측면에서 적절한 구현인지 궁금합니다.
init(count:) 코드에서 기본값을 명확하게 적어주기 위해 전역 변수 defaultFruitCount를 정의해주었습니다. private 접근 제어자를 이용해 전역 변수지만 외부에서 접근할 수 없도록 구현하였는데, 전역 변수를 사용하는 게 적절한지, 아니면 FruitStore 내부에서 static parameter로 정의하여 사용하는 게 좋을지(또는 그 외의 방법이 있는지) 궁금합니다!
저희가 typealias 키워드를 사용해서 Recipe라는 것을 정의 해줬는데요. Recipe라는 타입을 JuiceMaker 타입 내부에서만 사용한다는 이유로 내부에 정의를 해줬습니다. 이 부분에 대한 흰의 의견을 듣고싶습니다!
맨 처음 changeAmount 라는 메서드를 통해서 과일의 재고를 관리해주었는데요. 생각해보니 뭔가 changeAmount 메서드가 더하기, 빼기, 곱하기를 다 할 수 있어서 하는 일을 명확하게 해주기 위해 addFruitStock, subFruitStock 메서드를 별도로 구현해서 changeAmount를 활용하는 방식으로 변경해주었어요. 변경해준 이 방식이 적절한건지 판단이 잘 안서서 이 부분에 대한 흰의 의견을 듣고 싶습니다.
JuiceMaker 타입에 구현한 메소드 fruitsMixer(juice:) 코드를 아래와 같이 구현해주었습니다.
```
// JuiceMaker 타입 내부의 메소드 fruitsMixer
func fruitsMixer(juice: Juice) throws {
    guard let juiceRecipe = juiceRecipes[juice] else { return }
    guard canMakeJuice(recipe: juiceRecipe) else {
        throw RequestError.fruitStockOut
    }
    try juiceRecipe.forEach { (fruit, count) in
        try fruitStore.subFruitStock(fruit: fruit, count: count)
    }
}
```
canMakeJuice(recipe:)메서드 내부에서
hasFruitStock를 호출하여 fruitBasket[fruit]을 옵셔널 바인딩 (currentFruitAmount)
changeAmount 내부에서 fruitBasket[fruit]을 옵셔널 바인딩 (oldFruitCount)
이렇게 동일한 값을 서로 다른 메서드에서 옵셔널 바인딩을 해주고 있는데요. 즉, 이중으로 검증을 하고 있는 것처럼 보여져서 이것을 개선할 수 있을지에 대한 조언을 듣고 싶습니다.

아직 많이 부족하지만 잘 부탁드리겠습니다. 😊 🙏🏻

- 상수 프로퍼티 juiceRecipes 추가 - fruitsMixer 메소드 추가

- 프로퍼티 사이 줄바꿈 수정 - juiceRecipes 내부에 key 값에 juice 명시

- defaultFruitCount 전역변수 생성 후 이니셜라이저에서 사용 - addFruitStock, subFruitStock 생성 - changeAmount에 접근 제어자 사용 - JuiceMaker에서 사용하는 fruitStore의 메소드를 changeAmount에서 subFruitStock으로 변경

daheenallwhite

스텝1 빠르게 진행하셨네요
전반적으로 함수의 역할분리, 깔끔한 네이밍이 돋보였어요.
또 함수형 프로그래밍 관련한 Swift foundation method 를 많이 사용하셨더라구요. 함수를 method argument로 넘기는 방식도 알고 있고요

질문에 대한 답변

네이밍
네이밍은 전반적으로 깔끔하게 잘 하셨어요. 약간 어색한 부분도 있긴 한데 그건 어디까지나 영어가 저희 모국어가 아니라서 그런거라고 생각해요...ㅎㅎ 그리고 코멘트에도 달았지만 축약어 사용보다는 길어도 단어를 그대로 쓰는게 나중에 있을 오해나 잘못된 이해를 방지하는데 좋아요
에러 핸들링
요건 디엠으로 말씀드린 내용이지만 controller 에서 하시는게 역할에 더 적절할 것으로 보입니다.
nested type
절대적인 정답은 없지만 FruitStore 에서만 사용되는 거라면, 혹은 꼭 이를 통해 접근해야하는 관계라고 판단한다면 nested type 이 적절해 보이고요, 단독으로 쓰일 수 있는 혹은 자주 그렇게 쓰일 수 잇다면 같은 레벨로 빼는게 어떨까요? 나중에는 따로 쓰일 수도 있음을 언급해 주셨는데 이 경우에는 따로 빼는게 더 적절해보여요.
typalias
문제 없어 보여요. 깔끔하고 좋네요
changeAmount
add, sub 를 인터페이스 오픈하고 change~ method를 private 으로 설정해서 외부에서는 해당 operation 만 할 수 있도록 의도한 설계라고 생각했는데 맞나요? method 가 명확해서 가독성이나 사용성에 좋았어요
fruitMixer

method 는 동사로 시작하는게 좋아요. mix~ 는 어떤가요?
검증 과정이 많으면 더 안전한 것 아닌가요? 어떤 이유에서 과하다고(?) 생각했는지 좀더 설명해주세요

daheenallwhite · 2021-10-20T07:08:05Z

JuiceMaker/JuiceMaker/Model/FruitStore.swift

 class FruitStore {
+ enum Fruit: CaseIterable {


Fruit 은 FruitStore 를 통해서만 접근가능하도록 한 이유가 궁금해요

Fruit을 FruitStore 내부에서만 사용할거라고 생각했기 때문에 내부에서 정의해주었습니다😃

daheenallwhite · 2021-10-20T07:08:38Z

JuiceMaker/JuiceMaker/Model/FruitStore.swift

@@ -6,7 +6,52 @@

 import Foundation

-// 과일 저장소 타입
+private let defaultFruitCount = 10


이런 상수를 이렇게 단독으로 관리하면 어떤 장단점이 있나요?

코드 내부 가독성을 높일 수 있다는 장점 때문에 상단에 단독으로 정의해주었습니다.
만약 값을 변경하려면 직접 수동으로 수정해주어야 한다는 것, 그리고 런타임 시점에 값을 바꿀수 없다는 것이 단점이라고 생각합니다.

Count가 필요 없는 클래스나 구조체에서도 해당 상수에 접근 가능한데 그 점도 의도된건가요?

음.. 그 점은 고려해보지 못했네요..😲 와우... 감사합니다!
지금 코드상으로는 타입 모두 다른 파일에 정의가 되어있어서 말씀해주신 부분은 현 코드상에서는 문제가 되지않는다고 생각합니다. 만약 다른 타입을 만들어주게 된다면 파일을 나누어서 관리하면 될 것 같다고 생각이 드네요.
말씀하신 피드백은 꼭 참고하여 조심하도록 하겠습니다!

daheenallwhite · 2021-10-20T07:10:26Z

JuiceMaker/JuiceMaker/Model/FruitStore.swift

+ let fruitscount = Array(repeating: count, count: allFruits.count)
+


줄바꿈 하나에도 의미가 있어야 합니다. 여기서 빈줄로 남긴 이유가 있나요?

코드를 작성할땐 구지 이야기를 나누지 않았었는데요.
저와 제이티가 피드백을 받고 의견을 나누어봤을 때 각자만의 기준이 있었습니다.
의견을 나눠본 결과, 빈줄로 남겨둔 이유는 가독성 향상을 위해서 빈줄로 남겨두었습니다 !

빈줄이 가독성 향상에 도움을 준다는 근거가 있나요?

저는 단순히 코드사이 빈줄이 코드를 그룹화해서 가독성 향상에 도움이 된다는 생각을 가지고 있었는데, 흰 말씀을 듣고 근거를 찾아보니 구글의 Swift 코딩 컨벤션에 vertical whitespace라는 부분에서 논리적 하위섹션을 구성하는 용도로 빈줄을 사용하는 것을 알게되었습니다.
덕분에 빈줄 하나에도 의미와 근거를 가지고 작성해야된다는 것을 깨닫게되었네요.
저희 생각에는 변수 사이와 초기화하는 선언부 섹션을 나누려고 했고, 그게 가독성 향상에도 도움이 된다고 생각했습니다.

daheenallwhite · 2021-10-20T07:11:56Z

JuiceMaker/JuiceMaker/Model/FruitStore.swift


+ func subFruitStock(fruit: Fruit, count: Int) throws {


동사는 축약어 보다는 full 로 다 써주는게 추후에 발생가능한 혼란을 방지할 수 있어요

아.. 놓쳤던 부분인데 감사합니다! 피드백 반영하여 수정하였습니다!

daheenallwhite · 2021-10-20T07:12:21Z

JuiceMaker/JuiceMaker/Model/FruitStore.swift

+ try changeAmount(count: count, of: fruit, by: -)
+ }
+
+ private func changeAmount(count: Int, of fruit: Fruit, by calculator: (Int, Int) -> Int) throws {


함수를 받는 방식으로 구현하다니... 대단하네요

daheenallwhite · 2021-10-20T07:13:38Z

JuiceMaker/JuiceMaker/Model/FruitStore.swift

+ }
+
+ func hasFruitStock(of fruit: Fruit, count fruitCountSubtracted: Int) -> Bool {
+ guard let currentFruitAmount = fruitBasket[fruit] else { return false }


guard 문의 else block 을 한줄에 같이 쓰는 경우와 내려서 쓰는 경우의 기준이 있나요?

한줄로 적어도 가독성을 해치지 않을 경우에 한줄로 작성합니다!!

daheenallwhite · 2021-10-20T07:14:26Z

JuiceMaker/JuiceMaker/Model/JuiceMaker.swift

 struct JuiceMaker {
+ typealias Recipe = [FruitStore.Fruit: Int]


typalias 를 써서 타입에 대한 명확한 정보를 주었네요 👍

daheenallwhite · 2021-10-20T07:16:11Z

JuiceMaker/JuiceMaker/Model/JuiceMaker.swift

+ private let fruitStore = FruitStore()
+ private let juiceRecipes: [Juice: Recipe] = [
+ Juice.strawberry: [.strawberry: 16],
+ Juice.banana: [.banana: 2],
+ Juice.kiwi: [.kiwi: 3],
+ Juice.pineapple: [.pineapple: 2],
+ Juice.strawberryBanana: [.strawberry: 10, .banana: 1],
+ Juice.mango: [.mango: 3],
+ Juice.mangoKiwi: [.mango: 2, .kiwi: 1]
+ ]


initializer 를 사용하지 않고 정의부에서 바로 객체를 넣어준 이유가 있나요?

이니셜라이저로 받지않고 프로퍼티 기본값으로 주어도 별로 차이없다고 생각했었는데, 다시 생각해보니까 코드의 유연성을 위해서라도 기본값을 넣어주는 것보다는 이니셜라이저를 통해서 값을 초기화해주는게 낫다고 생각이 들었습니다.
근데 레시피는 이니셜라이저로 받기가 양이 많아서 enum 내부에 연산프로퍼티로 구현해줌으로써 로직을 바꾸었구요.
그래서 개선한 결과 JuiceMaker 이니셜라이저는 FruitStore 인스턴스만 받고 있는 형태로 변경해주었습니다.

daheenallwhite · 2021-10-20T07:16:47Z

JuiceMaker/JuiceMaker/Model/JuiceMaker.swift


+ private func canMakeJuice(recipe: Recipe) -> Bool {


가능 여부를 method 로 뺀 부분이 뜻을 명확하게 해서 코드 가독성에 도움을 주네요 👏

daheenallwhite · 2021-10-20T07:18:44Z

JuiceMaker/JuiceMaker/Model/JuiceMaker.swift

 struct JuiceMaker {
+ typealias Recipe = [FruitStore.Fruit: Int]
+
+ enum Juice {


Juice 도 JuiceMaker 의 하위개념으로 보는건가요?

음.. 이부분도 피드백을 받고 다시 고민해보니까 Juice는 나중에 메서드를 쓸때라도 외부에서 사용할 타입이라고 생각되어서 JuiceMaker 외부로 빼주었습니다. 감사합니다!

…으로 수정

- 메인 화면에서 재고 수정 버튼 터치 시 재고 추가 화면으로 이동 - 기존의 ViewController 이름을 JuiceMakerViewController로 변경 - 재고 추가 화면 구현을 위한 클래스 FruitStoreViewController 추가 - 재고 추가 화면에서 취소 버튼 터치 시 이전 화면으로 복귀 구현

leeari95 and others added 14 commits October 18, 2021 15:54

feat: FruitStore 타입 생성 후 재고를 관리하는 기능 추가

489808b

feat: JuiceMaker 타입 내부에 Juice 타입 생성 후 인스턴스 추가

c278fa0

feat: 에러처리 기능 구현

317f479

feat: JuiceMaker 기능 추가

4b5ad12

- 상수 프로퍼티 juiceRecipes 추가 - fruitsMixer 메소드 추가

refactor: changeAmount 메서드 내 guard 조건문 수정

52bccc6

feat: 주스를 만들 때 과일 재고를 확인하는 기능 추가

32a9832

refactor: JuiceMaker 타입 내부에 리팩토링

8801556

- 프로퍼티 사이 줄바꿈 수정 - juiceRecipes 내부에 key 값에 juice 명시

refactor: 접근 제어 부여

65d6fa6

refactor: fruitsMixer 내부에 recipe를 juiceRecipe로 변경

f4f3d55

refactor: fruitsMixer 메서드 내부에 print문 삭제

68ea67e

refactor: RequestError의 에러 메세지 출력 로직을 수정

7b55577

refactor: Recipe를 JuiceMaker 내부로 이동

e966ab8

docs: README.md 파일 생성

cf2dab3

daheenallwhite reviewed Oct 20, 2021

View reviewed changes

HoneyCoding and others added 9 commits October 21, 2021 15:16

refactor: JuiceMaker 타입의 juiceRecipes 프로퍼티를 Juice 타입 내부로 옮기고 switch 문…

4ff6591

…으로 수정

refactor: enum Juice와 typealias Recipe의 위치를 JuiceMaker 외부로 이동

9990dea

refactor: JuiceMaker 타입의 fruitStore 프로퍼티 기본값 제거 및 initializer 구현

9b053a9

refactor: fruitMixer 메서드 이름을 mixFruit으로 수정

87453e6

refactor: FruitStore 타입의 subFruitStock 메소드명을 subtractFruitStock으로 변경

af939bc

docs: README.md 'PR 후 개선사항' 추가

7668e47

FruitStore를 Singletone으로 변경

276ed75

refactor: 메인화면 버튼의 오타를 수정

4a677d5

daheenallwhite merged commit 1c0605d into yagom-academy:4-leeari95 Oct 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

쥬스 메이커 [STEP 1] 제이티, Ari #105

쥬스 메이커 [STEP 1] 제이티, Ari #105

leeari95 commented Oct 19, 2021 •

edited

Loading

daheenallwhite left a comment

daheenallwhite Oct 20, 2021

leeari95 Oct 20, 2021

daheenallwhite Oct 20, 2021

leeari95 Oct 20, 2021

daheenallwhite Oct 24, 2021

leeari95 Oct 25, 2021

daheenallwhite Oct 20, 2021

leeari95 Oct 21, 2021

daheenallwhite Oct 24, 2021

leeari95 Oct 25, 2021

daheenallwhite Oct 20, 2021

leeari95 Oct 21, 2021

daheenallwhite Oct 20, 2021

daheenallwhite Oct 20, 2021

leeari95 Oct 21, 2021

daheenallwhite Oct 20, 2021

daheenallwhite Oct 20, 2021

leeari95 Oct 21, 2021

daheenallwhite Oct 20, 2021

daheenallwhite Oct 20, 2021

leeari95 Oct 21, 2021

		let fruitscount = Array(repeating: count, count: allFruits.count)

		struct JuiceMaker {
		typealias Recipe = [FruitStore.Fruit: Int]

쥬스 메이커 [STEP 1] 제이티, Ari #105

쥬스 메이커 [STEP 1] 제이티, Ari #105

Conversation

leeari95 commented Oct 19, 2021 • edited Loading

고민했던 것

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

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

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 commented Oct 19, 2021 •

edited

Loading