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

Step2 구현 완료 #133

Merged
merged 11 commits into from
Apr 4, 2019
Merged

Step2 구현 완료 #133

merged 11 commits into from
Apr 4, 2019

Conversation

CheolHoJung
Copy link

안녕하세요.
qna 2단계 구현 완료하여 피드백 요청드립니다!

Copy link
Contributor

@javajigi javajigi left a comment

Choose a reason for hiding this comment

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

구현 깔끔하게 잘했네요. Acceptance Test, Unit Test 모두 구현 잘 했네요. 💯
단, 도메인 객체를 좀 더 적극 활용해도 좋을 것 같아요.
피드백 몇 개 남겼는데요. 3단계 진행할 때 같이 반영해 보세요.

@@ -38,6 +38,15 @@ public Question(String title, String contents) {
this.contents = contents;
}

public void update(Question updatedQuestion) {
Copy link
Contributor

Choose a reason for hiding this comment

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

자신이 쓴 글인지에 대한 체크를 이 메소드에서 진행하면 어떨까?
이에 따른 단위 테스트도 구현할 수 있지 않을까?

this.contents = updatedQuestion.contents;
}

public void delete() {
Copy link
Contributor

Choose a reason for hiding this comment

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

자신이 쓴 글인지에 대한 체크를 이 메소드에서 진행하면 어떨까?
이에 따른 단위 테스트도 구현할 수 있지 않을까?

}

origin.update(updatedQuestion);
return questionRepository.save(origin);
Copy link
Contributor

Choose a reason for hiding this comment

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

questionRepository.save(..)와 같이 명시적으로 구현하지 않고 단순히 question.update(updatedQuestion) 만으로도 수정이 가능한지 테스트해 본다.

}

origin.delete();
questionRepository.save(origin);
Copy link
Contributor

Choose a reason for hiding this comment

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

questionRepository.save(..)와 같이 명시적으로 구현하지 않고 단순히 origin.delete() 만으로도 수정이 가능한지 테스트해 본다.

@javajigi javajigi merged commit 0271dd7 into next-step:CheolHoJung Apr 4, 2019
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.

None yet

2 participants