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

[Feature/17] 자기소개 등록하기 API를 구현한다 #18

Merged
merged 3 commits into from
Jul 22, 2024

Conversation

injoon2019
Copy link
Collaborator

@injoon2019 injoon2019 commented Jul 21, 2024

💡 이슈 번호

close: #17

✨ 작업 내용

자기소개 등록하기 api 구현

🚀 전달 사항

@injoon2019 injoon2019 self-assigned this Jul 21, 2024
Comment on lines 28 to 39
profileRepository.findByUserId(user.id)?.let {
it.user = user
it.profileSelect = it.profileSelect
it.introduction = it.introduction
} ?: run {
profileRepository.save(
UserProfile(
user = user,
profileSelect = profileSelect,
)
)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

upsert가 가능하게 짰어요. 이렇게 하면 수정 기능을 붙일 때 추가로 개발해야할 것이 없을거 같아요

Comment on lines 45 to 63
@Transactional
fun saveIntroduction(introduction: List<QuestionAndAnswer>) {
// TODO User 회원 가입 기능 구현후 수정
val user = userRepository.findByIdOrNull(1L)
if (user != null) {
user.userProfile?.introduction = introduction

profileRepository.findByUserId(user.id)?.let {
it.user = user
it.profileSelect = it.profileSelect
it.introduction = it.introduction
} ?: run {
profileRepository.save(
UserProfile(
user = user,
introduction = introduction
)
)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

객체지향스럽게 짜는 코드가 정말 생각이 안나네요 혹시 살짝 알려주시면 해보겠습니다!
하면서 생각이 날랑말랑하는데 이럴 때는 user.userProfile도 업데이트하고 UserProfile은 또 따로 업데이트 했어야했나 이런것들이 헷갈리네요..ㅋㅋ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그리고 요런 코드 보신 후기가 어떠신가요ㅋㅋ 우려스러운 부분이 궁금해요 (이렇게 하자 제안 x. 요렇게 짜는 편리함에 중독된 입장에서 다른 시선이 궁금해요ㅋㅋ)

Copy link
Member

Choose a reason for hiding this comment

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

user.userProfile도 업데이트하고 UserProfile은 또 따로 업데이트 했어야했나

user.userProfile을 변경하면 jpa의 변경감지 때문에 자동으로 update 쿼리가 날아가서 따로 업데이트를 하지 않아도 돼요! (이걸 말씀하시는게 맞나요..?)

그런데 위 코드에서 50번째 줄은 굳이 없어도 잘 동작할 것 같아요! 어짜피 아래에서 무조건 profileRepository.findByUserId(user.id) 를 실행하기 때문에요!

Copy link
Member

Choose a reason for hiding this comment

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

음 코드가 사실 엘비스 연산자 쓰고 이런거는 코틀린 언어 특징이라서 익숙해진다면 읽기에는 괜찮을 것 같아요!
그리고 insert 와 update를 같이 한번에 처리하려면 저렇게 나올 수 밖에 없을 거 같아요!

여기서 그냥 조금 더 가독성이 좋게 한다면 아래처럼 할 수 있을거같아요!

val user = userRepository.findByIdOrNull(1L) ?: throw IllegalStateException ("회원가입 상태를 문의해주세요")

profileRepository.findByUserId(user.id)?.let {
    it.introduction = introduction
} ?: run {
    profileRepository.save(
        UserProfile(
            user = user,
            introduction = introduction
        )
    )
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

profileRepository.findByUserId(user.id)

요렇게 안하지 않나요? 보통 어떻게 하셨는지 궁금해요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

크 기가 막힙니다!

Copy link
Member

Choose a reason for hiding this comment

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

아하!!
만약에 User가 UserProfile을 가지고 있도록 설계했다면

profileRepository.findByUserId(user.id)

이걸 안하고, user만 조회해도 userProfile을 함께 가져올 수 있어서 좀 더 객체지향스럽게 할 수 있을 것 같네요!
그러면 양방향 연관관계를 걸어야하는데 굳이 User가 UserProfile을 들고 있지 않아도 될 것 같아서 그냥 저렇게 하면 될 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오호 양방향 관계 걸고 안걸고의 기준이 궁금하네요 요거 담에 얘기해보시죠!

Copy link
Member

@miseongk miseongk left a comment

Choose a reason for hiding this comment

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

고생하셨어요 🙇‍♀️🙇‍♀️🙇‍♀️
수정해야 할 부분이 있어서 RC 할게요!

Comment on lines 29 to 31
it.user = user
it.profileSelect = it.profileSelect
it.introduction = it.introduction
Copy link
Member

Choose a reason for hiding this comment

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

이 부분 잘못된거 같아요! profile이 존재한다면 it.profileSelect = profileSelect 이렇게 해야하지 않나요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

헉 맞습니다 발견해주셔서 감사합니다!

Comment on lines 53 to 55
it.user = user
it.profileSelect = it.profileSelect
it.introduction = it.introduction
Copy link
Member

Choose a reason for hiding this comment

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

요기도 it.introduction = introduction 으로 바꿔야 할 것 같아요!

Comment on lines 45 to 63
@Transactional
fun saveIntroduction(introduction: List<QuestionAndAnswer>) {
// TODO User 회원 가입 기능 구현후 수정
val user = userRepository.findByIdOrNull(1L)
if (user != null) {
user.userProfile?.introduction = introduction

profileRepository.findByUserId(user.id)?.let {
it.user = user
it.profileSelect = it.profileSelect
it.introduction = it.introduction
} ?: run {
profileRepository.save(
UserProfile(
user = user,
introduction = introduction
)
)
}
Copy link
Member

Choose a reason for hiding this comment

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

user.userProfile도 업데이트하고 UserProfile은 또 따로 업데이트 했어야했나

user.userProfile을 변경하면 jpa의 변경감지 때문에 자동으로 update 쿼리가 날아가서 따로 업데이트를 하지 않아도 돼요! (이걸 말씀하시는게 맞나요..?)

그런데 위 코드에서 50번째 줄은 굳이 없어도 잘 동작할 것 같아요! 어짜피 아래에서 무조건 profileRepository.findByUserId(user.id) 를 실행하기 때문에요!

Comment on lines 45 to 63
@Transactional
fun saveIntroduction(introduction: List<QuestionAndAnswer>) {
// TODO User 회원 가입 기능 구현후 수정
val user = userRepository.findByIdOrNull(1L)
if (user != null) {
user.userProfile?.introduction = introduction

profileRepository.findByUserId(user.id)?.let {
it.user = user
it.profileSelect = it.profileSelect
it.introduction = it.introduction
} ?: run {
profileRepository.save(
UserProfile(
user = user,
introduction = introduction
)
)
}
Copy link
Member

Choose a reason for hiding this comment

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

음 코드가 사실 엘비스 연산자 쓰고 이런거는 코틀린 언어 특징이라서 익숙해진다면 읽기에는 괜찮을 것 같아요!
그리고 insert 와 update를 같이 한번에 처리하려면 저렇게 나올 수 밖에 없을 거 같아요!

여기서 그냥 조금 더 가독성이 좋게 한다면 아래처럼 할 수 있을거같아요!

val user = userRepository.findByIdOrNull(1L) ?: throw IllegalStateException ("회원가입 상태를 문의해주세요")

profileRepository.findByUserId(user.id)?.let {
    it.introduction = introduction
} ?: run {
    profileRepository.save(
        UserProfile(
            user = user,
            introduction = introduction
        )
    )
}

Comment on lines +50 to +40
data class QuestionAndAnswer(
val question: String,
val answer: String,
)
Copy link
Member

Choose a reason for hiding this comment

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

controller에서 사용하는 dto를 도메인에서 가지고 있는게 맞는가 고민이 되네요!
dto는 변경될 가능성이 있으니까 도메인에 가지고 있으면 안된다고 생각하는데, 지금은 db에 json 자체를 집어넣는거라서 잘 모르겠어요..! 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이거 앞에 것들 디벨롭에 머지되면 rebase해서 한번에 반영할게요! 파사드 클래스에 두면 좋을거 같아요!

@injoon2019 injoon2019 changed the base branch from feature/15 to feature/12 July 22, 2024 11:15
Base automatically changed from feature/12 to develop July 22, 2024 11:56
@injoon2019
Copy link
Collaborator Author

앗 이전에 squash merge를 안했네요 앞으로 squash merge 할까요?

@injoon2019 injoon2019 force-pushed the feature/17 branch 2 times, most recently from 70a6a7c to 4dde7a8 Compare July 22, 2024 12:12
Copy link
Member

@miseongk miseongk left a comment

Choose a reason for hiding this comment

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

squash merge는 develop <- feature 머지할때만 하는거죠?!
저번에 무슨 상황인지 기억은 잘 안나는데 squash merge를 잘못썼는지 머지하고 나서 브랜치를 삭제해야해서 불편했었거든요,,!

Comment on lines +27 to +29
it.user = user
it.profileSelect = profileSelect
it.introduction = it.introduction
Copy link
Member

Choose a reason for hiding this comment

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

여기서 27, 29 line은 없어도 되지 않나요..?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

없앴습니다!

Comment on lines 45 to 63
@Transactional
fun saveIntroduction(introduction: List<QuestionAndAnswer>) {
// TODO User 회원 가입 기능 구현후 수정
val user = userRepository.findByIdOrNull(1L)
if (user != null) {
user.userProfile?.introduction = introduction

profileRepository.findByUserId(user.id)?.let {
it.user = user
it.profileSelect = it.profileSelect
it.introduction = it.introduction
} ?: run {
profileRepository.save(
UserProfile(
user = user,
introduction = introduction
)
)
}
Copy link
Member

Choose a reason for hiding this comment

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

아하!!
만약에 User가 UserProfile을 가지고 있도록 설계했다면

profileRepository.findByUserId(user.id)

이걸 안하고, user만 조회해도 userProfile을 함께 가져올 수 있어서 좀 더 객체지향스럽게 할 수 있을 것 같네요!
그러면 양방향 연관관계를 걸어야하는데 굳이 User가 UserProfile을 들고 있지 않아도 될 것 같아서 그냥 저렇게 하면 될 것 같아요!

@injoon2019
Copy link
Collaborator Author

squash merge는 develop <- feature 머지할때만 하는거죠?! 저번에 무슨 상황인지 기억은 잘 안나는데 squash merge를 잘못썼는지 머지하고 나서 브랜치를 삭제해야해서 불편했었거든요,,!

넵!!

@injoon2019 injoon2019 merged commit 9f6e52b into develop Jul 22, 2024
1 check passed
@injoon2019 injoon2019 deleted the feature/17 branch July 22, 2024 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

자기소개 등록하기 api를 구현한다
2 participants