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

FL 워들제출합니다. #5

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

Conversation

Flamme1004K
Copy link

No description provided.

ohgillwhan and others added 30 commits April 11, 2022 23:56
Copy link

@etff etff left a comment

Choose a reason for hiding this comment

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

깔끔한 코드 구현이 좋았습니다. 👍
클래스 분리나 인터페이스를 사용한 것이 인상에 깊었습니다.

몇 가지 생각해볼만한 코멘트를 남겼습니다. 확인부탁드립니다.

Comment on lines +16 to +30
do {
var result: MatchResults

try {
result = game.progress(input)
} catch (e: IllegalArgumentException) {
println(e.message)
continue
}
output.write(result)

if (result.isCorrect()) {
break
}
} while (!game.checkToRetry())
Copy link

Choose a reason for hiding this comment

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

NIT: 개발자의 취향일 수 도 있지만 do statement는 저는 잘 사용하지 않는 편입니다.
반복 실행되는 조건이 하단의 위치하는 조건을 확인하고 다시 상단의 조건을 확인하면서 내려와야하기 때문에
위아래로 최소 두번은 읽어야하기 때문입니다. 참고만 부탁드립니다. 😃

see also

Do Statement (do-while)에 관한 소고

Comment on lines +11 to +12
val text = readln()
return Tiles.of(text)
Copy link

Choose a reason for hiding this comment

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

Suggested change
val text = readln()
return Tiles.of(text)
return Tiles.of(readln())

인라인을 통해 변수 선언을 줄여보면 어떨까요?

import domain.MatchResults
import domain.Output

object DefaultOutput : Output {
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 +69 to +71
const val ERROR_TILE_SIZE_MSG = "타일은 5개로 구성되어야 합니다."
const val REQUIRE_TILE_SIZE = 5
const val EMPTY = 0
Copy link

Choose a reason for hiding this comment

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

해당 클래스 내부에서만 사용한다면 반드시 private 접근자를 붙여주세요

Comment on lines +11 to +12
const val ERROR_RESULTS_SIZE_MSG = "결과는 5개로 구성되어야 합니다."
const val REQUIRE_RESULTS_SIZE = 5
Copy link

Choose a reason for hiding this comment

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

해당 클래스 내부에서만 사용한다면 반드시 private 접근자를 붙여주세요

Comment on lines +16 to +20
private fun checkWord(tiles: Tiles) {
if (!words.exists(tiles)) {
throw IllegalArgumentException(NOT_FOUND_WORD)
}
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
private fun checkWord(tiles: Tiles) {
if (!words.exists(tiles)) {
throw IllegalArgumentException(NOT_FOUND_WORD)
}
}
private fun checkWord(tiles: Tiles) {
require(words.exists(tiles)) { NOT_FOUND_WORD }
}

require를 사용해 볼 수 도 있을 것같아요!


class Game(private val words: Words) {
private val answer: Answer = Answer(words.getTodayWords())
private var tryCount = 0
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 +15 to +16
fillGreens(result, other, countOfTile)
fillYellows(result, other, countOfTile)
Copy link

@etff etff Apr 17, 2022

Choose a reason for hiding this comment

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

정답객체에서 판단하는 모든 책임을 가지고 있는 것보다
Tiles에 메시지를 던지도록 구조를 바꿔 보면 어떨까요? 객체에 메시지를 보내 데이터를 가지는 객체가 일하도록 해보세요.

if (this.tiles.equals(tile, index)) {
result[index] = MatchResult.CORRECT

countOfTile[tile] = countOfTile[tile]!!.dec()
Copy link

Choose a reason for hiding this comment

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

!!을 쓰지않고 변경해볼 수 있을까요?!

fillGreen(tile, index, result, countOfTile)
}
}
private fun fillYellows(
Copy link

Choose a reason for hiding this comment

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

이미 MatchResult에서 색을 표현하지 않기로 결정했으니 특정색을 칠하는 상황을 다르게 설명해보면
어떨까요?

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