묵찌빠게임 [STEP 1] Doogie, Eddy #114
Conversation
There was a problem hiding this comment.
Doogie, Eddy 안녕하세요~
스텝1 구현하시느라 고생 많으셨습니다 🎉
서로 같이 고민해봐요!!! 잘 부탁드립니다 ㅎㅎ 😁
칭찬 드리고 싶은 부분
- enum을 적극적으로 활용해주셨네요. 💯
- 함수가 대체적으로 한가지의 기능만 하고 있네요. 💯
- 메소드의 순서도 고민해보셨군요! 💯
고민했던 부분에 대한 저의 의견
enum을 적절히 사용한 것일까요?
- 네! 제가 보기에는 적절히 잘 활용해주신 것으로 보입니다. 다만 스위프트의 enum에는 다양한 기능을 가지고 있는데, 어떤 기능들이 있고 어떤 역할들을 하는지 한번 더 살펴보면 좋을 것 같아요.
Git - revert를 하던 와중에 충돌을 해결하던 와중에 revert가 두번되었는데 이유를 모르겠습니다.
- revert는 예전 커밋으로 다시 되돌아가기 위해 사용하는 것으로 알고 있어요. 하지만 Git에는 취소하기 위해서 사용하는 명령어가 revert 말고도 restore, reset 등이 있습니다. 두 명령어와는 다르게 revert는 이전 커밋을 삭제하지않고 유지하면서, 어떠한 커밋으로 다시 되돌아갔다는 이력을 남길 수 있는 장점이 있어요.
- revert를 할 때 아래 3가지를 확인해보면 좋을 것 같습니다.
- revert를 하기 이전에 프로젝트 파일을 수정하진 않았나요?
- 파일을 건들고 revert를 하면 충돌이 납니다.
- 충돌을 해결하고 나서 git add를 하셨나요?
- add 하지 않으면 충돌을 해결한 수정사항이 반영되지 않습니다.
- git revert --continue 명령어로 충돌을 해결했다는 사실을 알렸을까요?
- revert를 하기 이전에 프로젝트 파일을 수정하진 않았나요?
- 시간이 남는다면 reset, restore, revert는 어떤 상황에 쓰면 좋을지 알아보는 것도 좋을 것 같습니다!
receiveNumber() 함수 내부 inputNumber의 옵셔널 바인딩에 실패했을 경우의 return 값이 적절할까요?
- 말씀하신대로 에러를 의도하시고 공백을 return 해주도록 한 것이라면 별 문제 없어보입니다. 자세한건 코멘트를 남겼습니다!
해결이 되지 않은 부분에 대한 저의 의견
makeUserValue(),makeComputerValue()함수를 동일한 함수로 합쳐서 활용해보고 싶어요.
- 코멘트로 남겨보았습니다 😁
조언을 얻고 싶은 부분에 대한 저의 의견
순서도 - 평행사변형과 직사각형의 사용이 적절한지?
- 제가 봤을 땐 적절히 잘 사용해서 그려주신 것 같습니다.
- 평행사변형, 직사각형 각 기호마다 용도가 다른 것으로 알고 있습니다. 순서도 기호의 의미를 한번 알아보시면 좋을 것 같습니다. 😊
두분 너무 고생 많으셨어요...!
자세한 피드백은 코멘트로 달아드렸는데, 혹시 의견이 다른 부분이나 궁금한 점은 바로바로 말씀해주세요~ DM을 주셔도 좋습니다!
| @@ -1,10 +1,105 @@ | |||
| // | |||
| // RockPaperScissors - main.swift | |||
| // Created by yagom. | |||
| // Created by Doogie, Eddy | |||
|
|
||
| import Foundation | ||
|
|
||
| print("Hello, World!") | ||
| enum GameValue { |
There was a problem hiding this comment.
저희가 토의해본 결과로는 GameOptions 이 더 명확하다고 생각했습니다. 이것이 게임내에서 유저가 입력할 수 있는 옵션들이라고 생각해서 이를 GameValue가 아닌 GameOptions 라고 생각했는데 좀 더 명확해졌을까요 ? 🧐
| case .exit: | ||
| print("게임 종료") | ||
| case .error: | ||
| print("잘못된 입력입니다. 다시 시도해주세요.") |
There was a problem hiding this comment.
자세히 보면 중복된 문자열들이 있어요. 🤔
이 부분을 enum에다가 모아서 관리해줄 수는 없을까요?
시간이 된다면 name space를 알아봅시다 😁
There was a problem hiding this comment.
일단 처음에는 문자열들만 정리해 ResultTexts 라는 enum을 만들었고 해당 변수들을 static으로 선언해 별도로 인스턴스화 시키지 않고도 showResult 함수에서 print(ResultTexts.winText) 이런식으로 바로 사용할 수 있게 구성을 했습니다.
하지만 이렇게 선언된 switch문을 조금 더 간결하고 명확하게 표현하기 위해 ResultTexts에 해당 case가 해주는 로직을 함수로 작성해 정리하여 조금 더 명확하게 사용할 수 있게 하였습니다.
There was a problem hiding this comment.
오.. static 키워드를 활용해보셨군요.
시간이 남는다면 알아보면 좋을 것 같은 질문을 드립니다. 나중에 배우실 내용인데.. 궁금하다면 찾아보세요!
static으로 선언된 변수나 메소드는 언제 메모리에서 사라질까요?
There was a problem hiding this comment.
컴파일할때 메모리에 할당되고 프로그램이 종료될 때 메모리에서 해제된다고 생각합니다!
| print("게임 종료") | ||
| case .error: | ||
| print("잘못된 입력입니다. 다시 시도해주세요.") | ||
| showMenu() |
There was a problem hiding this comment.
와우.. 재귀함수를 사용하셨네요. 깔끔합니다! 👏🏻
재귀함수와 반복문을 선택할 때 어떤 점을 고려해야 할까요?
There was a problem hiding this comment.
재귀함수를 사용하는 이유로는 가독성을 높일 수 있고 이는 변수 사용을 줄이는 것이라고 생각합니다. 이는 변수 사용으로 인해 변경 가능할 수 있는 가능성을 줄일 수 있으므로 오류 가능성을 더 줄일 수 있다고 생각합니다. 하지만 느린 실행이라는 단점과 무한 재귀는 스택 오버플로우를 일으킬 수 있으므로 반환 조건을 명확하게 명시해줄 필요가 있다고 생각합니다.
반복문 같은 경우에는 가독성이 재귀함수보다 좋지 못하지만 스택이 아닌 힙 메모리를 사용하며 빠른 실행의 장점이 있습니다. 하지만 가독성이 조금 떨어질 수 있는 점이 단점으로 보입니다.
그러므로 선택 시 가독성이 우선이라면 재귀함수를 선택할 수 있지만 무한 재귀에 대한 주의를 해야한다고 생각하고 가독성은 떨어지지만 성능적인 부분이 고려가 필요하다면 반복문을 사용하는 것이 더 옳다고 생각합니다. 그래서 이 부분에서 성능 적으로는 큰 차이가 없는 프로젝트라고 생각해서 가독성이 우선인 재귀함수를 선택해봤습니다😇
| return .exit | ||
| default: | ||
| return .error | ||
| } |
There was a problem hiding this comment.
와우.. 튜플을 써서 승패판정을 깔끔하게 구현하셨네요.
최고에요!!! 👍🏻
| case "3": | ||
| return .paper | ||
| default: | ||
| return .error |
There was a problem hiding this comment.
컴퓨터가 에러를 반환할 것 같지는 않네요. 고정된 범위에서 랜덤한 수에 해당하는 case를 고를테니깐요.
이 부분은 열거형 GameValue 내부에서 가위, 바위, 보를 랜덤으로 반환하는 연산 프로퍼티로 구현해볼 수 있을까요?
Protocol - CaseIterable
There was a problem hiding this comment.
좋은 자료 링크 알려주셔서 감사합니다. 저희가 먼저 연산 프로퍼티를 구현하는 것을 생각하기 전에 userNumber와 computerNumber를 분리해서 스위치로 받는 함수를 하나로 통합하는 방식을 생각해서 구현해봤습니다. 그래서 이 과정에서 유저와 컴퓨터가 원하는 값으로만 스위치 되도록 구현했기에 위에 연산 프로퍼티의 필요성이 없어졌다고 생각합니다. 만약 두개로 나눴다면 computerNumber에 필요한 것은 숫자 1,2,3 이기 때문에 이를 allCount 이 부분으로 해서 3가지만 추출해서 사용하면 되는 것 같은데 현재에는 필요가 없어졌다고 생각합니다. 나중에 사용할 곳이 있으면 사용해보는 건 어떨까요? 😄
There was a problem hiding this comment.
네! 기존 함수를 활용하셔도 무방해보입니다.
저런게 있구나~ 정도 알고가면 좋을 것 같아요 😊
|
|
||
| func receiveNumber() -> String { | ||
| guard let inputNumber = readLine() else { return " " } | ||
| return inputNumber |
There was a problem hiding this comment.
옵셔널바인딩에 실패한다면 공백을 return해주는 이유가 궁금합니다.
그리고 해당 메소드를 Nil-Coalescing Operator를 활용한다면 간단히 구현이 가능할 것 같아요.
There was a problem hiding this comment.
nil 병합 연산자를 이용하니 readline을 통해 입력값을 굳이 새로운 변수명을 할당해 옵셔널 바인딩 한 후 그 바인딩 된 값을 넘겨줄 필요없이 그 값이 nil 이면 " " 값을 넘겨주는 방식을 통해 훨씬 더 간결한 코드 작성이 가능했습니다.
| } | ||
|
|
||
| func makeComputerValue() -> GameValue { | ||
| let computerNumber = makeRandomNumber() |
There was a problem hiding this comment.
컴퓨터의 패를 정하는 함수 같은데... 컴퓨터 숫자라는 네이밍이 알맞을까요?
다시 한번 고민해볼까요? 😁
There was a problem hiding this comment.
일단 number 라는 네이밍이 맞지 않고, 바꾼다면 computerHand 정도의 이름으로 바꿀 수 있을 것 같습니다.
다만, enum의 형식으로 변경하는 과정에서 발견하지 못해 수정을 하지 못하였으며 함수를 합치는 과정에서 해당 변수는 필요없게 되어 제거하게 되었습니다.
| case draw | ||
| case lose | ||
| case error | ||
| } |
There was a problem hiding this comment.
enum을 잘 활용해주신 것 같아요!
원시값도 활용해보면 더 좋을 것 같아요~ 😊
Swift Language Guide - Enumerations
| } | ||
|
|
||
| func makeUserValue() -> GameValue { | ||
| let userNumber = receiveNumber() |
There was a problem hiding this comment.
receiveNumber() 함수를 활용해서 userNumber에 값을 할당해주고 있네요.
그렇다면 이 함수의 하는 일은 뭘까요? 제가 생각하기엔 두가지인 것 같아요.
- 사용자의 입력을 받는다.
- 입력받은 문자열을 판별(가위,바위,보)하여 반환한다.
따라서 기능을 분리하는 게 좋을 듯 합니다.
만약 이 부분을 String타입의 파라미터로 받게된다면 이렇게 변수를 생성해주지 않아도 되지 않을까요?
파라미터에 따라 값을 어떻게 반환할지 결정해줄 수 있으니 해결이 되지 않은점으로 남겨주셨던 부분이 해소될 것 같아요.
여기서 질문!
파라미터를 받게 된다면 얻는 이점은 무엇무엇이 있을까요?
그리고 따로 파라미터 말고도 인수 레이블이 존재하는 이유는 뭘까요?
There was a problem hiding this comment.
makeUserValue 함수와 makeComputerValue 함수를 병합하면서 해당 함수에서 사용자의 입력값이나 컴퓨터가 생성하는 임의의 값을 받는 기능을 매개변수로 받게 되었으며 해당 함수 내부는 하나의 기능만을 구성할 수 있게 되었습니다.
매개변수로 받게될 경우 해당 함수로 예를 들었을 때 굳이 변수를 생성할 필요가 없을 뿐더러 기능을 조금 더 분리할 수 있다는 생각이 들었습니다.
인수 레이블은 함수 외부에서 봤을 때 가독성을 더 높여주기 위해 사용한다고 생각합니다.
다만 지금 작성된 코드에서는 외부 매개변수와 내부 매개변수를 구별해 줄 필요성을 느끼지 못해 따로 작성하지 않았습니다.
이 부분에 대해서는 앞으로 다른 코드를 작성하면서 필요성을 느낄 수 있지 않을까 싶습니다.
| showResult(gameResult: compareValues(userValue: makeUserValue(), computerValue: makeComputerValue())) | ||
| } | ||
|
|
||
| func showResult(gameResult: GameResult) { |
There was a problem hiding this comment.
음.. 이 부분은 파라미터 이름을 생략해줘도 될 것 같지 않나요?
https://www.swift.org/documentation/api-design-guidelines/
Argument Labels을 참고해볼까요?
There was a problem hiding this comment.
먼저 링크남겨주신 곳에서 참고해봤을 때 'Omit all labels when arguments can’t be usefully distinguished' 이 말을 보고 저부분에서 생략을 해도 될 것 같다고 생각하게 되었습니다. 그리고 저희가 말씀해주신 것을 보고 다른 부분에서도 생략할 수 있을까 찾아봤는데 func func matchedValue(_ value: String) -> GameOptions 가 원래 makeValue였던 함수인데 이 부분에서도 이 value 라는 argument label이 생략되어도 다른 사람이 보았을 때 이해할 수 있다고 생각해서 생략하고 사용했는데 괜찮은 생각일까요?? 😆
There was a problem hiding this comment.
넵 좋은 의견인 것 같습니다. 😁👏🏻
다만 함수 이름 중 Value가.. 걸리는데요. 어떤 값을 matched한다는 것인지가 명확하지 않은 것 같아요.🤔
Hand는 어떠실까요?
There was a problem hiding this comment.
묵찌빠에 대해 Hand로 표시하는 것이 더 명확하다는 좋은 의견 감사합니다. 저희가 Value라고 생각했던 이유는 묵찌빠를 제외하고 게임종료, 에러에 대한 스위치문도 존재하기 때문에 이를 Value로 보는 것이 맞지 않을까 생각하게되었습니다. 그러나 Ari 의견에는 주요 기능이 묵찌빠 부분이기 때문에 Hand로 하는 것이 더 명확하다는 말씀인건가요? 아니면 Value 라는 단어를 사용하는 것이 문제가 된다는 말씀인가요?😀
There was a problem hiding this comment.
네 저는 묵찌빠 게임이니까 value라는 네이밍 보다는 hand가 더 명확해지지 않을까 하는 생각이 들어서 의견을 드려보았어요 😁
|
|
||
|  |
There was a problem hiding this comment.
꾸준한 README 작성은 언젠가 빛을 발하게 될 거에요.
화이팅이에요! 💪🏻
There was a problem hiding this comment.
넵 STEP 2 PR 보낼 때 README 부분 알차게 채워서 보내보도록 하겠습니다! 좋은 말씀 감사합니다 ㅎㅎ
| case rock | ||
| case paper | ||
| case error | ||
| init(option: String) { |
There was a problem hiding this comment.
오.. 새로운 이니셜라이저를 만들어서 활용해보셨네요.
기본 init?(rawValue:)를 쓰지 않고 별도로 만들어준 이유가 궁금하네요.
There was a problem hiding this comment.
처음에 생각했을 때 저희가 옵셔널부분까지 error case로 다 처리했다고 생각해서 저는 실패가능성이 없는 것으로 생각했습니다. 하지만 변수명을 직접 한 것보다 아리가 말한것처럼 rawValue로 하는 방향이 맞다고 생각합니다. 그런데 지금 현재 PR 2번째에서 비슷한 init 이 구현되고 있는데 이 부분의 내부구조가 비슷해서 rawValue로 사용할 수 없기에 각각 변수명을 지정하고 switch를 사용하는 방식으로 생각해봤습니다!
| print(""" | ||
| 가위(1), 바위(2), 보(3)! <종료 : 0> : | ||
| """, terminator: " ") | ||
| showResult(compare(userOption: matchedHand(receiveNumber()), computerOption: matchedHand(makeRandomNumber()))) |
There was a problem hiding this comment.
이 부분은 길어서 가독성이 떨어지는 것 같습니다.
두가지 방법으로 개선해볼 수 있을 것 같아요.
- 줄바꿈을 해서 가독성 개선
- 파라미터에 default 값을 할당해줘서 개선 (Default Parameter Values)
어떻게 개선해볼지는 여러분들의 선택입니다.
줄바꿈의 예시에요.
| showResult(compare(userOption: matchedHand(receiveNumber()), computerOption: matchedHand(makeRandomNumber()))) | |
| showResult( | |
| compareStepOne( | |
| userOption: matchedHand(receiveNumber()), | |
| computerOption: matchedHand(makeRandomNumber()) | |
| ) | |
| ) |
There was a problem hiding this comment.
오... 함수 내부에서 줄바꿈으로 정리한다는 생각을 해본 적이 없었는데 너무 좋은 것 같습니다!
다만, default값을 할당한다는게 파라미터에 기본값을 할당해 준다는 내용으로 이해 했는데 그 기본값이 어떤게 되어야 할 지 잘 생각이 되지 않습니다!
@leeari95 안녕하세요! 첫 번째 RP 보냅니다.
리뷰해주시면 감사하겠습니다 :)
STEP 1
코드를 작성할 때 고민되었던 점
receiveNumber()함수에서 입력값에 대한 에러 처리 부분에서 단순히 공백 문자열을 출력해도 동일하게 에러로 가기 때문에 괜찮다고는 생각했지만 이게 반환 값이 있는 함수 내에서 return 의 올바른 처리인지 궁금합니다.해결이 되지 않은 점
makeUserValue(), makeComputerValue()을 두 함수로 나눴는데 사실 사용하는 내부는 동일하다고 생각합니다. 그래서 이를 동일한 코드 속에서 분리해서 다른 변수값으로만 사용하고 싶은데 이를 정확하게 해결해보지 못했습니다. 그래서 나중에 시간이 가능하다면 합쳐보는 코드를 생각해보고 싶습니다.조언을 얻고 싶은 부분