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

참여자 닉네임 생성, 모임의 참여자 조회 기능 생성 #96

Merged
merged 13 commits into from
Jul 24, 2024

Conversation

hoyeonyy
Copy link
Contributor

PR의 목적이 무엇인가요?

참여자 닉네임 생성, 모임의 참여자 조회 기능 생성

이슈 ID는 무엇인가요?

설명

Member 생성
닉네임을 활용한 기능 수정

질문 혹은 공유 사항 (Optional)

@hoyeonyy hoyeonyy added BE 백엔드 관련 이슈입니다. 🌱 기능추가 feature (새로운 기능 구현) labels Jul 23, 2024
Copy link
Contributor

@Mingyum-Kim Mingyum-Kim left a comment

Choose a reason for hiding this comment

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

수고하셨어요 !
궁금한 점에 대해서 간단히 리뷰 남겨놓았습니다 😄

같이 의논해봐요

@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;

private String nickName;
Copy link
Contributor

Choose a reason for hiding this comment

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

nickname이 하나의 단어지 않나요? 카멜 케이스 적용하는 게 맞는 가 궁금하네요!

Comment on lines +25 to +26
@ManyToOne(fetch = FetchType.LAZY)
private Moim moim;
Copy link
Contributor

Choose a reason for hiding this comment

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

한 명의 사람이 하나의 모임에만 참여할 수 있는 구조인데,
수정할 필요가 있지 않을까요?!

Copy link
Contributor

Choose a reason for hiding this comment

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

아, 현재 로그인 기능이 구현되어있지 않아서
테니가 여러 개의 모임에 참여하여도 테니가 여러 명 생성되는 식으로 임시 개발한 거군요.

Copy link
Contributor

@ay-eonii ay-eonii Jul 23, 2024

Choose a reason for hiding this comment

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

지금 멤버가 계속 생성되는 상황에서 @OneToOne이 아니라 @ManyToOne을 사용한 이유가 후에 회원정보를 저장(로그인) 기능을 염두에 두었기 때문이 맞을까용?

아니네요 핫핫

) {

public static MoimDetailsFindResponse toResponse(Moim moim) {
public static MoimDetailsFindResponse toResponse(Moim moim, List<String> participants) {
Copy link
Contributor

Choose a reason for hiding this comment

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

모임 객체에 Participants가 있는 것이 아니라 따로 받아오도록 설계한 이유가 있을까요?

Comment on lines 60 to 62
member.joinMoim(moim);
memberRepository.save(member);
moim.join();
Copy link
Contributor

Choose a reason for hiding this comment

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

member.joinMoim(moim) 이 있고 moim.join() 도 있으니 어떤 게 모임 참여인지 이해하기 어려운 부분이 있는 것 같아요!
moim.join()을 참여 인원 증가 라는 의미를 담아 다른 네이밍을 하는 건 어떤가요??

이건 이전 작업이니 여유가 되면 같이 논의해봐용😊

Comment on lines +25 to +26
@ManyToOne(fetch = FetchType.LAZY)
private Moim moim;
Copy link
Contributor

@ay-eonii ay-eonii Jul 23, 2024

Choose a reason for hiding this comment

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

지금 멤버가 계속 생성되는 상황에서 @OneToOne이 아니라 @ManyToOne을 사용한 이유가 후에 회원정보를 저장(로그인) 기능을 염두에 두었기 때문이 맞을까용?

아니네요 핫핫

Copy link
Contributor

@ay-eonii ay-eonii left a comment

Choose a reason for hiding this comment

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

고생많으셨어요 호기~
코멘트 확인 부탁드립니다 :>

@@ -1,6 +1,7 @@
package mouda.backend.moim.dto.request;

public record MoimJoinRequest(
Long moimId
Long moimId,
String nickName
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
String nickName
String nickname

Copy link
Contributor Author

Choose a reason for hiding this comment

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

눈썰미 미쳐버렸다

Comment on lines 21 to 26
// String sql = "SELECT table_name FROM information_schema.tables WHERE table_schema = 'PUBLIC' AND table_type='BASE TABLE'";
// List<String> tables = jdbcTemplate.queryForList(sql, String.class);
// for (String tableName : tables) {
// jdbcTemplate.update("DELETE FROM " + tableName);
// jdbcTemplate.update("ALTER TABLE " + tableName + " alter column id restart with 1");
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

이건 계속 두나용?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

바로 지우겠습니다

databaseCleaner.cleanUp();
}

@DisplayName("모임에 가입된 맴버의 이름을 반환한다.")
Copy link
Contributor

Choose a reason for hiding this comment

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

DisplayName이 참여자 수 검증하는 부분(Assertions)과 어울리지 않는 것 같아요!

저는 참여자의 수를 확인한다는 느낌을 받았습니당 어떻게 생각하세요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DisplayName을 수정해 보겠씁니다!

Copy link
Contributor

@Mingyum-Kim Mingyum-Kim left a comment

Choose a reason for hiding this comment

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

한번더 수정 부탁드려요 !!!

this.maxPeople = maxPeople;
this.authorNickname = authorNickname;
this.description = description;
}

public void join() {
public void validateCurrentPeople(int currentPeople) {
Copy link
Contributor

Choose a reason for hiding this comment

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

메서드명이 구체적이었음 좋겠는데요 .. 저도 딱히 떠오르는 메서드명이 없어서 생각해보구 없으면 넘어갑시다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

validateAlreadyFullMoim요걸로 수정하겠습니다!

Comment on lines +45 to +47
.map(moim -> {
List<Member> participants = memberRepository.findAllByMoimId(moim.getId());
return MoimFindAllResponse.toResponse(moim, participants.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

양방향이면 확실히 이런 부분에서 코드가 개선되긴 하겠네요 😄
나중에 수정해봅시다 💪

memberRepository.save(member);
List<Member> participants = memberRepository.findAllByMoimId(moim.getId());

moim.validateCurrentPeople(participants.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

validateAlreadyFullMoim 으로 수정어떤가요 ~?

Copy link
Contributor

@Mingyum-Kim Mingyum-Kim left a comment

Choose a reason for hiding this comment

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

수고하셨습니다~🙋‍♀️

Copy link
Contributor

@ay-eonii ay-eonii left a comment

Choose a reason for hiding this comment

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

굿이에용!

@hoyeonyy hoyeonyy merged commit 4e2447c into develop-backend Jul 24, 2024
1 check passed
@Mingyum-Kim Mingyum-Kim deleted the feature/#90 branch July 24, 2024 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE 백엔드 관련 이슈입니다. 🌱 기능추가 feature (새로운 기능 구현)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants