-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: 어드민 멤버 정보 수정 api 구현 #39
Conversation
} | ||
|
||
private void updateStudentId(String studentId) { | ||
validateStringNotNull(studentId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
업데이트 시 not null 정책은 dto 단에서 검증하는 것으로 하면 어떨까요?
도메인에서 입력값 유효성 검증까지 처리하게 되면 로직이 너무 복잡해지는 것도 있어서,
입력값 신택스보다는 시멘틱스 위주로 처리하는 쪽으로 가면 좋을 것 같습니다.
@@ -31,4 +35,11 @@ public ResponseEntity<Void> withdrawMember(@PathVariable Long memberId) { | |||
memberService.withdrawMember(memberId); | |||
return ResponseEntity.ok().build(); | |||
} | |||
|
|||
@PutMapping("/{memberId}") | |||
public ResponseEntity<Void> withdrawMember( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드 이름이 잘못되었습니다.
this.name = name; | ||
} | ||
|
||
private void updatePhone(String phone) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전화번호는 굳이 하이픈 붙여서 저장할 필요는 없을 것 같아요.
DB에는 제거해서 넣고 나중에 프론트로 내려줄 때 변환해주든 하는 게 나을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
db에 넣을 때 제거해서 넣으면 좋은 점이 있나요?
의도가 무엇인지 궁금합니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
010-1234-5678
에 대해서 4567
이라는 키워드로 검색한다고 칩시다.
사용자는 010-1234-5678
이 검색될 것을 기대하지만 실제로는 4-567
과 4567
이 일치하지 않기 떄문에 검색되지 않습니다.
해당 문제를 방지하기 위해서는 하이픈 없이 저장해야 합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
검색하려면 필요하겠네요👍
수정하겠습니다!
Member member = | ||
memberRepository.findById(memberId).orElseThrow(() -> new CustomException(ErrorCode.MEMBER_NOT_FOUND)); | ||
|
||
member.validateStatus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떤 Status임을 검증하는지 궁금한데, "삭제된 유저가 아님을 검증"으로 바꾸는건 어떨까요?
일부러 "상태를 검증한다"로 어떤 상태를 검증하는 것인지 숨긴거라면,
"업데이트가 가능한 상태인지 검증"으로 바꾸는건 어떤가요?
저만의 생각이라 혹시 의도가 있는 거라면 알려주세요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validateStatusUpdatable
로 수정했습니다.
이와 별개로 validateStatusUpdatable
을 member.updateMemberInfo
안에서 호출하는게 맞을지 아니면 밖에서 먼저 호출하는게 맞을지 고민되는데 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우선은 member.updateMemberInfo
안에서 호출하도록 수정했습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 Member가 업데이트를 해줄 수 있는 상태인지 확인하는거라 member.updateMemberInfo
이런 형태가 좋습니다 ㅎㅎ
@@ -4,4 +4,8 @@ public record ErrorResponse(String errorCodeName, String errorMessage) { | |||
public static ErrorResponse of(ErrorCode errorCode) { | |||
return new ErrorResponse(errorCode.name(), errorCode.getMessage()); | |||
} | |||
|
|||
public static ErrorResponse of(String errorCodeName, String errorMessage) { | |||
return new ErrorResponse(errorCodeName, errorMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GlobalExceptionHandler에서 사용하는 방식을 보긴 했는데,
ErrorCode와 Message를 받지 않고, 이름을 String으로 바로 받는 이유가 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다른 handler 메서드에서는
return ResponseEntity .status(ErrorCode.INTERNAL_SERVER_ERROR.getStatus()) .body(ErrorResponse.of(ErrorCode.INTERNAL_SERVER_ERROR));
와 같이 ErrorCode를 바로 넘겼는데 해당 handler 메서드에서는 Validation 어노테이션이 넘겨주는 default message를 이용하려고 했습니다.
더 좋은 방법이 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 취향 차이일 수 있는데,
저는 만약에 Message를 받아서 사용하고 싶은 것이라면 아래와 같은 형태가 어떤가 했어요
public static ErrorResponse of(ErrorCode errorCode, String errorMessage) {
return new ErrorResponse(errorCode.name(), errorMessage);
errorCodeName도 자유롭게 사용하려면 지금 짜주신 방법이 더 좋은 것 같구요 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 제가 잘못 이해했었네요
말씀해주신 방법이 GlobalExceptionHandler 쪽에서 더 깔끔할 것 같아서 수정했습니다
감사합니다!
Member member = | ||
memberRepository.findById(memberId).orElseThrow(() -> new CustomException(ErrorCode.MEMBER_NOT_FOUND)); | ||
|
||
member.validateStatus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 Member가 업데이트를 해줄 수 있는 상태인지 확인하는거라 member.updateMemberInfo
이런 형태가 좋습니다 ㅎㅎ
@@ -4,4 +4,8 @@ public record ErrorResponse(String errorCodeName, String errorMessage) { | |||
public static ErrorResponse of(ErrorCode errorCode) { | |||
return new ErrorResponse(errorCode.name(), errorCode.getMessage()); | |||
} | |||
|
|||
public static ErrorResponse of(String errorCodeName, String errorMessage) { | |||
return new ErrorResponse(errorCodeName, errorMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 취향 차이일 수 있는데,
저는 만약에 Message를 받아서 사용하고 싶은 것이라면 아래와 같은 형태가 어떤가 했어요
public static ErrorResponse of(ErrorCode errorCode, String errorMessage) {
return new ErrorResponse(errorCode.name(), errorMessage);
errorCodeName도 자유롭게 사용하려면 지금 짜주신 방법이 더 좋은 것 같구요 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
🌱 관련 이슈
📌 작업 내용 및 특이사항
📝 참고사항
📚 기타