Skip to content

Conversation

@sstto
Copy link

@sstto sstto commented Oct 1, 2023

1주차 과제와 캐시 구현 추가 과제 제출합니다.

다음은 특이사항입니다. 리뷰시 참고하시면 좋을 것 같습니다.

  1. UserException의 경우는 PlaylistController에도 해당 Exception이 발생하여 Global ExceptionHandler로 처리하였습니다.
  2. PlaylistIntegrationTest에서 각 테스트마다 회원 가입 메소드가 실행되도록 @BeforeEach를 사용하였고 해당 테스트 클래스에 @Transactional 어노테이션을 붙여 중복 회원가입을 막았습니다.
  3. 엔티티와 DTO 사이 매핑은 생성자와 변환 함수(toSong 등)을 혼용하여 사용하였습니다. 어떤 것이 더 적합한지 궁금합니다.
  4. 캐시의 경우 common 패키지에 SimpleCache를 이용합니다. SimpleCache는 메모리 저장을 위해 HashMap을 사용하였습니다. 캐시가 미스된 경우 block이 실행되도록 구현하여 가독성을 증진시켜 보았습니다.

좋은 과제 제공해주셔서 감사합니다!

Copy link
Member

@PFCJeong PFCJeong left a comment

Choose a reason for hiding this comment

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

굿굿~ 딱히 코멘트 드릴게 없어서 짜치는 리뷰만 달았습니다. 수고하셨습니다~

(1),(2),(4) 👍

(3) 흠 요거는 뭐가 더 적합할까요? 저도 잘 모르겠지만 제 선호를 말씀드리면,,

맵핑에서 확장함수는 잘 안쓰려는 편이에요. DTO 맵핑을 위해 여러 엔티티가 필요한 경우 애매해지는 느낌을 받습니다.

근데 그렇다고 생성자 쓰진 않는데요. 대신 생성자처럼 보이는 함수를 쓰는 편입니다. 생성자가 클래스 안에 명시되면 뭔가 지저분해지는 느낌..?

흠 근데 이펙티브 자바에서도 말하듯이 생성자도 좋은 방법은 사실 아니긴 하네요.

import org.springframework.data.jpa.repository.Query

interface PlaylistGroupRepository : JpaRepository<PlaylistGroupEntity, Long> {
@Query("SELECT pg FROM playlist_groups pg LEFT JOIN FETCH pg.playlists p WHERE pg.open = TRUE AND SIZE(p) > 0")
Copy link
Member

Choose a reason for hiding this comment

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

👍


override fun create(playlistId: Long, userId: Long) {
TODO()
val playlist = playlistRepository.findByIdWithJoinFetch(playlistId) ?: throw PlaylistNotFoundException()
Copy link
Member

Choose a reason for hiding this comment

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

짜치는 리뷰긴 한데 조인 없이 findById로 해도 좋을 것 같습니다.

playlistLikesRepository.save(PlaylistLikesEntity(playlist = playlist, user = user))
}

@Transactional
Copy link
Member

Choose a reason for hiding this comment

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

@Transactional의 범위는 짧게 가져가는 것이 어떨까요?

deleteByPaylistIdAndUserId 함수에 걸어도 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

deleteByPlaylistIdAndUserId가 naming 규칙으로 생성한 자동생성 함수여서 Service 함수에서 걸었는데 자동생성 함수에 걸 수 있는지 확인해봐야겠네요.

}

@ControllerAdvice
class GlobalExceptionHandler {
Copy link
Member

Choose a reason for hiding this comment

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

👍

*/
private inner class Item(val value: V) {

private val createdAt = Instant.now().epochSecond
Copy link
Member

Choose a reason for hiding this comment

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

Instant 객체 생성도 비용이긴 하기 때문에 Long 타입의 millis로 했어도 좋았을 것 같습니다.

class SimpleCache<K, V> {

companion object {
private const val TTL = 10
Copy link
Member

Choose a reason for hiding this comment

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

minute? second? millis? 단위가 명시되면 좋을 것 같습니다. 앗 그런데 주석에 있긴하네요.

Copy link
Author

Choose a reason for hiding this comment

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

주석으로 명시하는게 더 명확할 것 같네요!

*
* 캐시 Miss시 [block] 실행 값을 캐싱하고 리턴한다.
*/
fun get(key: K, block: () -> V): V {
Copy link
Member

Choose a reason for hiding this comment

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

👍

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.

2 participants