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] 파랑(이하은) 미션 제출합니다. #13

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

Conversation

summerlunaa
Copy link

안녕하세요 파랑입니다.
워들 미션 제출합니다. 감사합니다!

README.md Outdated
Comment on lines 13 to 24
- [ ] 시작 문구를 띄워준다.
- [ ] 답안을 입력받는다.
- [x] 답안은 5글자여야 한다.
- 답안은 `words.txt` 안에 존재하는 단어여야 한다.
- [ ] 정답과 답안을 비교하여 결과를 타일로 표현한다.
- 맞는 글자는 초록색으로 표현한다.
- 위치가 틀리면 노란색으로 표현한다.
- 정답에 존재하지 않는 글자면 회색으로 표현한다.
- 두 개의 동일한 문자를 입력하고 그중 하나가 회색으로 표시되면 해당 문자 중 하나만 최종 단어에 나타난다.
- [x] 정답은 매일 바뀐다.
- [x] `words.txt` 안의 ((현재 날짜 - 2021년 6월 19일) % 배열의 크기) 번째의 단어이다.
- [ ] 정답을 맞췄을 경우 타일 표현 전에 몇 번째 만에 맞췄는지 출력한다.
Copy link

Choose a reason for hiding this comment

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

기능 요구 사항을 잘 정리해주셨는데 아직 [x] 업데이트가 아직 안된 것 같아요!

Comment on lines 7 to 11
var count: Int = 0
private set

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

Choose a reason for hiding this comment

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

private set👍 👍 좋은방법같아요!

Copy link
Author

Choose a reason for hiding this comment

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

그때 제로가 질문해서 배웠습니다👍

private set

fun isGameOver(answer: Word): Boolean {
return count == 6 || words.isCorrect(answer)
Copy link

Choose a reason for hiding this comment

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

OutputView에서 WORDLE을 6번 만에 맞춰 보세요.6과 도메인 Game의 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에서 public 상수로 선언해서 OutputView에서 가져다 쓰는 것으로 변경했습니다~

Copy link

Choose a reason for hiding this comment

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

깰끔하네여 ㅎ

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

fun isSameChar(other: Word, index: Int): Boolean {
return this.value[index] == other.value[index]
}
Copy link

Choose a reason for hiding this comment

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

깔끔 하네요ㅎㅎ 💯

val result: ArrayList<Tile> = ArrayList()
repeat(5) { result.add(Tile.GRAY) }

repeat(5) { i -> result[i] = findTileBySameCheck(word, i) }
Copy link

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) { result[it] = findTileBySameCheck(word, it) }

이렇게도 될거같아요!

class Words(private val values: List<Word>) {

private val answer: Word = findAnswer()
private var answerMap: MutableMap<Char, Int> = HashMap()
Copy link

Choose a reason for hiding this comment

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

HashMap을 사용한 이유가 궁금해요!

Copy link
Author

Choose a reason for hiding this comment

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

문자를 비교해서 같은 경우 문자를 소진하기 때문에 문자의 개수를 저장할 필요가 있었습니다. 그렇기 때문에 문자 - 문자의 개수를 key, value로 저장하기 위해 HashMap을 사용했습니다! 이 부분이 궁금했던 거 맞나요?

Copy link

@asebn1 asebn1 May 19, 2022

Choose a reason for hiding this comment

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

이해했습니다!! 역시 S....

Copy link
Owner

Choose a reason for hiding this comment

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

코틀린은 읽기 전용 컬렉션과 변경 가능한 컬렉션을 구별해 제공합니다.

Suggested change
private var answerMap: MutableMap<Char, Int> = HashMap()
private var answerMap: MutableMap<Char, Int> = mutableMapOf()

Comment on lines 6 to 13
object WordsReader {

fun getWords(): List<Word> {
val path = "src/main/resources/words.txt"
val reader = FileReader(path)
return reader.readLines().map { Word(it) }
}
}
Copy link

Choose a reason for hiding this comment

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

WordsReader 좋네요!! 근데 해당 클래스가 Util 패키지에 속하는 것이 맞는지 궁금해요. 저도 잘 몰라서 질문드립니다 😄

Copy link
Author

Choose a reason for hiding this comment

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

txt 파일에서 문자열을 읽어와 Word list로 변환해주는 역할만 하고 있어서 util 클래스라고 생각했습니다!

import io.kotest.matchers.shouldBe
import org.junit.jupiter.api.Test

class WordTest {
Copy link

Choose a reason for hiding this comment

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

코테스트 배워갑니다. 👍 👍 👍 👍 👍 👍

@asebn1
Copy link

asebn1 commented May 15, 2022

파랑 안녕하세요 ^-^

파랑이 구현한 코드를 보고 한번 제 의견 달아봤습니다.

코드를 보면서 저도 다시 한번 배울 수 있었습니다 👍

요즘 바쁜데 힘 같이 내봐요

파랑 화이티리링~ 🟦 🔢 💙 🈂️ 🔤 🧊

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

Choose a reason for hiding this comment

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

results.add(words.check(answer))에서 오류가 발생하더라도 count가 증가할 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

그렇네요 수정했습니다!

fun main() {
val game = Game(WordsReader.getWords())
OutputView.printStartMessage()
doGame(game)
Copy link

Choose a reason for hiding this comment

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

doGame보다 playGame은 어떠세요?ㅎㅎ 파랑은 어떻게 생각하시나요!?

Copy link
Author

Choose a reason for hiding this comment

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

play game이 더 자연스러운 것 같아서 변경했습니다👍

@asebn1
Copy link

asebn1 commented May 19, 2022

코틀린 장인을 꿈꾸는 파랑...
이번에 또 파랑코드 보면서 많이 배웠습니다.
저도 어쩌면 코틀린이랑 친숙해지고있는지...
뿌듯하군요
다음에도 배우러 가겠습니다 ^-^

Copy link
Owner

@woowahan-pjs woowahan-pjs left a comment

Choose a reason for hiding this comment

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

잘 했어요. 👍 조금 더 수정이 필요한 것 같아요.
가능한 읽기 전용 컬렉션을 사용하면 어떨까요?

@@ -0,0 +1,3 @@
package study.skill

interface Skill
Copy link
Owner

Choose a reason for hiding this comment

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

Skill, HardSkill, SoftSkill을 하나의 파일로 모으고 봉인된 클래스를 사용해 볼까요?
https://kotlinlang.org/docs/sealed-classes.html

Copy link
Author

Choose a reason for hiding this comment

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

sealed class는 이번에 처음 알게 되었네요! 말씀해주신대로 하나의 파일로 모아 Skill을 봉인된 클래스로 만들었습니다!

Comment on lines 13 to 14
OutputView.printCount(game.count)
OutputView.printResults(game.results)
Copy link
Owner

Choose a reason for hiding this comment

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

View에 대한 호출을 최소화해 보면 어떨까요?

return
}
OutputView.printResults(game.results)
playGame(game)
Copy link
Owner

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.

while문을 사용하는 것으로 로직을 수정했습니다.

var count: Int = 0
private set

var results: MutableList<List<Tile>> = ArrayList()
Copy link
Owner

Choose a reason for hiding this comment

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

뒷받침하는 프로퍼티(backing property)를 사용해 보세요.


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

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

Copy link
Author

Choose a reason for hiding this comment

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

Regex는 실행될 때마다 리컴파일되는 군요.. pattern을 상수로 만들어두고 pattern.matcher를 사용하는 것으로 변경했습니다!

class Words(private val values: List<Word>) {

private val answer: Word = findAnswer()
private var answerMap: MutableMap<Char, Int> = HashMap()
Copy link
Owner

Choose a reason for hiding this comment

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

코틀린은 읽기 전용 컬렉션과 변경 가능한 컬렉션을 구별해 제공합니다.

Suggested change
private var answerMap: MutableMap<Char, Int> = HashMap()
private var answerMap: MutableMap<Char, Int> = mutableMapOf()

Comment on lines 33 to 39
private fun initAnswerMap(): MutableMap<Char, Int> {
val answerMap: MutableMap<Char, Int> = HashMap()
answer.value.forEach {
answerMap[it] = answerMap.getOrDefault(it, 0) + 1
}
return answerMap
}
Copy link
Owner

Choose a reason for hiding this comment

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

아래의 코드로 개선해 보면 어떨까요?

Suggested change
private fun initAnswerMap(): MutableMap<Char, Int> {
val answerMap: MutableMap<Char, Int> = HashMap()
answer.value.forEach {
answerMap[it] = answerMap.getOrDefault(it, 0) + 1
}
return answerMap
}
private fun initAnswerMap(): Map<Char, Int> {
return answer.value
.groupBy { it }
.mapValues { (_, values) -> values.count() }
}

object OutputView {

fun printStartMessage() = println(
"WORDLE을 ${Game.MAX_GAME_COUNT}번 만에 맞춰 보세요.\n" + "시도의 결과는 타일의 색 변화로 나타납니다."
Copy link
Owner

Choose a reason for hiding this comment

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

아래의 코드로 개선해 보면 어떨까요?

Suggested change
"WORDLE을 ${Game.MAX_GAME_COUNT}번 만에 맞춰 보세요.\n" + "시도의 결과는 타일의 색 변화로 나타납니다."
"WORDLE을 ${Game.MAX_GAME_COUNT}번 만에 맞춰 보세요.\n시도의 결과는 타일의 색 변화로 나타납니다."

@Test
fun `게임 종료여부를 확인한다`() {
val game = Game(listOf(Word("apple"), Word("hello"), Word("spicy")))
repeat(6) { game.match(Word("apple")) }
Copy link
Owner

Choose a reason for hiding this comment

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

매번 이렇게 match()를 사용해야 할까요? 어떻게 하면 더 손쉬운 테스트를 할 수 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

Game을 생성할 때 maxGameCount를 받아 match를 사용하지 않고도 테스트할 수 있도록 수정했습니다!

@summerlunaa
Copy link
Author

제이슨 안녕하세요! 파랑입니다.
피드백 반영하면서 sealed class, backing properties, groupBy 등 코틀린에 대해 많이 배울 수 있었습니다.
확인 부탁드립니다! 감사합니다~

@summerlunaa
Copy link
Author

마지막 PR 제출합니다. 이번 워들 미션 덕분에 코틀린에 대해 많이 알아갈 수 있었습니다. 아래 내용들을 중심으로 리팩토링을 진행했습니다. 감사합니다!

  1. mutable collection의 사용 지양하기
  2. 확장 함수 사용
  3. 잘못된 입력이 들어와도 프로그램이 바로 종료되지 않고 입력을 다시 받도록 수정
  4. 테스트에서 AnnotationSpec 사용, junit 삭제
  5. 기타 코드 최적화

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.

4 participants