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

[Wordle] 로마(김범수) 미션 제출합니다. #12

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

kbsat
Copy link

@kbsat kbsat commented May 13, 2022

잘 부탁드립니당

Copy link

@dongho108 dongho108 left a comment

Choose a reason for hiding this comment

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

안녕하세요 로마! 애쉬에요!!

워들 구현 진짜 잘해주셨네용;;
저도 많이 배워갑니다. 특히 kotest should be 하는거 너무 간지나요. 저도 수정하려구요ㅋㅋ

부족하지만 코멘트 조금 남겨봤어요
본인의 생각이 다르다면 답글 달아주셔도 좋아여 저도 잘 모르는 부분이 많아서영

row마 화이팅

Comment on lines 19 to 23
fun match(answer: Word) {
count++
require(words.contains(answer)) { "등록된 단어가 아닙니다." }
results.add(words.check(answer))
}

Choose a reason for hiding this comment

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

등록된 단어가 아니어도 count가 증가할 수 있을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

오 놓쳤던 부분인데 감사합니다! 😮

Comment on lines +18 to +31
override fun equals(other: Any?): Boolean {
if (this === other) return true
if (javaClass != other?.javaClass) return false

other as Word

if (value != other.value) return false

return true
}

override fun hashCode(): Int {
return value.hashCode()
}

Choose a reason for hiding this comment

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

data클래스로 만들면 equals hashcode가 필요 없지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

해당 클래스에서 lowerCase로 바꿔주는 작업을 위해서 주생성자에서 인자의 형식으로 값을 받습니다.
그렇기 때문에 프로퍼티가 선언이 되어있지 않는 주생성자는 data class로 바꿀 수 없었습니다. 그래서 직접 정의해주는 방식을 선택했습니다~!

Comment on lines 9 to 17
@Test
fun `등록된 단어가 아닌 경우 예외가 발생한다`() {
val game = Game(listOf(Word("apple"), Word("hello"), Word("spicy")))

val exception = shouldThrow<IllegalArgumentException> {
game.match(Word("spell"))
}
exception.message shouldBe "등록된 단어가 아닙니다."
}

Choose a reason for hiding this comment

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

kotest 쓰셨네요 ㄷㄷ 멋져요

Copy link
Author

Choose a reason for hiding this comment

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

허허.. 케이의 블로그를 참고해봤습니다.

- [x] 정답은 매일 바뀐다.
- [x] `words.txt` 안의 ((현재 날짜 - 2021년 6월 19일) % 배열의 크기) 번째의 단어이다.
- [x] 정답을 맞췄을 경우 타일 표현 전에 몇 번째 만에 맞췄는지 출력한다.

Copy link

@dongho108 dongho108 May 15, 2022

Choose a reason for hiding this comment

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

./gradlew ktlintCheck 이 실패하네요! 한번 확인해보시면 좋을 것 같아요

(여기서 틀렸다는게 아니라 코멘트만 여기에 남긴거에요!)

Copy link
Author

Choose a reason for hiding this comment

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

오 감사합니다! 수정하고 깜빡한 것 같아요 😥

Comment on lines 7 to 9
fun printStartMessage() {
println("WORDLE을 6번 만에 맞춰 보세요.")
println("시도의 결과는 타일의 색 변화로 나타납니다.")

Choose a reason for hiding this comment

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

wordle의 라운드수가 바뀌게되면 view도 수정해야 되겠네요!

Suggested change
fun printStartMessage() {
println("WORDLE을 6번 만에 맞춰 보세요.")
println("시도의 결과는 타일의 색 변화로 나타납니다.")
fun printStartMessage(int round) {
println("WORDLE을 $round 만에 맞춰 보세요.")
println("시도의 결과는 타일의 색 변화로 나타납니다.")

Copy link
Author

Choose a reason for hiding this comment

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

좋은 의견 감사합니다 애쉬🙇‍♂️
애쉬 의견대로 수정해보겠습니다!

Comment on lines 30 to 31
repeat(5) { i -> result[i] = findTileBySameCheck(word, i) }
repeat(5) { i -> result[i] = findTileByContainCheck(result, word, i) }

Choose a reason for hiding this comment

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

이렇게 바꿀 수 있을 것 같아요

Suggested change
repeat(5) { i -> result[i] = findTileBySameCheck(word, i) }
repeat(5) { i -> result[i] = findTileByContainCheck(result, word, i) }
repeat(5) { result[it] = findTileBySameCheck(word, it) }
repeat(5) { result[it] = findTileByContainCheck(result, word, it) }

Copy link
Author

Choose a reason for hiding this comment

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

오 좋은 의견 감사합니다 애쉬~ 😁

Copy link

@dongho108 dongho108 left a comment

Choose a reason for hiding this comment

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

좋은 리뷰 드리고싶어서 파랑 PR에 달린 제이슨 리뷰도 참고했고 로마 코드도 꼼꼼히 봤습니당

몇가지 코멘트 달아봤어용!

var results: MutableList<List<Tile>> = ArrayList()
private set

constructor(words: List<Word>) : this(Words(words))

Choose a reason for hiding this comment

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

Game에서 List를 Words로 바꿔주는 이유가 있을까요?
외부에서 Words(words)로 주입받으면 생성자에서 Words로 변환해줄 필요가 없을 것 같아요!


fun printCount(count: Int) {
println()
println("$count/6")

Choose a reason for hiding this comment

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

여기도 round를 매개변수로 받을 수 있겠네용

Suggested change
println("$count/6")
println("$count/${round}")

Comment on lines 10 to 12
require(value.length == WORD_SIZE) { "단어의 길이는 5글자여야 합니다." }
require(Regex("[a-zA-Z]*").matches(value)) { "단어에 영어가 아닌 글자나 공백이 포함될 수 없습니다." }
}

Choose a reason for hiding this comment

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

저는 조건문안에 명확하게 식이 있는것도 선호합니다!
그런데 이 부분을 확장함수로 리팩토링하니까 재밌더라구요~
로마도 확장함수 연습겸 추천드립니당

Choose a reason for hiding this comment

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

Suggested change
require(value.length == WORD_SIZE) { "단어의 길이는 5글자여야 합니다." }
require(Regex("[a-zA-Z]*").matches(value)) { "단어에 영어가 아닌 글자나 공백이 포함될 수 없습니다." }
}
require(value.isRightSize()) { "[ERROR] ${SIZE}글자의 단어를 입력하세요." }
require(value.isAlphabet()) { "[ERROR] 영어 단어를 입력하세요." }

Choose a reason for hiding this comment

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

"제이슨이"
String을 패턴 매칭을 위해 Regex으로 바꾸는 것은 꽤나 비싼 행위입니다. 미리 Regex으로 만들어 사용하는 것은 어떨까요? 아래의 글을 참고하세요.
https://stackoverflow.com/questions/2469244/whats-the-difference-between-string-matches-and-matcher-matches/2469275

Comment on lines 18 to 26
private fun doGame(game: Game) {
val answer = Word(InputView.inputAnswer())
game.match(answer)
if (game.isGameOver(answer)) {
return
}
OutputView.printResults(game.results)
doGame(game)
}

Choose a reason for hiding this comment

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

"제이슨이"
재귀 함수보다 조금 더 직관적으로 만들 수 있을까요?

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.

3 participants