-
Notifications
You must be signed in to change notification settings - Fork 178
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
계산기 [STEP2] 가마 #582
Open
forseaest
wants to merge
27
commits into
yagom-academy:ic_11_forseaest
Choose a base branch
from
forseaest:step2
base: ic_11_forseaest
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
계산기 [STEP2] 가마 #582
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
안녕하세요, step2 PR 드립니다. @stevenkim18
감사합니다!
궁금한 점
1. ExpressionParser 열거형과 String 익스텐션의 split()
UML을 보면서 사용자 입력을 연산식으로 파싱하고 변환하는 과정은 다음처럼 이루어진다고 생각했습니다.
먼저, ExpressionParser의
parse()
함수는 사용자 입력으로 받은 문자열을 파라미터로 받고 이를componentsByOperators()
함수로 보냅니다.componentsByOperators()
함수는 사칙연산자를 기준으로 나눈 문자열 배열을 리턴시킵니다. 다시parse()
에서는 연산자는 operators에, 숫자는 operands로 Formula 인스턴스를 만들어 리턴시키게 됩니다.이를 바탕으로 ExpressionParser의 코드를 짰지만, 여기서 입력 받은 target Character를 기준으로 문자열 배열을 리턴시키는 String 익스텐션에서 구현한
split()
이 어디서도 사용되지 않는 것을 알게 되었습니다. 또한 UML에서도 다른 객체들과 아무런 관계선이 없는 것으로 보고,split()
을 구현만 하고 사용하지 않게 되었습니다. 이런 방향성이 맞는지 궁금합니다!2. 커밋 컨벤션
함수나 변수명을 변경할 때 rename을 쓰는지 refactor를 쓰는지 궁금합니다. 코드 가독성을 보완시키는 측면에서 refactor가 맞는지, 파일명이나 폴더명을 수정하는 rename이 변수, 함수명에도 적용되는지 궁금합니다!
그리고 커밋 메시지 형식이 안 맞는 것은,, 개인 레파지토리 별로 컨벤션 통일을 시키지 않아서 헷갈렸습니다ㅠㅠ
3. 네이밍
전반적으로 함수, 변수명이 적절하게 이루어졌는지 궁금합니다! 특히 확장을 구현하는 파일명은
DoubleExtension
처럼{ 확장시키는 파일이나 타입 이름 }+Extension
으로 설정하였는데 맞는 방식인지 궁금합니다!4. 폴더 구조
MVC를 바탕으로 폴더 구조를 크게 Model, View, Controller, Extension으로 나눠 보았습니다. 계산기 관련 데이터 및 로직들은 모두 Model 폴더에 넣었는데,
CalculatorError
와 같이 에러 정의 파일도 Model에 속해야 하는지 아니면 따로 Error 폴더를 만들어 넣어야 하는지 헷갈렸습니다. 그리고 AppDelegate, SceneDelegate.swift와 Info.plist은 어디에 속해야 할지 고민하다가 별도의 폴더를 만들지 않고 냅두게 되었습니다(…) 리뷰어님은 평소에 어떤 식으로 폴더 구조를 짜시는지 궁금합니다!