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] 차리(이찬주) 미션 제출합니다. #11

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

Conversation

cjlee38
Copy link

@cjlee38 cjlee38 commented May 13, 2022

wordle 미션 제출합니다

감사합니다 :)

Copy link

@SuyeonChoi SuyeonChoi left a comment

Choose a reason for hiding this comment

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

안녕하세요 차리! 페퍼입니다😊
코틀린으로 워들을 깔끔하게 잘 구현해주셨네요👍👍 리뷰하면서도 배우고 갑니다!

추가로, 다음과 같이 정답이 marry인 경우에 mimic을 입력한 경우 🟩⬜⬜⬜ 가 표시되어야하지 않을까요?
이부분까지 확인해주면 좋을거 같아요.
image

private val results: MutableList<List<Color>> = mutableListOf()

fun printIntroduction() {
println("WORDLE을 6번 만에 맞춰 보세요.")

Choose a reason for hiding this comment

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

단어를 입력하는 횟수가 바뀌는 경우, 뷰 로직의 코드도 수정해야할거 같아요. 쉽게 관리할 수 있도록 변경하는건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

그렇네요. Game 클래스 내부의 companion object에서 상수로 관리하도록 처리하였습니다~

}

@Test
@DisplayName("test.")

Choose a reason for hiding this comment

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

DisplayName으로도 무엇을 테스트하고자 하는지 잘 모르겠어요🥲

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 +23 to +24
private val CACHE: List<String> = File("src/main/resources/words.txt")
.readLines()

Choose a reason for hiding this comment

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

캐싱 사용💯💯

}

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

Choose a reason for hiding this comment

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

여기서도 6이 사용되고 있네요!

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 9 to 11
val exactIndices = compareExact(word)
val anyIndices = compareAny(word)
return merge(exactIndices, anyIndices)

Choose a reason for hiding this comment

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

단어를 비교하는 로직을 이해하기 쉽네요. 한 수 배우고 갑니다😲😲

}

private fun playWordle(answer: Answer): Boolean {
repeat(6) {

Choose a reason for hiding this comment

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

6은 워들 게임에서 의미있는 숫자이지 않을까요?🤔

Copy link
Author

Choose a reason for hiding this comment

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

Game 클래스를 만들어서 내부 Companion object에서 관리하도록 처리하였습니다~

Copy link

@SuyeonChoi SuyeonChoi left a comment

Choose a reason for hiding this comment

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

안녕하세요 차리! 리뷰 반영해주신거 확인했습니다👍👍
차리의 꼼꼼한 테스트 코드와 kotest를 사용한 부분은 저도 인상깊게 봤습니다🤓
추가로 코멘트 남겼는데 확인해주시면 감사하겠습니다!

}

@Test
@DisplayName("위치만 일치하는 글자들과 틀린 글자들이 존재하는 예측을 비교한다..")

Choose a reason for hiding this comment

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

Suggested change
@DisplayName("위치만 일치하는 글자들과 틀린 글자들이 존재하는 예측을 비교한다..")
@DisplayName("위치만 일치하는 글자들과 틀린 글자들이 존재하는 예측을 비교한다.")

아련해지네요..🌟

Comment on lines 15 to 21
private fun String.isLowerCase(): Boolean {
return this.all { it.isLowerCase() }
}

fun compareByIndex(other: Word, myIndex: Int, otherIndex: Int = myIndex): Boolean {
return word[myIndex] == other.word[otherIndex]
}

Choose a reason for hiding this comment

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

Suggested change
private fun String.isLowerCase(): Boolean {
return this.all { it.isLowerCase() }
}
fun compareByIndex(other: Word, myIndex: Int, otherIndex: Int = myIndex): Boolean {
return word[myIndex] == other.word[otherIndex]
}
private fun String.isLowerCase(): Boolean = this.all { it.isLowerCase() }
fun compareByIndex(other: Word, myIndex: Int, otherIndex: Int = myIndex): Boolean =
word[myIndex] == other.word[otherIndex]

간단한 로직은 간결성을 위해 식으로 변경하면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

그렇네요~ 한줄짜리 메소드는 모두 식으로 변경하였습니다~

consumed.add(it)
return true
}
return@any false

Choose a reason for hiding this comment

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

😲😲😲

Comment on lines 4 to 6
GREEN("🟩"),
YELLOW("🟨"),
GRAY("⬜")

Choose a reason for hiding this comment

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

UI영역에서는 초록색, 노란색, 회색으로 결과를 보여주는 부분을 도메인 로직에서 의존하는거 같아요. 당장은 아니더라도 한번 고민해보시면 좋을거 같습니다😊

Copy link
Author

Choose a reason for hiding this comment

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

도메인쪽에서 뷰 로직에 영향을 받는게 좋아보이지는 않네요. view쪽에서 when절로 처리하였습니다~

Comment on lines 19 to 27
val consumed = mutableListOf<Int>()
return (RANGE_START..RANGE_END).filter { isAnyMatch(word, it, consumed) }
}

private fun isAnyMatch(word: Word, outerIndex: Int, consumed: MutableList<Int>): Boolean {
return (RANGE_START..RANGE_END).any {
if (this.word.compareByIndex(word, it, outerIndex) && !consumed.contains(it)) {
consumed.add(it)
return true

Choose a reason for hiding this comment

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

워들 로직이 아직 개선되지 않은거 같아요!
만약 정답이 humph이고 hippy를 입력한 경우, p가 한개임을 유추할 수 있도록 첫번째 p는 회색으로 나와야하지 않을까요?
로마의 리뷰에서 참고해보았는데, 차리도 워들 세부규칙을 참고하면 좋을거 같습니다!
image

Copy link
Author

Choose a reason for hiding this comment

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

해결했다고 생각했는데 여전히 남아있었네요 ㅠ. 해당 부분 다시 수정하였으니 다시 확인해주시면 감사하겠습니다~

@SuyeonChoi
Copy link

차리 리뷰 반영해주신거 모두 확인했습니다! 빠르게 반영해주셔서 감사합니다🙇‍♀️
저는 더 이상 피드백 드릴게 없는거 같아요👍👍
차리 코드 리뷰하면서 저도 도움이 되고 유익한 시간이였습니다〰️
그럼 내일 만나요👋

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