COMMON: 7기 통합 온보딩 제출(v2) #32
Conversation
… fe/onboarding_v2
… fe/onboarding_v2
|
route가 public private으로 나뉘는데, 이에 관련해서 전반적인 설명해주세용 의도를 알고 싶네요! |
덕담 보기 페이지 처음 들어왔을때 더보기 버튼 보이는데 다른 탭 갔다 돌아오면 없어집니다..! |
|
코멘트 남긴 부분 제외 테스트했을때 잘 동작하네용!! 까비 스타일 물씬나게 구현하셨네요 ㅎㅎ 저와는 다르게 구현하신 부분들, 낯선 리액트 훅 사용 등 리뷰하면서 공부가 되었습니다아 pr 리뷰 열심히 했으니 코멘트 확인하기 +-+ 모두모두 고생하셨습니당~! |
| private void verifyValidMessageFormat(String context) { | ||
| if (context.isEmpty() | ||
| || context.length() > MESSAGE_LENGTH_LIMIT) { | ||
| throw ExceptionStatus.INVALID_FORMAT_MESSAGE.asGreetingException(); | ||
| } | ||
| } |
There was a problem hiding this comment.
메시지 검증 로직을 도메인 클래스(Message)에서 수행하는 방식도 괜찮았을 것 같습니다.
현재 패키지 구조가 Domain-Driven 구조(최상단에 도메인이 위치하고, 그 하위에 controller, service 등이 존재)인 만큼, 값 검증의 책임도 도메인 객체가 갖는 것이 자연스럽다고 생각했어요.
특히, Message 클래스 내에서 이미 MAX_LENGTH를 사용한 검증 로직이 이미 존재하므로, 검증을 서비스가 아닌 도메인 객체에서 처리하는 방향이 일관성 측면에서도 괜찮다고 생각했어요.
현재 방식도 충분히 좋은 접근이라고 생각합니다! 😊
There was a problem hiding this comment.
그러네요 이미 Message 클래스 내에서 검증 로직이 존재하는데 서비스에 검증 로직을 또 만들어놓고 있었네요! 감사합니다 다음부터는 일관성에 대해서 주의깊게 생각하고 짜보겠습니다!!
| String extension = originalFilename.contains(".") ? originalFilename.substring( | ||
| originalFilename.lastIndexOf(".")) : ""; | ||
| verifyExtensionType(extension); | ||
| String safeFileName = originalFilename.length() > 10 ? originalFilename.substring(0, 10) : originalFilename; | ||
| String safeFileName = originalFilename.length() > 10 ? originalFilename.substring(0, 10) | ||
| : originalFilename; |
There was a problem hiding this comment.
메시지 처리와 직접적으로 관련있는 로직이 아니므로, 해당 함수 자체를 별도의 모듈로 추출하는 것도 괜찮을 것 같습니다!
중요한 부분은 아니지만, 되도록이면 리터럴 대신에 상수 사용을 지향하는게 좋을 것 같아요.
There was a problem hiding this comment.
매직넘버를 코드에 넣어버렸네요. 주의하면서 코드를 작성하는 습관을 들여야 겠어요:) 감사합니다!
| private Page<Message> getMessagesFromMe(String userName, PageRequest pageRequest) { | ||
| return messageRepository.findAllBySenderName(userName, pageRequest); | ||
| } |
There was a problem hiding this comment.
구체적인 클래스인 PageRequest 대신에 인터페이스인 Pageable 을 사용하는 편이 좋을 것 같습니다.
"객체는 인터페이스를 사용하여 참조해라" 이펙티브 자바에 나오는 내용이니 참고하시면 좋을 것 같아요.
There was a problem hiding this comment.
헉 기본적인걸 놓치고 있었네요 링크 걸어주신 내용도 잘 봤습니다! 다음엔 이펙티브 자바도 정독해보겠습니다 🫡
| private void verifyUserAuthorized(String userName, Message message) { | ||
| if (!message.getSenderName().equals(userName)) { | ||
| throw ExceptionStatus.UNAUTHORIZED.asGreetingException(); | ||
| } | ||
| } |
There was a problem hiding this comment.
인증이 되지 않은 상황이 아니라, 인증은 되었지만(서비스까지 왔다면 인증은 되었을 것) 정책적으로 허용되지 않은 케이스인 것으로 보여서,UNAUTHORIZED 대신에 FORBIDDEN 을 던지는 것이 조금 더 restful 하다고 생각합니다!
There was a problem hiding this comment.
restful에 대해 너무 막연하게 생각하고 있었던것 같습니다. 좀 더 적절한 에러코드를 선택하도록 고민하겠습니다!
There was a problem hiding this comment.
UNAUTHORIZED와 FORBIDDEN 중 어떤 status로 반환할 지 고민이었는데, 판단 기준에 대해 알려주셔서 감사합니다 !
There was a problem hiding this comment.
- https://restfulapi.net/http-status-codes/
- https://restfulapi.net/http-methods/
- https://restfulapi.net/resource-naming/
restful api 가이드 문서인데, 저는 이거 읽고 이해가 많이 됐어요. 참고하시면 좋을 것 같아 공유드려요.
| @GetMapping("/search/group") | ||
| public ResponseEntity<?> searchGroup(@RequestParam(name = "input") String input) { | ||
|
|
||
| GroupSearchDto groups = userService.searchGroupByName(input); | ||
|
|
||
| return ResponseEntity.ok() | ||
| .body(groups); |
There was a problem hiding this comment.
질문: 리턴 타입을 GroupSearchDto 로 명시하지 않아도 Swagger 예시가 들어가나요?
There was a problem hiding this comment.
Swagger를 사용하고 있지 않아서 신경을 못썼습니다! 찾아보니 <?> 말고 로 명시를 해줘야 Swagger에서 응답 객체의 스키마를 제대로 추론할 수 있다고 하네요. 앞으로 제대로 명시하는 습관을 들이겠습니다!
| @Getter | ||
| @AllArgsConstructor | ||
| public class GroupSearchDto { | ||
|
|
||
| List<String> GroupNames; | ||
| } |
There was a problem hiding this comment.
단순한 내용이라 상관없을 것 같지만, 복잡한 DTO라면 어노테이션으로 스웨거 example을 명시해주면 좋을 것 같습니다!
There was a problem hiding this comment.
Example이 있으면 가독성이 훨씬 좋아지겠네요! 조언해주셔서 감사합니다 :)
| } | ||
|
|
||
| private void verifyNameIsNumericOrAlphabet(String name) { | ||
| if (name == null || !name.matches("^[a-zA-Z0-9]+$")) { |
There was a problem hiding this comment.
String.matches를 사용하면 호출할 때마다 내부적으로 Pattern 객체가 생성되어 상황에 따라 비효율적일 수 있습니다.
import java.util.regex.Pattern;
public class Validator {
private static final Pattern NAME_PATTERN = Pattern.compile("^[a-zA-Z0-9]+$");
public static boolean isValidName(String name) {
return NAME_PATTERN.matcher(name).matches();
}
}
Pattern 객체를 미리 상수로 정의해두고 사용하는 방식을 고민해보면 좋을 것 같아요.
이펙티브 자바에도 나오는 내용이니 참고하시면 좋을 것 같아요.
There was a problem hiding this comment.
Pattern 객체를 static으로 만들어놓고 재사용 하면 되네요! 생각지도 못한 부분이었습니다. 알려주셔서 감사합니다!!
There was a problem hiding this comment.
String.matches 메소드가 객체를 생성할 거라곤 생각 못했네요. 자주 사용하는 함수들의 내부 구현에 대해 파악해야 한다는 걸 다시 한번 깨닫습니다. 감사합니다:)
There was a problem hiding this comment.
이펙티브 자바를 한번 꼼꼼히 공부해보고 기본적인 내용은 더 확실히 익혀두겠습니다!!
| verifyDuplicatedName(name); | ||
| verifyNameLength(name); | ||
| verifyNameIsNumericOrAlphabet(name); // 하나의 메서드로 따로 빼는게 나을까? 코드만 복잡해지는게 아닌지? |
There was a problem hiding this comment.
검증에 대한 책임을 도메인 객체(User)에게 맡기고, 서비스에서는 호출하는 방식을 사용하면, 서비스 측 코드를 간결하게 사용할 수도 있을 것 같아요.
There was a problem hiding this comment.
검증 책임을 도메인 객체에 맡기면 되는군요.. 도메인 객체의 책임은 어디까지인지, 아예 검증 책임을 맡을 객체를 하나 더 만들어야 할지 또 고민이 되네요. 감사합니다!
| const handleRegister = async () => { | ||
| if (!idRegex.test(id)) { | ||
| alert("형식에 맞지 않는 아이디입니다."); | ||
| return; | ||
| } | ||
| if (!pwRegex.test(pw)) { | ||
| alert("형식에 맞지 않는 비밀번호입니다."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
올바른 형식이 무엇인지 설명이 들어간다면 유저 경험이 더 향상되지 않을까 생각합니다!
| if (!inputFile.name.match(/\.(jpg|jpeg|png)$/)) { | ||
| target.value = ""; | ||
| alert("이미지 파일만 업로드 가능합니다."); | ||
| return; | ||
| } |
이 부분 다시 한번 살펴보겠습니다. 데이터를 많이 집어넣고 테스트 해봐서 미처 몰랐네요. |
Screen Recording 2025-01-31 at 7.30.23 PM.zip
해당 사항 (중복 선택)
Screen.Recording.2025-01-31.at.7.30.23.PM-2.mov
실행 방법
이때, env 파일은 온보딩 api 명세서 페이지에 있습니다!