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

[4기][3주차] Wordle 과제 제출 - 짐배 #26

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

Conversation

jimbaemon
Copy link

No description provided.

@Saerang
Copy link

Saerang commented Apr 8, 2023

파일이 없네요 확인부탁드립니다. :)

@jimbaemon
Copy link
Author

파일이 없네요 확인부탁드립니다. :)

업로드 하였습니다 :)

README.md Outdated
* X X Y G X

* 입력 : h e l l o
*
Copy link

Choose a reason for hiding this comment

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

빈 공간이 있네요

return result;
}

private Color mapped(char answer, char input) {
Copy link

Choose a reason for hiding this comment

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

메서드가 너무 좋네요 👍

Copy link

Choose a reason for hiding this comment

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

메서드가 private라서 테스트를 안하신 것 같은데 Char를 클래스로 뽑아서 해당 메서드 테스트코드 추가 해도 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

테스트가 가능하도록 Color 쪽으로 이동해 봤습니다.

public static String[] readFromResource(String resourceName){
URL txtUrl = IOUtils.class.getClassLoader().getResource(resourceName);
try {
List<String> words = Files.readAllLines(Paths.get(txtUrl.toURI()));
Copy link

Choose a reason for hiding this comment

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

txtUrl.toURI()에서 NPE 가 발생할 수 있을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

assert 로 null값 처리 하였습니다.

URL txtUrl = IOUtils.class.getClassLoader().getResource(resourceName);
try {
List<String> words = Files.readAllLines(Paths.get(txtUrl.toURI()));
return words.toArray(new String[words.size()]);
Copy link

Choose a reason for hiding this comment

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

toArray를 사용할 때 size를 0으로 주면 성능상 이점이 있습니다. 아래 참고하시면 좋을 것 같네요 :)
https://stackoverflow.com/questions/174093/toarraynew-myclass0-or-toarraynew-myclassmylist-size

Copy link
Author

Choose a reason for hiding this comment

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

꿀팁 왕왕 감사합니다 🙇🏻‍♂️


TryResult tryResult = new TryResult();
int round = 0;
while (round++ < PLAY_ROUND && !tryResult.isFinished()) {
Copy link

Choose a reason for hiding this comment

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

메서드 추출이나, 뭔가 경기를 종료시키는 다른게 있으면 좋을 것 같습니다.

}

public static void results(TryResult tryResult) {
List<List<Color>> results = tryResult.getResults();
Copy link

Choose a reason for hiding this comment

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

List 가 많이 쓰이는데 별도의 클래스가 있어도 좋아보이네요.

Copy link
Author

Choose a reason for hiding this comment

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

Colors로 일급 컬렉션 생성하였습니다.

}

@Test
void readFile() throws URISyntaxException, IOException {
Copy link

Choose a reason for hiding this comment

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

throws를 던진필요가 없어보입니다.

Copy link
Author

Choose a reason for hiding this comment

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

제거 하였습니다

@@ -0,0 +1,35 @@
import domain.Color;
import domain.Word;
import org.junit.jupiter.api.Assertions;
Copy link

Choose a reason for hiding this comment

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

AssertJ도 한번 써보시면 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

변경 하였습니다.

Words words = new Words("aaaaa", "bbbbb");

//when
Word answers = words.answer(LocalDate.of(2021, 6, 20));
Copy link

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.

테스트 강화해 보았습니다.

import java.util.Arrays;
import java.util.List;

public class WordleTest {
Copy link

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.

필요 없는 기능들 딴곳으로 이동 시키고 테스트 강화 하였습니다.

@Saerang
Copy link

Saerang commented Apr 10, 2023

처음 게임 시작할 때 단어집에 없는 단어를 선택하면 게임이 종료가 되네요.
의도가 되신걸까요?
image

@jimbaemon
Copy link
Author

처음 게임 시작할 때 단어집에 없는 단어를 선택하면 게임이 종료가 되네요. 의도가 되신걸까요? image

image

요구사항에 근거해서 만들었는데 종료되지 않게끔은 처리해 보겠습니다 :)

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