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

[이영진] 2주차 미션 #12

Open
wants to merge 2 commits into
base: youngreal
Choose a base branch
from

Conversation

youngreal
Copy link

No description provided.

Copy link

@this-is-spear this-is-spear left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다. 영진님 ☺️
미션을 진행하면서 설계에 정말 많은 시간을 투자하신게 눈에 보이네요 ☺️
수정이 필요한 부분은 리뷰로 달았습니다. 고생많으셨습니다. 👍

while (answerNum.size() < ANSWER_MAX_SIZE) {
int element = (int) (Math.random() * ANSWER_NUMBER_RANGE);
if(isNotContainsNumber(element)){
answerNum.add(element);

Choose a reason for hiding this comment

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

answerNum을 불변 컬렉션으로 만드는 방법은 좋았지만 해당 리스트 안에 인자를 추가하는 건 불변이라고 부르기 어려울 것 같아요..!!

불변 컬렉션과 관련된 링크 첨부하겠습니다. ☺️
https://www.daleseo.com/java9-immutable-collections/

그리고 추가적으로 불변 객체를 생성하는 이유도 링크 첨부하겠습니다!
https://mangkyu.tistory.com/131

/**
* 상수값들을 보관한 클래스
*/
public class Constant {

Choose a reason for hiding this comment

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

상수 값들을 보관하는 클래스를 구현하셨군요! 이 방법 말고 상수가 필요한 구현체에서 선언하는건 어떨까요? 해당 방식은 결합도를 높이면서 응집도를 저해하는 방식이라 생각이 듭니다! ☺️

}

private ArrayList<Integer> getAnswerNumber() {
Answer answer = new Answer();

Choose a reason for hiding this comment

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

새로운 객체를 생성해서 구현하는 방식보다 generateAnswerNumber 메서드에서 Answer를 구현하는 방식이이 낫지 않을까 생각이 듭니다. 그리고 너무 많은 메서드가 만들어져서 오히려 가독성을 해치는 것 같습니다! 딱 필요한 메서드만을 의미있는 이름으로 짓는건 어떨까요? ☺️

}
}

private void DuplicateNumberExceptionValidation(char c) {

Choose a reason for hiding this comment

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

메서드 이름에서 첫글자는 항상 소문자여야 합니다. ☺️

스크린샷 2022-05-29 오후 10 30 02

Suggested change
private void DuplicateNumberExceptionValidation(char c) {
private void duplicateNumberExceptionValidation(char c) {

}
}

private void notOnlyExceptionValidation(char c) {

Choose a reason for hiding this comment

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

동사로 시작하는 이름으로 메서드로 짓는건 어떨까요? ☺

스크린샷 2022-05-29 오후 10 29 22

}

public void printResult(CompareAnswer answer) {
if (answer.getStrikeCount()>STRIKE_BALL_MIN) {

Choose a reason for hiding this comment

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

인텔리제이에서 자동 줄 정렬 기능을 제공하고 있습니다!

https://penthegom.tistory.com/47

Suggested change
if (answer.getStrikeCount()>STRIKE_BALL_MIN) {
if (answer.getStrikeCount() > STRIKE_BALL_MIN) {

}

public static void printEnding() {
System.out.println(ALL_NUMBER_CORRECT_MESSAGE);

Choose a reason for hiding this comment

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

만약 시간적인 여유가 되신다면 메시지와 같은 상수를 enum으로 관리하시는건 어떨까요? ☺️

https://techblog.woowahan.com/2527/

/**
* 정답숫자와 인풋숫자를 비교하여 점수를 카운팅하는 클래스
*/
public class CompareAnswer {

Choose a reason for hiding this comment

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

정답 숫자와 입력하는 숫자를 비교하기 위해 새로운 구현체를 만들지 않고, Answer 클래스에서 비교할 수 있도록 구현하는건 어떨까요? ☺️ 이 부분은 응집도를 저해하는 방법이라 생각이 듭니다. ☺️

this.answerNum = answerNum;
}

public void ballScoreCount() {

Choose a reason for hiding this comment

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

메서드 명은 항상 동사로 시작해야 합니다!

this.answerNum = answerNum;
}

public void ballScoreCount() {

Choose a reason for hiding this comment

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

예를 들면 이 방식이 있겠군요!

Suggested change
public void ballScoreCount() {
public void countBall() {

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