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

Member 생성, 조회, 수정 구현 #12

Merged
merged 13 commits into from
Jan 22, 2025
Merged

Member 생성, 조회, 수정 구현 #12

merged 13 commits into from
Jan 22, 2025

Conversation

yeeunli
Copy link
Collaborator

@yeeunli yeeunli commented Jan 17, 2025

  • commit 세분화 못했는데 다음부턴 잘 하겠습니다!..

import java.util.ArrayList;
import java.util.List;

@Entity
@Builder
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@AllArgsConstructor
Copy link
Collaborator

Choose a reason for hiding this comment

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

builder 로만 생성가능하도록
access = private 로 추가하면 어떨까요

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
Collaborator

Choose a reason for hiding this comment

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

DTO 는 record 로 바꾸는게 권장드리옵니다

Copy link
Collaborator

Choose a reason for hiding this comment

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

이것도 record 로 바꿔드리기를 간청드리옵니다

Copy link
Collaborator

Choose a reason for hiding this comment

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

class 에 transactional readOnly 걸고, write 하는 메소드만 다시 어노테이션 거는게 어떠하옵니까

* @return 성공하면 생성 성공 메시지와 생성된 회원의 ID 반환
*/
@PostMapping
public ResponseEntity<?> createMember(@RequestBody MemberCreateRequestDto request) {
Copy link
Collaborator

@cmj7271 cmj7271 Jan 21, 2025

Choose a reason for hiding this comment

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

<?> 를 쓰신 의중이 궁금하옵니다


memberService.updateMember(memberId, request);

return ResponseEntity.status(HttpStatus.NO_CONTENT).body("프로필 수정 성공");
Copy link
Collaborator

Choose a reason for hiding this comment

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

body 에 메세지를 적지않아도 충분해보이옵니다
그리고, 204 No Content 는 찾아보니 Res Body 가 없는 Res 라고 한다하옵니다
따라서 이대로면 body 가 없는게 맞는거 같습니다요
데이터가 업데이트 된 것이니 새로 리셋하라는 의미에서
205 Reset Content 가 더 적절해보이옵니다


@Entity
@Builder
@ToString
@NoArgsConstructor
Copy link
Collaborator

@cmj7271 cmj7271 Jan 21, 2025

Choose a reason for hiding this comment

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

noArg 에는 protected(JPA 명세서가 private 는 허용하지 않습니다) 로 제한하여
builder 로만 만들도록 조정하는 것이 어떠하옵니까

@@ -25,4 +31,16 @@ public class Member {
private String name;
private String github;
private String studentNumber;

public void updateMember(String name, String github, String studentNumber) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

null 검사는 서비스 단계에서 발생가능한 부분을 Optional 로 감싸서
Optional 사용하는 곳에서 처리하는 것이 어떠하옵니까

import lombok.Getter;

@Getter
public class MemberCreateRequestDto {
Copy link
Collaborator

Choose a reason for hiding this comment

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

record 사용을 권장드리옵니다

Copy link
Collaborator

Choose a reason for hiding this comment

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

e379980#r1923096926 와 같사옵니다

Copy link
Collaborator

@cmj7271 cmj7271 left a comment

Choose a reason for hiding this comment

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

LGTM

@cmj7271 cmj7271 merged commit 487ae2e into main Jan 22, 2025
1 check passed
cmj7271 added a commit that referenced this pull request Jan 22, 2025
#12 main 에서 develop 으로 변경
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.

3 participants