Conversation
- 숫자 최대길이 20자
- Int.max 범위 넘어가는 숫자에 대해서 동작 미구현
- "00" 버튼을 최대길이 - 1에서 눌렀을 때, "0" 하나만 추가
- scrollView 최하단의 하단으로 가야함
- ViewController -> CalculatorViewController - final 키워드 추가
- extension으로 구현
- UILabel 변수명 변경 operationLabel -> operatorLabel currentNumberLabel -> entryNumberLabel - 매직 넘버 20을 maxDigitLength로 변경 - isNumberTyped를 삭제 기존에는 0을 누른 후 operator를 눌렀을 때 history에 추가하기 위해 해당 변수를 사용했으나, 숫자가 0만 있을 떄에는 history에 추가하지 않도록 작동 방식 변경
- history stack 비우기 제외 구현
- formulaString 생성 후 추적. 이를 parser에 넘겨서 result를 받아올 예정 - clear 관련 리팩토링 - 메서드 내 number 변수 네이밍 refactoring (currentNumber -> number)
…ber, displayOperator 구현 - 기존은 displayNumber가 0이 아닐 때 숫자를 추가했다. - 계산 결과에서 숫자를 누르면 추가가 아니라 새로 입력받을 수 있도록 isNumberInTyping 변수를 활용해 구현했다.
- reduce 내 current prefix 제거
- entryNumberLabel.text를 displayNumber 로 변경 - buttonName -> buttonTitle로 변수 네이밍 변경
corykim0829
left a comment
There was a problem hiding this comment.
두분 모두 수고 많으셨습니다 👏👏👏
☄️ throw Error, ResultType
result 사용 잘하셨네요!
가독성도 있지만 저는 순회를 두번할 필요가 없을 것 같아서, reduce 안에서 처리하면 좋을 것 같다고 생각한 거였습니다!
reduce와 Result를 모두 사용하면 throw를 사용하는게 과해보이는건 있네요!
차이점은 다 알고계신게 중요하다 생각합니다 :)
😴 숫자에 쉼표, NumberFormatter
이 부분은 위에를 정리하면 조금 해결될 수도 있다고 생각이 드는데요. NumberFormatter는 마지막에 Label이 표시할 때에만 처리하는 방향으로 처리해보면 좋을 것 같아요.
그 밖의 궁금중
displayNumber를 get set을 잘 활용하셔서 처리한 부분은 매우 좋았습니다! 👍
단, displayNumber 데이터를 get으로 가져올 때 entryNumberLabel에서 가져오는건 어색합니다.
컨트롤러에서 뷰에게 데이터를 넘겨주기만 하고, 뷰는 해당 데이터를 표출해주는 역할만 하는게 깔끔한 로직이라고 생각합니다.
그래서 최대한 데이터와 뷰의 역할은 분리시켜주는게 좋습니다.
지금은 서로 얽혀있는 것 같아서, 코드를 읽기 위해서 위 아래를 자주 왔다갔다 해야하는 느낌이 들고, 나중에 수정하기가 힘들어보여요!
너무 어려운 질문을 해주셨는데... 개인적으로는 그냥 코드를 쓰는게 아니고 이 코드의 이유를 알고 사용하는게 좋은 개발자라고 생각합니다. 어떻게 작동되는지 알고 사용하는게 중요하다고 생각합니다 :)
|
|
||
| final class CalculatorViewController: UIViewController { | ||
|
|
||
| private enum Constant { |
| let suffix = (displayNumber + buttonTitle).count > maxDigitLength ? Constant.zero : buttonTitle | ||
| displayNumber += suffix | ||
| case Constant.dot: | ||
| guard false == hasDisplayNumberDecimalPlaces else { return } |
There was a problem hiding this comment.
hasDisplayNumberDecimalPlaces변수는 여기에서만 쓰이고 있고,
guard문과 쓰이니 가독성이 더 떨어지는 것 같아요.
displayNumber.contains(Constant.dot)를 사용하는게 오히려 더 가독성이 좋은 것 같네요.
그와 별개로, false == hasDisplayNumberDecimalPlaces 보다는
hasDisplayNumberDecimalPlaces == false 처럼 표기하는게 일반적이 표현이라 보기 좋습니다.
또한 Bool 타입이면 굳이 == 연산자를 사용할 필요없이 사용하는게 조금 더 보기에 깔끔합니다 (가끔 예외 경우도 있지만)
개인적으로 Dot이 들어갈 경우에만 return 시키는 로직이기 때문에
if displayNumber.contains(Constant.dot) {
return
}와 같이 처리해주면 훨씬 깔끔할 것 같아요!
| displayNumber.contains(Constant.dot) | ||
| } | ||
| private var formulaString = "" | ||
| private var isNumberInTyping = false |
There was a problem hiding this comment.
문법에 맞게 isTypingNumber가 자연스러운 것 같아요
| get { entryNumberLabel.text?.replacingOccurrences(of: ",", with: "") ?? "0" } | ||
| set { entryNumberLabel.text = newValue } | ||
| } | ||
| private var displayOperator: String { | ||
| get { operatorLabel.text ?? "" } | ||
| set { operatorLabel.text = newValue } |
There was a problem hiding this comment.
get set을 잘 사용하셨네요 👍
{, } 기준으로 개행을 해주면 가독성이 더 좋아질 것 같아요
| super.viewDidLoad() | ||
| } | ||
|
|
||
| @IBAction private func digitDidTap(_ sender: UIButton) { |
There was a problem hiding this comment.
digitButtonDidTap처럼 어떤 UIView 요소를 처리하고 있는 메소드인지 명시해주면 더 좋을 것 같아요
| override func viewDidLoad() { | ||
| super.viewDidLoad() | ||
| } |
| } | ||
|
|
||
| @IBAction private func digitDidTap(_ sender: UIButton) { | ||
| guard let digit = sender.currentTitle, displayNumber.count < maxDigitLength else { return } |
There was a problem hiding this comment.
guard 의 else 뒷부분은 {, } 기준으로 확실하게 개행을 해주어서 여기서 return 된다는 것을 명시해주는게 좋습니다!
| let result = ExpressionParser.parse(from: formula).result() | ||
| switch result { | ||
| case .success(let res): | ||
| return parse(String(res)) |
There was a problem hiding this comment.
parse 함수로 인해 ,가 들어가기 때문에 더 복잡해지는 것 같아요. 제가 말씀드린 뷰와 데이터 분리를 고려해보세요!
| let stackView = HistoryStackView(operator: `operator`, operand: parsedNumber) | ||
| calculationHistoryContentView.addArrangedSubview(stackView) | ||
|
|
||
| view.layoutIfNeeded() |
There was a problem hiding this comment.
layoutIfNeeded는 왜 호출했나요? 꼭 필요한 코드인가요?
1. hasDisplayNumberDecimalPlaces 삭제 2. 사용하지 않는 viewDidLoad method 삭제
- StackView에 arrangedView가 0개일 때 AutoLayout 오류가 생겨서 1개를 남겨놓았었음 - stackView의 intrinsic size 설정 후 해당 오류가 해결되어서 기존에 사용않던 첫 번째 arrangedView 관련 코드 삭제
- displayNumber, displayLabel 과 view 의 분리 - 변수 naming isNumberInTyping -> isTypingNumber - Constant 추가 및 string literal을 상수로 대체
🧮 계산기 프로젝트
안녕하세요 코리! @corykim0829
늦은 pr 받아주셔서 감사해요 :)
설날 덕분에 고민하고 구현할 시간이 부족해서 내심 아쉬웠는데요. 어떠한 리뷰든 좋으니 마구 마구 부탁드려요!!
한 주 동안 고생 많으셨고 좋은 주말 보내세요😊
Step3: 요구 사항 및 구현 사항
요구 사항 정리
계산기
Step3
🧹 고민한 부분
☄️ throw Error, ResultType
(0, .divide)가 있다면 굳이 reduce를 하지 않아도 될 것 같아서 밖으로 뺐어요!❓ 여전히 where절은 복잡하지만, 기존 함수가 error를 throw하지 않고 result를 반환하는 지금도 위의 코드가 더 가독성이 좋을까요?
❓ 코리의 제안을 수용하면서(where절을 복잡하지 않게 수정하고) result type을 사용하는 더 가독성 좋은 코드가 있으면 제안해주세요!
😴 숫자에 쉼표, NumberFormatter
❓ overflow.. 어떻게 해결할 수 있을까요?🥹
그 밖의 궁금증
❓ 전반적으로 이렇게 했으면 더 이해하기/보기 좋았을텐데..! 싶은 부분이 있으면 편하게 알려주세요:)
❓ 코리가 생각하는 좋은 개발자란? 그리고 좋은 개발자가 되기 위해 어떤 노력을 하고 있나요?