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

[김대겸] Step3 PR #803

Merged
merged 11 commits into from Dec 1, 2022
Merged

[김대겸] Step3 PR #803

merged 11 commits into from Dec 1, 2022

Conversation

Gyeom
Copy link

@Gyeom Gyeom commented Nov 30, 2022

안녕하세요 리뷰어님,

3단계 - 인증을 통한 기능 구현 PR 입니다.

이번 리뷰도 잘 부탁드립니다 :)

Copy link

@wooobo wooobo left a comment

Choose a reason for hiding this comment

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

3단계 역시 꼼꼼하게 잘 진행해주셨네요 💯

DynamicTest를 사용한 부분이 인상깊네요! ㅎ
이번단계도 잘 진행해주셔서 별다르게 코멘트 드릴게 없었네요!

merge진행하겠습니다.


@Transactional
public void deleteFavoriteById(final Long id, final Long memberId) {
Favorite favorite = favoriteRepository.findByIdAndMemberId(id, memberId)
Copy link

Choose a reason for hiding this comment

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

유저가 좋요한 즐겨찾기를 삭제할수 있도록 해주셨네요 👍

if (Objects.nonNull(originalSection)) {
originalSection.updateUpStation(section);
}
updateUpStation(section, originalSection);
Copy link

Choose a reason for hiding this comment

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

뎁스를 줄여보셨네요 👍


assertAll(
() -> assertThat(favorites).hasSize(1),
() -> assertStationId(favorites.get(0).getSourceStation(), favorite.getSourceStation()),
Copy link

Choose a reason for hiding this comment

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

해당 테스트에 대해서는 큰문제가 없을것 같지만,

개인적인 관점을 공유해드리면
assertThat(..).containsExactly 같은 검증을 사용해볼것 같아요!

@Column(name = "distance")
private int value;

protected Distance() {
this(0);
this(ZERO);
Copy link

Choose a reason for hiding this comment

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

상수화를 해보셨네요!

@wooobo wooobo merged commit 990021d into next-step:gyeom Dec 1, 2022
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