Conversation
byungwook-min
left a comment
byungwook-min
left a comment
There was a problem hiding this comment.
전체적으로 봤을 때 미션의 핵심을 잘 이해하고 구현했다는 인상을 받았습니다. 이전까지 만들어놓은 서비스 로직을 웹 API로 열어주는 것인데, 그 목적에 맞게 컨트롤러 레이어를 도메인별로 깔끔하게 분리하고, @RequestMapping을 활용해서 각 API를 잘 매핑해놓은 부분이 좋은 것 같습니다. 특히 컨트롤러가 비즈니스 로직 없이 서비스에 위임만 하는 형태가 잘 지켜지고 있습니다.
아쉬운 부분이 크게 두 가지 있는데, 첫 번째는 API의 응답으로 무엇을 내보내는가에 고민입니다. 조회 API에서는 UserDto, ChannelDto를 잘 사용하고 있는데, 생성이나 수정 API에서는 Entity를 그대로 반환하고 있습니다. 이는 Entity는 내부 구현 세부사항이고 데이터베이스에 어떤 필드가 있는지, 비밀번호가 어떤 형태로 저장되는지 같은 것들이 그대로 외부에 노출되는 것입니다. 컨트롤러에서 Entity를 직접 반환하지 않는다라는 원칙을 세워두면 좋을 것 같습니다. 모든 응답은 DTO를 통해서만 나가도록 하고 귀찮더라도 이 습관을 지금 잡아두면 좋을 것 같습니다.
두 번째는 예외 처리의 전체적인 그림입니다. GlobalExceptionHandler를 만들고 커스텀 예외를 처리하는 구조 자체는 훌륭하지만 전체 서비스를 놓고 봤을 때, 어떤 서비스는 커스텀 예외를 던지고, 어떤 서비스는 NoSuchElementException이나 IllegalArgumentException을 던지고 있어서, 예외가 일관되게 관리되지 않고 있습니다. 결국 GlobalExceptionHandler가 잡지 못하는 예외가 발생하면 클라이언트에게 500 에러가 그대로 전달되는 건데, 예외 처리를 설계할 때는 이 서비스에서 발생할 수 있는 모든 예외가 결국 어디서 처리되는가?라는 질문을 한번 해보면 좋을 것 같습니다. 커스텀 예외로 통일하든, GlobalExceptionHandler에 범용 핸들러를 추가하든, 어떤 방식이든 빈틈없이 잘 구성하면 좋을 것 같습니다.
그 외에 엔드포인트 네이밍의 일관성(단수/복수, prefix 유무 혼재), 코드 포맷팅의 일관성(공백, 빈 줄), 미사용 import 정리 같은 부분들도 있는데, 이런 것들은 기능적인 문제는 아니지만 코드의 완성도에 영향을 줍니다. IDE의 자동 포맷팅이나 린트 설정을 활용하는 습관을 들이면 별도의 노력 없이도 깔끔한 코드를 유지할 수 있습니다.
전체적인 방향성으로 보면, 지금까지 Entity → Repository → Service → Controller 순서로 레이어를 하나씩 쌓아오면서 흐름이 잘 쌓고 있으므로 다음 단계에서는 API를 사용하는 클라이언트 입장에서 이 응답이 충분한가?, 예상치 못한 상황에서 이 API가 어떻게 반응하는가?처럼 소비자 관점에서 API를 바라보는 연습을 하면 좋을 것 같습니다. 수고하셨습니다 :)
| import com.sprint.mission.discodeit.dto.request.MessageCreateRequest; | ||
| import com.sprint.mission.discodeit.dto.request.PublicChannelCreateRequest; | ||
| import com.sprint.mission.discodeit.dto.request.UserCreateRequest; | ||
| import com.sprint.mission.discodeit.entity.Channel; | ||
| import com.sprint.mission.discodeit.entity.ChannelType; | ||
| import com.sprint.mission.discodeit.entity.Message; | ||
| import com.sprint.mission.discodeit.entity.User; | ||
| import com.sprint.mission.discodeit.repository.ChannelRepository; | ||
| import com.sprint.mission.discodeit.repository.MessageRepository; | ||
| import com.sprint.mission.discodeit.repository.UserRepository; | ||
| import com.sprint.mission.discodeit.repository.file.FileChannelRepository; | ||
| import com.sprint.mission.discodeit.repository.file.FileMessageRepository; | ||
| import com.sprint.mission.discodeit.repository.file.FileUserRepository; | ||
| import com.sprint.mission.discodeit.service.ChannelService; | ||
| import com.sprint.mission.discodeit.service.MessageService; | ||
| import com.sprint.mission.discodeit.service.UserService; | ||
| import com.sprint.mission.discodeit.service.basic.BasicChannelService; |
There was a problem hiding this comment.
import 되어 있는데 실제로 사용되지 않고 있습니다. 사용하지 않는 import는 제거해주세요. IDE의 Optimize Imports 기능을 활용하면 편리합니다.
| @RequestMapping(method = RequestMethod.POST) | ||
| @ResponseBody | ||
| public User createUser(@RequestBody UserCreateRequest createRequest) { | ||
|
|
||
| return userService.create(createRequest); | ||
| } |
There was a problem hiding this comment.
createUser 메서드가 User Entity를 직접 반환하고 있는데, 이러면 응답 JSON에 password 필드가 그대로 포함됩니다. UserDto로 변환해서 반환하거나, 생성 전용 Response DTO를 만들어서 사용하면 좋을 것 같습니다.
| @RequestMapping(value = "/findAll", method = RequestMethod.GET) | ||
| @ResponseBody | ||
| public ResponseEntity<List<UserDto>> findAllUser(Model model) { | ||
| return ResponseEntity.ok(userService.findAll()); | ||
| } |
There was a problem hiding this comment.
findAllUser 메서드에 Model model 파라미터가 있는데, 이 메서드는 @ResponseBody로 JSON을 반환하는 API이므로 Model은 전혀 사용되지 않습니다. 불필요한 파라미터는 혼란을 줄 수 있으니 제거하면 좋을 것 같습니다.
| @RequestMapping(value = "/{id}",method = RequestMethod.PATCH) | ||
| @ResponseBody | ||
| public User updateUser(@PathVariable UUID id, | ||
| @RequestBody UserUpdateRequest userUpdateRequest) { | ||
|
|
||
| return userService.update(id,userUpdateRequest); | ||
| } |
There was a problem hiding this comment.
updateUser 메서드도 User Entity를 직접 반환하고 있습니다.
| @RequestMapping(value = "/{id}/status",method = RequestMethod.PATCH) | ||
| @ResponseBody | ||
| public UserDto updateOnline(@PathVariable UUID id, | ||
| @RequestBody UserStatusUpdateRequest userStatusUpdateRequest){ | ||
|
|
||
| userStatusService.update(id, userStatusUpdateRequest); | ||
| return userService.find(id); | ||
|
|
||
| } |
There was a problem hiding this comment.
userStatusService.update(id, userStatusUpdateRequest)를 호출하고 있는데, update 메서드의 첫 번째 파라미터는 userStatusId입니다. 하지만 여기서 전달하는 id는 URL 경로의 User ID입니다. updateByUserId 메서드를 호출하는 것이 맞을 것 같습니다.
| @Override | ||
| public UserStatus update(UUID userId, UserStatusUpdateRequest request) { | ||
| Instant newLastActiveAt = request.newLastActiveAt(); | ||
|
|
||
| UserStatus userStatus = userStatusRepository.findByUserId(userId) | ||
| .orElseThrow(() -> new NoSuchElementException("UserStatus with id " + userId + " not found")); | ||
| userStatus.update(newLastActiveAt); | ||
|
|
||
| return userStatusRepository.save(userStatus); | ||
| } | ||
|
|
||
| @Override | ||
| public UserStatus updateByUserId(UUID userId, UserStatusUpdateRequest request) { | ||
| Instant newLastActiveAt = request.newLastActiveAt(); | ||
|
|
||
| UserStatus userStatus = userStatusRepository.findByUserId(userId) | ||
| .orElseThrow(() -> new NoSuchElementException("UserStatus with userId " + userId + " not found")); | ||
| userStatus.update(newLastActiveAt); | ||
|
|
||
| return userStatusRepository.save(userStatus); | ||
| } |
There was a problem hiding this comment.
update(UUID userId, ...) 메서드와 updateByUserId(UUID userId, ...) 메서드가 거의 동일한 로직인 것 같습니다. 둘 다 findByUserId로 조회하고 있어 하나를 제거하거나, update의 첫 번째 파라미터를 userStatusId로 명확히 하고 findById를 사용하도록 분리하는게 좋을 것 같습니다.
| public User(String username, String email, String password, UUID profileId) { | ||
| this.id = UUID.randomUUID(); | ||
| this.createdAt = Instant.now(); | ||
| // | ||
| this.username = username; | ||
| this.email = email; | ||
| this.password = password; | ||
| this.profileId = profileId; | ||
| } |
There was a problem hiding this comment.
생성자에서 updatedAt을 초기화하지 않아서 null 상태로 남습니다. 조회 시 updatedAt이 null로 반환될 수 있어서,createdAt과 동일한 값으로 초기화하거나, DTO 변환 시 null 처리를 하는 것이 좋을 것 같습니다. Channel, Message, ReadStatus, UserStatus 엔티티도 동일한 이슈가 있습니다.
| public Boolean isOnline() { | ||
| Instant instantFiveMinutesAgo = Instant.now().minus(Duration.ofMinutes(5)); | ||
|
|
||
| return lastActiveAt.isAfter(instantFiveMinutesAgo); | ||
| } |
There was a problem hiding this comment.
반환 타입이 Boolean(Wrapper)입니다. boolean(primitive)을 사용하면 auto-boxing 오버헤드도 없고 null 가능성도 배제할 수 있어요. Boolean을 사용할 명확한 이유가 없다면 boolean을 권장합니다.
| @Getter | ||
| @AllArgsConstructor | ||
| public class ErrorResponseDto { | ||
| private String code; | ||
| private String message; | ||
| } |
There was a problem hiding this comment.
다른 DTO들(UserDto, ChannelDto, 모든 Request DTO)은 Java Record로 만들어져 있는데, ErrorResponseDto만 Lombok을 사용한 일반 클래스로 되어 있네요. Record로 통일하면 일관성도 좋아지고 코드도 더 간결해질 것 같습니다.
| @Controller | ||
| public class MainController { | ||
| @GetMapping("/") | ||
| public String userListPage() { | ||
| return "user-list.html"; | ||
| } | ||
| } |
There was a problem hiding this comment.
다른 컨트롤러에서는 요구사항에 맞게 @RequestMapping(method = ...) 형태를 사용하고 있는데, MainController에서는 @GetMapping을 사용하고 있습니다. 기능적으로는 동일하지만, 프로젝트 전체에서 한 가지 스타일로 통일하는 것이 좋겠습니다.
|
그리고 Conflicts가 발생하고 있는데, 이 부분 해결해주시면 감사하겠습니다! |
2 participants
웹 API 요구사항
사용자 관리
권한 관리
채널 관리
메시지 관리
메시지 수신정보 관리
바이너리 파일 다운로드
Post Man
심화 요구사항
정적 리소스 서빙