Skip to content

Conversation

@soobin-jeon
Copy link

No description provided.

Copy link
Member

@davin111 davin111 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다~

@GetMapping("/api/v1/playlist-groups")
fun getPlaylistGroup(): PlaylistGroupsResponse {
TODO()
//TODO()
Copy link
Member

Choose a reason for hiding this comment

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

작업하셨으면 TODO 는 지우셔도 괜찮습니다

Comment on lines +12 to +18
@ManyToOne // default FetchType.EAGER
@JoinColumn(name = "playlist_id")
val playlist: PlaylistEntity,

@ManyToOne // default FetchType.EAGER
@JoinColumn(name = "song_id")
val song: SongEntity,
Copy link
Member

Choose a reason for hiding this comment

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

어떠한 경우의 연관관계에도 Lazy 를 지향하는 것이 좋습니다. ManyToOne 의 default 가 EAGER 라 하더라도 이것을 변경해두는 것이 좋아요. wafflestudio/seminar-2023#169 를 참고하시면 좋습니다.

이걸 Lazy 로 해두었다면, 불필요한 N+1 쿼리가 덜 날아가서 곳곳에서 JOIN FETCH 를 덜 써도 되었을 수도 있어요.

@ManyToOne // default FetchType.EAGER
@JoinColumn(name = "song_id")
val song: SongEntity,
) No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

git 에서 파일의 마지막 newline 을 넣는 것이 좋은 습관입니다.

Comment on lines +7 to +8
@Query("SELECT p FROM playlists p JOIN FETCH p.playlistGroup pg JOIN FETCH p.playlistSong ps JOIN FETCH ps.song s JOIN FETCH s.album a JOIN FETCH a.artist WHERE p.id = :id")
fun findPlaylistEntityById(id: Long): PlaylistEntity?
Copy link
Member

Choose a reason for hiding this comment

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

예를 들어 service get() 에서는 쿼리를 playlistSongRepository.findSongsInPlaylist() 과 함께 나눠서 실행하는데, 여기서 불필요하게 너무 많이 join 해서 가져오는 듯한 느낌이 있네요. 이거 혹시 연관관계 FetchType 을 EAGER 로 해둬서 발생하는 N+1 때문이었다면, 다른 코멘트와 조합해서 생각해보시면 좋을 듯해요.

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

interface PlaylistLikeRepository : JpaRepository<PlaylistLikeEntity, Long> {
//@Query("")
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 +42 to +51
//TODO()
var i = 400
if (e is SignUpUsernameConflictException) {
i = 409
} else if (e is SignInUserNotFoundException || e is SignInInvalidPasswordException) {
i = 404
} else if (e is AuthenticateException) {
i = 401
}
return ResponseEntity.status(i).build()
Copy link
Member

Choose a reason for hiding this comment

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

가변인 var 사용을 가급적 지양하면 좋습니다. when 을 사용하여 더욱 깔끔한 코드를 만들 수 있을 거 같습니다.

Comment on lines +10 to +11
private val songRepository: SongRepository,
private val albumRepository: AlbumRepository
Copy link
Member

Choose a reason for hiding this comment

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

ktlint 를 참고하여 일관된 indent 를 하면 좋겠네요

return playlistGroupList.map { PlaylistGroup(
id = it.id,
title = it.title,
playlists = it.playlist.map { PlaylistBrief(it.id, it.title, it.subtitle, it.image) }
Copy link
Member

Choose a reason for hiding this comment

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

IDE 도 알려줬을 거 같은데, map { } 바깥과 내부에서 it 이 가리키는 것이 달라서 혼동을 줄 수 있습니다. 이런 경우 map { playlist -> ... }처럼 한 쪽의 이름을 명시적으로 주는 게 좋습니다. IDE 의 경고는 무시하지 않는 게 좋아요.

TODO()
//TODO()
val playlistLike = playlistLikeRepository.findPlaylistLikeEntityByPlaylistId(playlistId).filter { it.user.id == userId }
return !playlistLike.isEmpty()
Copy link
Member

Choose a reason for hiding this comment

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

isNotEmpty()를 그냥 쓰면 더 좋겠네요.

TODO()
//TODO()
val playlist = playlistRepository.findPlaylistEntityById(playlistId) ?: throw PlaylistNotFoundException()
val playlistLike = playlistLikeRepository.findPlaylistLikeEntityByPlaylistId(playlistId).filter { it.user.id == userId }
Copy link
Member

Choose a reason for hiding this comment

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

이거 그냥 쿼리로 userId 같은 걸 찾도록, findByPlaylistIdAndUserId()를 만들면 될 거 같은데요? 만약에 이 playlist 를 like 한 유저가 너무 많다면, 서버 내부로 다 가져와서 filtering 하는 것에 성능 이슈가 있을 수 있습니다.

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