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

[숫자 야구 게임] 윤영로 미션 제출합니다. #6

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

Conversation

YeongRoYun
Copy link

No description provided.

@YeongRoYun YeongRoYun marked this pull request as draft January 25, 2024 08:05
@YeongRoYun YeongRoYun marked this pull request as ready for review January 25, 2024 08:06
Copy link
Owner

@Jaeyoung22 Jaeyoung22 left a comment

Choose a reason for hiding this comment

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

영로님, 코드 잘 읽었습니다.

테스트 코드도 작성하시고 빠른 시간 안에 정말 잘 구현해주신 것 같아요.

몇가지 커멘트 남겼습니다. 확인해주시면 감사하겠습니다!

1주차 고생하셨습니다~!

@@ -10,7 +10,9 @@ repositories {
}

dependencies {
implementation('com.github.woowacourse-projects:mission-utils:1.0.0')
implementation 'com.github.woowacourse-projects:mission-utils:1.0.0'
testImplementation 'com.tngtech.archunit:archunit-junit5:1.2.1'
Copy link
Owner

Choose a reason for hiding this comment

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

엥 JUnit 의존성이 추가 안 되어있었나요?

Copy link
Author

Choose a reason for hiding this comment

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

@Jaeyoung22 앗.. 중복으로 추가한 것 같습니다. 감사합니다!!

public class Application {
public static void main(String[] args) {
//TODO: 숫자 야구 게임 구현
RandomDigit random = new RandomDigit();
Copy link
Owner

Choose a reason for hiding this comment

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

접근제한자가 따로 없고 default로 선언하신 이유가 있을까요?
더해서, 로컬 변수로 두지 않고 필드에 선언하신 이유가 있을까요?


while (isContinue) {
String goal = random.generate(3);
// System.out.println(goal);
Copy link
Owner

Choose a reason for hiding this comment

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

사용하지 않는 코드는 지워주세용 ㅎㅎ

String goal = random.generate(3);
// System.out.println(goal);
while (true) {
systemOutput.begin();
Copy link
Owner

Choose a reason for hiding this comment

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

개인적으로 밑에 end()와 더불어서 begin이나 end가 약간 SystemOutput 이라는 객체가 시작되고 끝나는 것 같은 네이밍이라고 느껴지네요... 영로님 생각은 어떠실까요?

systemOutput.begin();
String input = userInput.predict();
BaseballResult result = rule.check(input, goal);
systemOutput.miss(result.getBall(), result.getStrike());
Copy link
Owner

Choose a reason for hiding this comment

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

miss가 어떤 의미인지 이 코드만 봐서는 잘 모르겠어요. 바로 위의 커멘트와 비슷한 맥락으로 생각해주시면 감사하겠습니다!

}

@Override
public boolean equals(Object obj) {
Copy link
Owner

Choose a reason for hiding this comment

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

equls()를 오버라이딩 하신 이유가 있으실까요? BaseballResult를 VO라고 보신걸까요?! 저도 매우 동의합니다.
equals()와 hashcode()를 동시에 오버라이딩하라 라는 키워드를 검색해보시는 걸 추천드려요!

if (Objects.isNull(input) || Objects.isNull(goal) || input.length() != 3 || goal.length() != 3) {
throw new IllegalArgumentException("입력값이 잘못되었습니다. 3자리 숫자를 입력해주세요: goal: " + goal + "/input: " + input);
}
// input, goal 모두 3자리 숫자
Copy link
Owner

Choose a reason for hiding this comment

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

주석보다, 위에 있는 input.length() != 3에서 3을 DIGIT_NUMBER 같은 상수로 두면 더 이해하기 쉬울 것 같아요!

int strike = 0;
int ball = 0;
for (int i = 0; i < 3; ++i) {

Copy link
Owner

Choose a reason for hiding this comment

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

여기에 개행을 하신 이유가 있을까요?

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

public class RandomDigitTest {
Copy link
Owner

Choose a reason for hiding this comment

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

테스트코드 아주 바람직하고 멋집니다

String n2 = digit.generate(3);

// then
assertTrue(n1.length() == 3 && n2.length() == 3);
Copy link
Owner

Choose a reason for hiding this comment

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

assertAll이나 SoftAssertions의 assertSoftly에 대해서 검색해보시는 걸 추천드려요.

지금 같은 상황에서는 아래 반복문 안에 있는 assertFalse() 중에 하나라도 실패하면 아예 이 테스트 자체가 실패했다고 나옵니다.

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