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] 조조그린(조한석) 미션 제출합니다. #9

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

Conversation

jojogreen91
Copy link

kotlin-wordle 미션 제출합니다 :)

Copy link

@summerlunaa summerlunaa left a comment

Choose a reason for hiding this comment

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

조조그린 안녕하세요! 파랑입니다.

코드리뷰는 처음인데 오히려 제가 더 많이 배워가네요!
저의 얕은 코틀린 지식으로 열심히 리뷰 달아보았습니다ㅎㅎ.

파이팅🔥💙

import wordle.controller.WordleController

fun main() {
WordleController().run()

Choose a reason for hiding this comment

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

image
ktlintCheck가 실패합니다ㅠ_ㅠ 한 번 확인하시고 수정해야할 것 같아요!

Copy link
Author

@jojogreen91 jojogreen91 May 18, 2022

Choose a reason for hiding this comment

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

이거 계속 수정하는데 자꾸 새로운게 튀어나오고 지적하는 부분이 뭘 수정하라는 건지 모르겠네요ㅠㅠ
일단 계속 해볼게요 파랑🥲

}

private fun isOver(result: MutableList<Mark>) =
results.isLimit() || result.all { mark -> mark == Mark.EXACT }

Choose a reason for hiding this comment

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

mark 대신 it을 사용하면 더 간결하게 바꿀 수 있을 것 같아요.

Suggested change
results.isLimit() || result.all { mark -> mark == Mark.EXACT }
results.isLimit() || result.all { it == Mark.EXACT }

Copy link
Author

Choose a reason for hiding this comment

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

아 'it' 이라고 해야지 생략도 할 수 있군요!
수정했습니다🔧

Comment on lines 10 to 13
fun printInputMessage(): String {
println("정답을 입력해 주세요.")
return readln()
}

Choose a reason for hiding this comment

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

printInputMessage라는 이름만 보고 "정답을 입력해 주세요."라는 메시지만 출력해주는 줄 알았는데 입력값까지 받고 있어서 조금 헷갈리는 것 같아요!

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 17 to 19
if (!isPlaying) {
printTryCount(tryCount)
}

Choose a reason for hiding this comment

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

view에서 isPlaying 상태를 확인해주는 것이 맞는지 잘 모르겠어요🤔 조조그린은 어떻게 생각하세요?

Copy link
Author

Choose a reason for hiding this comment

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

서버관점에서 봤을 때 View 레이어에서 Game 을 전달받아서 알아서 출력할 내용물을 만들고 출력하는 건 자연스럽다고 생각해서 말씀해주신 부분을 옮기진 않았고
View 에 존재하는 메서드 중 너무 많은 책임을 가지고 있는 것들을 리팩토링하면서 분리해줘봤습니다! :)

Comment on lines 31 to 39
val stringBuilder = StringBuilder()
result.forEach {
when (it) {
Mark.NONE -> stringBuilder.append("⬜")
Mark.EXIST -> stringBuilder.append("\uD83D\uDFE8")
Mark.EXACT -> stringBuilder.append("\uD83D\uDFE9")
}
}
println(stringBuilder.toString())

Choose a reason for hiding this comment

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

stringBuilder를 사용하지 않고도 출력할 수 있는 방법이 있을 것 같아요😀

Copy link
Author

@jojogreen91 jojogreen91 May 18, 2022

Choose a reason for hiding this comment

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

저는 떠오르는 생각이 없는데 어떤방법이 있을까요 파랑? 😀
일단 이 부분 apply() 를 사용해서 리팩토링 할 수 있을 것 같아서 수정해봤습니다!

Choose a reason for hiding this comment

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

result.forEach {
    print(
        when (it) {
            NONE -> ""
            EXIST -> "\uD83D\uDFE8"
            EXACT -> "\uD83D\uDFE9"
        }
    )
}

이런식으로 하면 stringBuilder를 사용하지 않고도 출력할 수 있지 않을까요!? 참고만 해주세용ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

아하 바로 출력해도 되겠네요 ㅋㅋㅋ

@Test
fun 글자길이가_5가_아닌_경우_예외발생() {
assertThrows<IllegalArgumentException> { Answer("abcdef") }
.shouldHaveMessage("[ERROR] 부적절한 글자 길이입니다.")

Choose a reason for hiding this comment

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

shouldHaveMessage 제가 찾던 기능이네요! 배워갑니다ㅎㅎ

class Words {

companion object {
private val VALUE: List<String> = Files.readAllLines(Paths.get("src/main/resources/words.txt"))

Choose a reason for hiding this comment

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

이렇게 하면 더 간단할 것 같아요!

Suggested change
private val VALUE: List<String> = Files.readAllLines(Paths.get("src/main/resources/words.txt"))
private val VALUE: List<String> = FileReader("src/main/resources/words.txt").readLines()

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 +20 to +22
for (char in word) {
wordTable[char] = wordTable.getOrDefault(char, 0) + 1
}

Choose a reason for hiding this comment

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

getOrDefault를 사용하면 wordTable을 더 간편하게 초기화할 수 있군요..! 배워갑니다👍

@jojogreen91
Copy link
Author

피드백 반영했어요 파랑 :)
추가적으로 오늘 배운것들도 한번 적용하면서 리팩토링 해봤는데 어떨지 모르겠네요 ㅋㅋ
감사합니다 파랑😀

Copy link

@summerlunaa summerlunaa left a comment

Choose a reason for hiding this comment

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

조조그린 수정된 코드 확인했습니다! 더 이상 크게 리뷰할 부분이 보이지 않는 것 같아요ㅎㅎㅎ
덕분에 backing properties랑 언더바 사용법도 배웠어요!

그리고 슬랙에 언더바 사용 관련해서 질문 남겨주셔서 제가 찾아본 내용 공유해드릴게요!

private val _results: MutableList<List<Mark>> = mutableListOf()
val results: List<List<Mark>>
    get() = _results
private val _results: MutableList<List<Mark>> = mutableListOf()
val results: List<List<Mark>> = _results

위 코드를 아래 코드로 작성해도 상관 없냐고 질문주셨는데, 결론부터 말하자면 두 가지 방식에 차이가 있습니다!

첫 번째 방식을 사용하면 result를 getter로 가져올 때 _result의 값을 반환해줍니다.
반면에 두 번째 방식을 사용하면 초기화될 때 result에 _result 값이 세팅되어 버립니다.
따라서 두 번째 예시처럼 코드를 작성하시면, 중간에 _result의 값이 바뀌는 경우에 변경된 값이 아니라 초기에 세팅된 값을 가져오게 됩니다🥲
근데 지금은 _result가 value라서 상관없을 것 같아요!!

https://velog.io/@haanbink/Kotlin-Backing-Properties

제가 참고한 블로그 링크 공유해드릴게요!
감사합니다~~👏💙

@@ -0,0 +1,60 @@
package wordle.domain

import wordle.domain.Mark.*

Choose a reason for hiding this comment

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

요렇게 wildcard import 사용하면 ktlintCheck 에러 떠요!

https://blog.leocat.kr/notes/2020/12/14/intellij-avoid-wildcard-imports-in-kotlin-with-intellij

요 블로그 참고하심 좋을 것 같아요!

나머지 에러 두 개는 import 순서 때문에 뜨네요. 포매팅 해주시면 에러 없앨 수 있을 것 같습니다!

Comment on lines +7 to +13
private val _results: MutableList<List<Mark>> = mutableListOf()
val results: List<List<Mark>>
get() = _results

fun add(result: List<Mark>) {
_results.add(result)
}

Choose a reason for hiding this comment

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

내부에서는 MutableList를 외부에서는 List를 사용! 좋습니다!
언더바 사용도 💯💯 덕분에 저도 배워가요ㅎㅎㅎ

import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Test

class DslTest {

Choose a reason for hiding this comment

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

DslTest까지 구현해주셨네요ㅎㅎ 좋습니다!

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