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

비밥 워들 구현제출합니다. #3

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

Conversation

pci2676
Copy link

@pci2676 pci2676 commented Apr 15, 2022

변명이지만.. 코로나 약 기운이 꽤 쎄서 정신없이 짠 감이 없지않아 있습니다..
더 손대고 싶은 부분이 있지만 어느정도 만족해서 일단 PR 생성합니다!

 - 알파벳 변수 이름 변경 _value -> alphabet
 - 알파벳 팩토리를 사용하도록 변경
 - 함수 위치 변경
 - 깨진 테스트 수정
Copy link

@ohgillwhan ohgillwhan left a comment

Choose a reason for hiding this comment

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

안녕하세요! 귀하게 짜신 코드 잘 보았습니다!
작은 리뷰를 남겼으니, 한번 같이 고민해보면 좋을것 같습니다!
:D

*/
class MemoryWordProvider(val words: List<Word>) : WordProvider {
override fun provide(date: LocalDate): Word {
val index = (date.format(DateTimeFormatter.ofPattern("yyyyMMdd")).toInt() - 20210619) % words.size

Choose a reason for hiding this comment

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

공식이 조금 이해한 바와는 다른것 같습니다! 오늘 날짜 기준으로 20220417-20210619 가 아닌, 2021년6월19일 부터의 날짜를 계산하는게 맞지 않을까 싶습니다. (302일 정도네요.)

Copy link
Author

Choose a reason for hiding this comment

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

아 그렇군요 이것도 제가 잘못 이해한것 같네요 ㅎㅎ

val words: List<Word>,
) : WordFinder {
override fun contain(input: Word): Boolean {
return words.contains(input)

Choose a reason for hiding this comment

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

List에서 해당 Word가 있는지 찾는것은 많은 비용이 들것 같은데, List보단 Set으로 검색하는건 어떨까요

Copy link
Author

Choose a reason for hiding this comment

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

성능에 대한 부분을 간과했네요! 감사합니다~

val windows: Set<Window>,
) {
init {
if (this.windows.size != WORD_SIZE) {

Choose a reason for hiding this comment

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

Exception을 직접 만들어서 처리하면 좋을것 같습니다.
테스트코드에서 보면 메세지를 가지고 의도한 위치에서 Exception이 터진것을 확인하고 있는데, 이렇게 되면 메세지가 바뀔경우 테스트도 깨지게 됩니다.

fun run() {
val targetWord = wordProvider.provide()
var wordle = Wordle(target = targetWord, wordFinder = wordFinder)
start()

Choose a reason for hiding this comment

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

start()수를 보면 Game.start()같은 느낌이 있습니다.
showStartMessage() 같은 함수명을 주어서 게임의 시작과는 관련이 없어보이도록 하는게 좋을것 같습니다.

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 +27
fun match(input: Word): List<WindowResult> {
var results = listOf<WindowResult>()

for (inputWindow in input.windows) {
val windowResult = getWindowResult(inputWindow)
results = results + windowResult
}

return results
}

Choose a reason for hiding this comment

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

Suggested change
fun match(input: Word): List<WindowResult> {
var results = listOf<WindowResult>()
for (inputWindow in input.windows) {
val windowResult = getWindowResult(inputWindow)
results = results + windowResult
}
return results
}
fun match(input: Word): List<WindowResult> {
return input.windows.map(::getWindowResult)
}

해당 코드로도 충분해보입니다!

Choose a reason for hiding this comment

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

아래의 조건이 빠진것 같습니다.
두 개의 동일한 문자를 입력하고 그중 하나가 회색으로 표시되면 해당 문자 중 하나만 최종 단어에 나타난다.

예시는 다음과 같습니다.

답: genre
입력 값: error

현재 해당 로직이 구현이 안되어서
🟨🟨🟨⬜🟨
이렇게 표출이 됩니다. 하지만 답에서 r은 1개이므로,
🟨🟨⬜⬜⬜
다음과 같이 표출이 되어야합니다.

우선순위는 PERFECT -> WRONG -> MISS 순으로 처리가 되어야합니다.

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 +25 to +27
private fun Word.rawWord(): String {
return windows.sortedBy { it.position }.joinToString(separator = "") { it.alphabet.alphabet }
}

Choose a reason for hiding this comment

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

Retry 객체를 생성하기 위한 메세지를 만드는것으로 보입니다.
메세지 만들기 위해 Word.windows가 public으로 풀린것이 조금 아쉽습니다! 다른 방법은 없을까요?


data class Wordle(
val target: Word,
val wordResult: List<WordResult> = emptyList(),

Choose a reason for hiding this comment

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

이 부분을 WordResults를 가지도록 하면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

지금과 같은 경우에는 한가지 경우밖에 없지만 저는 일급컬렉션을 멤버변수로 들고있는것 보다 사용되는 위치에 따라 해당 상황에 맞는 일급컬렉션을 그때 랩핑해서 사용하는게 좋다고 생각하고있습니다.

import edu.nextstep.wordle.application.wordle.Word
import java.time.LocalDate

interface WordProvider {

Choose a reason for hiding this comment

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

혹시 WordFinderfun interface로 구현하고, 여기는 그렇지 않은 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

WordProvider를 굳이 fun interface로 구현해둘 필요성을 못느꼈습니다.

Comment on lines +7 to +8
val target = Alphabet(alphabet)
return set.first { it == target }

Choose a reason for hiding this comment

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

Set을 사용하고 있지만, first를 사용하여 Iterable에 구현된 반복을 하고 있네요! Map<String, Alphabet>은 어떨까요?

//then
assertThat(answer).isInstanceOf(WordleAnswer.Right::class.java)

assertThat(answer.wordle).isEqualTo(Wordle(

Choose a reason for hiding this comment

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

isEqualTo 에 있는게 어떤 값인지 변수로 뽑으면 더 읽기 쉬울거 같습니다.

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.

2 participants