Skip to content

Conversation

@chaeeun1103
Copy link
Contributor

#️⃣연관된 이슈

ex) close #이슈번호

📝작업 내용

이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)

스크린샷 (선택)

💬리뷰 요구사항(선택)

리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요

ex) 메서드 XXX의 이름을 더 잘 짓고 싶은데 혹시 좋은 명칭이 있을까요?

@chaeeun1103 chaeeun1103 self-assigned this Nov 22, 2024
Copy link
Member

@jang-namu jang-namu left a comment

Choose a reason for hiding this comment

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

4주차 고생많으셨습니다!
단순한 시스템인데도 불구하고 나날이 코드베이스가 커져가는게 느껴지시나요??
그럴수록 코드베이스를 일관되게 유지하는 네이밍 규칙이나, 정확한 반환 타입 지정, 문서화가 중요해지죠.

4주차 과제는 로깅과 예외처리 관련한 부분이 많았죠.
커스텀 예외 사용과 로깅을 꼼꼼하게 작성하신 점이 인상깊었습니다!
코드를 보면서 궁금한 부분, 조금 아쉬운 부분을 찾아 몇 가지 코멘트를 달았어요.

확인해보시고, 5주차도 화이팅하시길 바랍니다!

  • 추가적으로 @Transcational(readOnly=true)에 대해 알아보시는 것도 도움이 되실것 같아요!

Comment on lines 13 to 16
@Validated
@RestController
@RequestMapping("/books")
public class BookController {
Copy link
Member

Choose a reason for hiding this comment

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

BookController에 Validated를 달아주셨네요.
따로 이유가 있는 걸까요??
Valid와 Validated의 차이와 Validated가 어떻게 동작하는지 되짚어볼까요?

Copy link
Contributor Author

@chaeeun1103 chaeeun1103 Nov 29, 2024

Choose a reason for hiding this comment

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

@Valid는 주로 컨트롤러 계층에서의 유효성 검증에서 사용되며 메소드 파라미터에 @Valid 어노테이션을 붙여 검증을 수행합니다.
@validated는 컨트롤러 이외에도 다른 계층에서도 사용되며 제약 조건이 적용될 검증 그룹을 지정할 수 있습니다.
이 도서 관리 시스템에서 아직까지 @Valid를 사용해서만 유효성 검증을 진행했는데 추가로 넣어야할 유효성 검증이 있을까요??

Copy link
Member

Choose a reason for hiding this comment

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

@validated를 이용한 그룹검증도 사실 복잡도가 높아져서 많이들 지양하는 편입니다.
컨트롤러가 아닌 다른 계층에서 메서드 파라미터를 검증하는 용도로 사용할수는 있으나,
저도 아직까지 그러한 요구사항을 만족해야 하는 경우를 접해보지 못해서.. 어렵네요 ㅎㅎ

Comment on lines 20 to 26
@PostMapping
public ResponseEntity<RentalDTO> rentBook(@RequestBody RentalDTO rentalDTO) {
log.info("책 대여 요청: memberId={}, bookId={}", rentalDTO.getMemberId(), rentalDTO.getBookId());
RentalDTO createdRentalDTO = rentalService.registerRental(rentalDTO.getMemberId(), rentalDTO.getBookId());
log.info("책 대여 성공 : {}", createdRentalDTO);
return ResponseEntity.ok(createdRentalDTO);
}
Copy link
Member

Choose a reason for hiding this comment

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

전체적으로 깔끔하게 로그를 작성하셨네요!
운영상의 이유로 로깅 코드를 작성은 불가피하지만, 로깅을 위한 코드를 작성하게 되면 로직과 섞이는 문제가 생기게되죠.
이러한 경우, 코드 베이스를 깔끔하게 유지하기 위해 어떤 방식으로 로깅을 걷어낼 수 있을까요?
아직 나오지않은 주제이지만 미리 고민해보셔도 좋을 것 같습니다!

Copy link
Contributor Author

@chaeeun1103 chaeeun1103 Nov 25, 2024

Choose a reason for hiding this comment

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

운영상 로깅은 필수적인 요소이지만, 로킹 코드가 비즈니스 로직과 섞이면 코드의 가독성과 유지보수성이 저하됩니다. 이를 해결하기 위해 AOP(Aspect-Oriented Programming)을 활용하여 로깅 책임을 분리하는 방식을 적용했습니다.
LogAspect 클래스 구현을 해 Controller, Service 패키지의 메소드 호출 전후에 로깅을 수행하도록 설정하였습니다. 이를 통해 기존에 각 클래스에 분산되어 있던 로깅 코드를 제거함으로써 코드베이스가 간결해졌습니다. 이 방식은 로깅 로직을 중앙애서 관리할 수 있게 해줍니다.

Copy link
Member

Choose a reason for hiding this comment

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

굳, 아주 정확합니다!😁

Book book = bookRepository.findById(bookId)
.orElseThrow(() -> new IllegalArgumentException(bookId + " 는 존재하지 않는 책입니다."));
.orElseThrow(() -> {
log.error("존재하지 않는 회원으로 대여에 실패했습니다. bookId={}", bookId);
Copy link
Member

Choose a reason for hiding this comment

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

로깅 메시지가 잘못된 것 같아요!

Comment on lines 51 to 54
if(rentalRepository.findByBookAndReturnDateIsNull(book).isPresent()){
log.warn("이미 대여중인 책은 대여가 불가합니다. bookId={}", bookId);
throw new AlreadyRentedException("이 책은 이미 대여중이므로 대여할 수 없습니다.");
}
Copy link
Member

Choose a reason for hiding this comment

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

이런건 좀 생각해볼 수 있는게 많은데,
연관관계를 맺은 객체를 통해 대상을 조회할 때, 어떤 방식으로 조회가 되나 한 번 알아보세요~
어떤 쿼리가 실행이 되는건지. Book과 연관된 엔티티를 어떻게 조회하는지 조건절을 유심히 확인해보세요!


@Slf4j
@Service
public class RentalService {
Copy link
Member

Choose a reason for hiding this comment

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

클래스 수준에 Transactional 어노테이션을 붙이면, 클래스 내 모든 메서드에 붙인것과 같은 효과가 있어요!

@@ -0,0 +1,48 @@
# 공통 설정
spring:
Copy link
Member

Choose a reason for hiding this comment

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

yml과 properties 파일이 공존하네요.
하나의 포맷만 남기고 다른 하나는 삭제해주세요!

Comment on lines 38 to 43
@GetMapping("/search")
public ResponseEntity<List<BookDTO>> getBookTitle(@RequestParam String title){
List<BookDTO> books = bookService.findByTitle(title);
log.info("도서 검색 결과: {}건", books.size());
return ResponseEntity.ok(books);
}
Copy link
Member

Choose a reason for hiding this comment

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

컬렉션을 조회해오는 API는 일반적으로 페이징을 적용해요.
페이지네이션에는 어떤 방법들이 있는지 찾아보시고 적용해보세요!

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