-
Notifications
You must be signed in to change notification settings - Fork 166
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 1] 제이티, Ari #105
쥬스 메이커 [STEP 1] 제이티, Ari #105
Conversation
- 상수 프로퍼티 juiceRecipes 추가 - fruitsMixer 메소드 추가
- 프로퍼티 사이 줄바꿈 수정 - juiceRecipes 내부에 key 값에 juice 명시
- defaultFruitCount 전역변수 생성 후 이니셜라이저에서 사용 - addFruitStock, subFruitStock 생성 - changeAmount에 접근 제어자 사용 - JuiceMaker에서 사용하는 fruitStore의 메소드를 changeAmount에서 subFruitStock으로 변경
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 빠르게 진행하셨네요
전반적으로 함수의 역할분리, 깔끔한 네이밍이 돋보였어요.
또 함수형 프로그래밍 관련한 Swift foundation method 를 많이 사용하셨더라구요. 함수를 method argument로 넘기는 방식도 알고 있고요
질문에 대한 답변
- 네이밍
네이밍은 전반적으로 깔끔하게 잘 하셨어요. 약간 어색한 부분도 있긴 한데 그건 어디까지나 영어가 저희 모국어가 아니라서 그런거라고 생각해요...ㅎㅎ 그리고 코멘트에도 달았지만 축약어 사용보다는 길어도 단어를 그대로 쓰는게 나중에 있을 오해나 잘못된 이해를 방지하는데 좋아요 - 에러 핸들링
요건 디엠으로 말씀드린 내용이지만 controller 에서 하시는게 역할에 더 적절할 것으로 보입니다. - nested type
절대적인 정답은 없지만 FruitStore 에서만 사용되는 거라면, 혹은 꼭 이를 통해 접근해야하는 관계라고 판단한다면 nested type 이 적절해 보이고요, 단독으로 쓰일 수 있는 혹은 자주 그렇게 쓰일 수 잇다면 같은 레벨로 빼는게 어떨까요? 나중에는 따로 쓰일 수도 있음을 언급해 주셨는데 이 경우에는 따로 빼는게 더 적절해보여요. - typalias
문제 없어 보여요. 깔끔하고 좋네요 - changeAmount
add, sub 를 인터페이스 오픈하고 change~ method를 private 으로 설정해서 외부에서는 해당 operation 만 할 수 있도록 의도한 설계라고 생각했는데 맞나요? method 가 명확해서 가독성이나 사용성에 좋았어요 - fruitMixer
- method 는 동사로 시작하는게 좋아요. mix~ 는 어떤가요?
- 검증 과정이 많으면 더 안전한 것 아닌가요? 어떤 이유에서 과하다고(?) 생각했는지 좀더 설명해주세요
class FruitStore { | ||
enum Fruit: CaseIterable { |
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.
Fruit 은 FruitStore 를 통해서만 접근가능하도록 한 이유가 궁금해요
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.
Fruit
을 FruitStore
내부에서만 사용할거라고 생각했기 때문에 내부에서 정의해주었습니다😃
@@ -6,7 +6,52 @@ | |||
|
|||
import Foundation | |||
|
|||
// 과일 저장소 타입 | |||
private let defaultFruitCount = 10 |
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.
코드 내부 가독성을 높일 수 있다는 장점 때문에 상단에 단독으로 정의해주었습니다.
만약 값을 변경하려면 직접 수동으로 수정해주어야 한다는 것, 그리고 런타임 시점에 값을 바꿀수 없다는 것이 단점이라고 생각합니다.
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.
Count가 필요 없는 클래스나 구조체에서도 해당 상수에 접근 가능한데 그 점도 의도된건가요?
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.
음.. 그 점은 고려해보지 못했네요..😲 와우... 감사합니다!
지금 코드상으로는 타입 모두 다른 파일에 정의가 되어있어서 말씀해주신 부분은 현 코드상에서는 문제가 되지않는다고 생각합니다. 만약 다른 타입을 만들어주게 된다면 파일을 나누어서 관리하면 될 것 같다고 생각이 드네요.
말씀하신 피드백은 꼭 참고하여 조심하도록 하겠습니다!
let fruitscount = Array(repeating: count, count: allFruits.count) | ||
|
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.
코드를 작성할땐 구지 이야기를 나누지 않았었는데요.
저와 제이티가 피드백을 받고 의견을 나누어봤을 때 각자만의 기준이 있었습니다.
의견을 나눠본 결과, 빈줄로 남겨둔 이유는 가독성 향상을 위해서 빈줄로 남겨두었습니다 !
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.
저는 단순히 코드사이 빈줄이 코드를 그룹화해서 가독성 향상에 도움이 된다는 생각을 가지고 있었는데, 흰 말씀을 듣고 근거를 찾아보니 구글의 Swift 코딩 컨벤션에 vertical whitespace라는 부분에서 논리적 하위섹션을 구성하는 용도로 빈줄을 사용하는 것을 알게되었습니다.
덕분에 빈줄 하나에도 의미와 근거를 가지고 작성해야된다는 것을 깨닫게되었네요.
저희 생각에는 변수 사이와 초기화하는 선언부 섹션을 나누려고 했고, 그게 가독성 향상에도 도움이 된다고 생각했습니다.
|
||
func subFruitStock(fruit: Fruit, count: Int) throws { |
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.
동사는 축약어 보다는 full 로 다 써주는게 추후에 발생가능한 혼란을 방지할 수 있어요
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.
아.. 놓쳤던 부분인데 감사합니다! 피드백 반영하여 수정하였습니다!
try changeAmount(count: count, of: fruit, by: -) | ||
} | ||
|
||
private func changeAmount(count: Int, of fruit: Fruit, by calculator: (Int, Int) -> Int) throws { |
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.
함수를 받는 방식으로 구현하다니... 대단하네요
} | ||
|
||
func hasFruitStock(of fruit: Fruit, count fruitCountSubtracted: Int) -> Bool { | ||
guard let currentFruitAmount = fruitBasket[fruit] else { 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.
guard 문의 else block 을 한줄에 같이 쓰는 경우와 내려서 쓰는 경우의 기준이 있나요?
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.
한줄로 적어도 가독성을 해치지 않을 경우에 한줄로 작성합니다!!
struct JuiceMaker { | ||
typealias Recipe = [FruitStore.Fruit: Int] |
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.
typalias 를 써서 타입에 대한 명확한 정보를 주었네요 👍
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] | ||
] |
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.
initializer 를 사용하지 않고 정의부에서 바로 객체를 넣어준 이유가 있나요?
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.
이니셜라이저로 받지않고 프로퍼티 기본값으로 주어도 별로 차이없다고 생각했었는데, 다시 생각해보니까 코드의 유연성을 위해서라도 기본값을 넣어주는 것보다는 이니셜라이저를 통해서 값을 초기화해주는게 낫다고 생각이 들었습니다.
근데 레시피는 이니셜라이저로 받기가 양이 많아서 enum 내부에 연산프로퍼티로 구현해줌으로써 로직을 바꾸었구요.
그래서 개선한 결과 JuiceMaker
이니셜라이저는 FruitStore
인스턴스만 받고 있는 형태로 변경해주었습니다.
|
||
private func canMakeJuice(recipe: Recipe) -> Bool { |
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.
가능 여부를 method 로 뺀 부분이 뜻을 명확하게 해서 코드 가독성에 도움을 주네요 👏
struct JuiceMaker { | ||
typealias Recipe = [FruitStore.Fruit: Int] | ||
|
||
enum Juice { |
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.
Juice 도 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.
음.. 이부분도 피드백을 받고 다시 고민해보니까 Juice는 나중에 메서드를 쓸때라도 외부에서 사용할 타입이라고 생각되어서 JuiceMaker
외부로 빼주었습니다. 감사합니다!
- 메인 화면에서 재고 수정 버튼 터치 시 재고 추가 화면으로 이동 - 기존의 ViewController 이름을 JuiceMakerViewController로 변경 - 재고 추가 화면 구현을 위한 클래스 FruitStoreViewController 추가 - 재고 추가 화면에서 취소 버튼 터치 시 이전 화면으로 복귀 구현
@daheenallwhite
@codingJT @leeari95
안녕하세요 흰!
생각보다 STEP 1을 빨리 구현해서 코드에 허점이 많지 않을까 걱정이 들지만, 저희끼리 충분히 고민해서 고쳐볼 수 있는 부분을 고민하고 고쳐서 PR 보내드립니다.
저희가 놓친 부분이 있다면 번거롭더라도 한 번만 더 짚어주신다면 감사드리겠습니다.
잘 부탁드립니다...! 😁
고민했던 것
enum
으로 구현하는 것Dictionary
의init(uniqueKeysWithValues:)
,zip
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:)
코드를 아래와 같이 구현해주었습니다.canMakeJuice(recipe:)
메서드 내부에서hasFruitStock
를 호출하여fruitBasket[fruit]
을 옵셔널 바인딩 (currentFruitAmount)changeAmount
내부에서fruitBasket[fruit]
을 옵셔널 바인딩 (oldFruitCount)이렇게 동일한 값을 서로 다른 메서드에서 옵셔널 바인딩을 해주고 있는데요. 즉, 이중으로 검증을 하고 있는 것처럼 보여져서 이것을 개선할 수 있을지에 대한 조언을 듣고 싶습니다.
아직 많이 부족하지만 잘 부탁드리겠습니다. 😊 🙏🏻