Skip to content
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

[Round2] Step3 - kingkiboots #758

Merged

Conversation

kingkiboots
Copy link

@kingkiboots kingkiboots commented Jun 22, 2024

@jaemuYeo
안녕하세요! Round2 Step3 Pull Request 작성하여 요청드립니다!

고민했던 점

for in 이 아닌 while

  • 요구사항 중에 “새로운 로또 당첨 번호를 생성하면, 직전 회차에 1을 더해 금번 회차를 만들어낸다.” 라는 내용이 있었습니다.
  • For I in 1…5 하는 것이 제게 제일 익숙하긴 했지만 직전 회차에 1을 더하기 위해서는 while문을 사용하는 것이 요구사항에 더욱 적합할 것이라고 생각되었습니다.
var index: Int = 0
while index < 5 {
   index += 1
   let lottoNumbers: Set<Int> = generateLottoNumbers()
   let lottoHistoryIndex: String = combineIndexWithLottoHistorySuffix(index)
   lottoHistories[lottoHistoryIndex] = Array(lottoNumbers).sorted()
}

Dictionary에 들어갈 key를 만드는 함수

  • Dictionary에 값을 저장하는 것과 관련하여 이런 요구사항이 있었습니다.
  • 회차와 Lotto번호들을 저장하는 Dictionary 타입의 변수를 생성합니다.
    * "1회차": [1, 2, 3, 4, 5, 6] 와 같은 Key와 Value를 가집니다.
  • 현재 제가 작성한 코드에서 lottoHistories에 들어가는 key인 "n회차"를 생성하고 사용하는 곳은 두 함수 입니다. 하나는 Dictionary에 값을 추가하는 함수, 다른 하나는 Dictionary에서 값을 추출하는 함수이죠
func generateLottoHistories() -> Dictionary<String, [Int]> {
    ...
    let lottoHistoryIndex: String = "\(index)회차"
    ...
}

func findLottoHistory(at index: Int) {
    ...
    let lottoHistoryIndex: String = "\(index)회차"
    ...
}
  • generateLottoHistories, findLottoHistory 함수에서 "(index)회차" 이렇게 Dictionary에 들어갈 key를 만들고 있는데 만약 한곳에서 회차 라는 접미사를 다른 글자로 바꾸게 된다면 어떻게 될까 생각해보았습니다.
    • 한 곳을 바꾼다면 다른 곳도 바꿔줘야 합니다. 분명 key 는 같아야 하는데 접미사가 바뀌면 두 곳 모두 바꿔줘야 하니 일을 두번 하는 것입니다.
    • 만약 그렇지 않다면 index는 같아도 key 가 일치하지 않아서 Dictionary에 저장된 로또 당첨 번호를 확인할 수 없을 것입니다.
    • 그렇기 때문에 key를 생성하는 근원이 두 개이므로 휴먼 에러 없이 Dictionary에 저장된 로또 당첨 번호를 확인할 수 있다고 보장할 수가 없습니다.
  • 이런 이유로 key를 생성하는 하나의 근원, 즉 공통(?)함수를 만들어주면 좋겠다고 생각했습니다. 만약 "회차" 라는 접미사가 바뀌게 된다면 해당 함수만 변경해주면 되기 때문이죠.

클로저 사용해보기

  • 위 섹션에 이어, Dictionary에 들어갈 key 생성하는 하나의 함수를 만들어보자고 생각하였는데, 피드백 세션에서 클로저 이야기를 들은 것이 생각났습니다. 그래서 이름 있는 클로저, 즉 함수가 아닌 근본(?) 클로저 방식으로 Dictionary에 들어갈 key 생성하는 하나의 함수를 작성하였습니다.
let combineIndexWithLottoHistorySuffix = { (index: Int) -> String in
    let lottoHistorySuffix = "회차"
    return "\(index)\(lottoHistorySuffix)"
}

func generateLottoHistories() -> Dictionary<String, [Int]> {
    ...
    let lottoHistoryIndex: String = combineIndexWithLottoHistorySuffix(index)
    ...
}

func findLottoHistory(at index: Int) {
    ...
    let lottoHistoryIndex: String = combineIndexWithLottoHistorySuffix(index)
    ...
}

조언을 얻고 싶은 부분

함수 vs 클로저

  • 함수와 클로저가 근본은 같다는 것은 이해가 된 것 같지만,,, 제가 작성한 것 같이 클로저를 저렇게 상수에 할당해서 사용하는 경우도 있는지 궁금합니다!

Copy link

@jaemuYeo jaemuYeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

스텝3로 완벽하게 잘 구현해주셨군요!!
몇 가지 의견을 남겼으니 확인해주세요 :)

@@ -6,3 +6,34 @@
//

import Foundation

let combineIndexWithLottoHistorySuffix = { (index: Int) -> String in

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

클로저를 활용해보셨군요!
상수에 클로저를 저장해서 사용하는 것과 함수로 만들어 사용하는 것을 고민하셨던 것 같아요.
각각의 장단점이 있습니다.
내부적으로 로직이 돌아가고 결과를 반환하는 형식은 크게 다를 것이 없습니다.

피드백 세션 내용에도 있듯 '함수는 이름이 있는 클로저' 이니까요!
다면 위의 문장에서 말한 것처럼 '함수는 이름이 있다'라고 표현되었는데
지금 처럼 상수에 저장한다고 해서 저 클로저에 이름이 붙는 것은 아니예요.
저런 동작을 하는 클로저를 특정 상수에 '할당' 한 것 뿐입니다.

그래서 위에 상수에 저장한 방식과 함수를 구현하는 방식을 비교하는 것은 서로 다른 것을 비교하는 것과 같다고 볼 수 있을 것 같아요.
클로저 자체와 함수를 비교한다면 할 수 있겠지만 현재는 구현 방식이 다르다라고 생각합니다!
만약 함수와 비교한다면

func 예시함수() {

}

let combineIndexWithLottoHistorySuffix = 예시함수()

이 표현이 맞을 것 같아요!

아무래도 함수를 구현하는 것이 재사용성과 명확한 네이밍을 통해 가독성을 높이기에는 더 좋은 방법이지 않을까 생각합니다 :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

클로저를 그저 상수에 '할당'했다는 것이 꽤나 와닿네요.
그런 거라면 정말 그냥 함수로 구현하는 것이 재사용성과 명확성 면에서는 더 나은 방법일 수도 있겠네요 :)

return "\(index)\(lottoHistorySuffix)"
}

func generateLottoHistories() -> Dictionary<String, [Int]> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반환을하는 딕셔너리 타입에서 키값은 몇 회차인지를 저장하고있는데
String보다는 Int가 더 적절하지 않을까요??

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 그럼 그렇게 수정하겠습니다!

return lottoHistories
}

func findLottoHistory(at index: Int) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

매개변수

몇 회차인지에 대해 전달인자를 통해 주입받는데 index라는 네이밍 보다 round가 조금 더 어울리지 않을까 생각합니다!
킹키부츠님은 어떻게 생각하시나요~?

네이밍

만약 위의 매개변수가 round로 바뀐다면 find라는 '찾다'의 의미보다 '검색(serch)' 등의 다른 표현으로도 바꿔볼 수 있을 것 같아요.
history는 없애도 될 것 같습니다! -> ex. 몇 회차 로또 번호 검색

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

색인인 index보다 round 라는 단어가 회차라는 그 역할에 더 부합한 것 같습니다!
또한 find 보다는 무엇을 검색해서 정보를 얻는다 라는 느낌이라서 확 와닿네요!

@kingkiboots
Copy link
Author

@jaemuYeo

안녕하세요!
피드백 반영하여 PR 재요청드립니다!

  • 회차 저장 딕셔너리의 키값의 타입을 Int로 변경하였으며, 이에 따라 n회차 만들어주는 함수가 불필요해져 삭제하였습니다.
  • 매개변수 및 딕셔너리에 들어갈 키값의 변수명을 index에서 round로 변경하였습니다.
  • 매개변수가 index에서 round로 변경됨에 따라 함수명과 변수명 또한 그 의미에 맞게 변경해보았습니다.
    • findLottoHistory => searchLotto
    • selectedLottoHistory => searchedLottoNumbers
    • lottoHistoryResult => searchResult

@jaemuYeo
Copy link

코드도 훨씬 간결해지고 네이밍들이 명확해진 것 같습니다! 👍
Round2도 너무 고생 많으셨어요 :)
다음 라운드에서 만나요~~

@jaemuYeo jaemuYeo merged commit bf8b4c1 into yagom-academy:ss_17_kingkiboots Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants