[Master] 7-8주차 PR #84
Conversation
- QueryDsl을 적용하기위해 customRepo를 작성하였습니다. 관련 이슈: #7
- placeService에서 video가 조회되지 않는 오류를 예외처리하였습니다. 관련 이슈: #7
…tep3/Team7_BE into refactor/#49-VideoIntegrationRefactor
[Feat]#8 weekly에서 배포되도록 변경했어요
- 장소세부정보 조회에서 video 정보가 있으면 가져오고 없으면 빈문자열()만 들어가도록 수정하였습니다. 관련 이슈: #7
- VideoRepository: findByPlaceId시, Video 여러개 조회되는 문제로 List로 return하도록함 - PlaceService: video 여러개 조회될시에 제일 첫번째 video만 반영 - 관련하여 테스트 코드 추가함 관련 이슈: #7
관련 이슈 : #44
불필요한 userQuery한개를 줄였습니다.
- 기존에 Respository에 바로 query문을 쓰던것을 오류해결을 원활히 하기위해 QueryDsl을 적용했습니다. - PlaceRepositOry 메서드명을 get에서 find로 변경하여 일원화했습니다. 관련이슈: #7
[fix] #7 장소 세부정보 반환 오류 패치
인플루언서 -> true, 장소 -> false
refreshToken swagger를 적용하였습니다. 관련 이슈: #44
- pirce를 String 타입으로 변경했습니다. 관련 이슈: #7
인플루언서 -> true, 장소 -> false
[refacotr] #7 QueryDsl을 적용했어요
…VideoIntegrationRefactor [Refactor] #49 통합 과정에서 발생하는 사항을 리펙터링 했어요
…nRefresh [fix] oauth2login 실패 시, 무한 리다이렉트 해결
…nRefresh [Fix] #44 cors Error를 해결하였습니다.
…nRefresh [fix] #44 cors error 수정 2
…rize-youtube [feat] Place에 menuboardImgUrls 추가 했어요!
- MenuBoardPhotoUrls를 Response에 담아서 반환하도록했습니다. 관련 이슈: #7
[refactor] #7 MenuBoardPhotoUrls를 DTO로 반환하도록 했어요
더이상 spring security login page를 사용할 수 없습니다. 그동안의 노고에 감사드립니다. 관련 이슈: #50
…rize-youtube [feat] Url 반환 시 youtube url 반환하도록 변경
…eMessage [Feat] #50 login page 없애버리기
…uencer [Feat] #20 인플루언서 좋아요와 반영하여 반환하도록 했어요
…rize-youtube [fix] 값이 없을 경우 default 저장값을 변경했어요
…oAdminPage [Feat] #80 관리자 페이지 프로토 타입을 만들었어요
[Weekly] 7-8주차 PR
[Master] 7-8주차 PR
There was a problem hiding this comment.
안녕하세요~ 7-8 주차 PR 잘 작성해주셨네요.
점점 서비스가 고도화되어 가는 것이 보이는군요~ 👍
몇 가지 코멘트 달았으니, 한 번 팀원분들과 논의해보세요!
질문을 살펴보자면,,
- 지난 주에 드렸던 리뷰에서 1대1 대응인 포인트를 못잡았군요ㅠ 특별히 수정하지 않으셔도 될 것 같아요. 근데 이 코드에서 이번에 코멘트 남긴 내용은 한 번 고민해보세요!
- 이렇게 null이 포함된 정보를 부득이하게 전달해야 할 때는,, 어쩔수 없이 꼼꼼하게 null 처리를 꼼꼼하게 할 수밖에는 없겠네요ㅜ 다만 코드 로직을 꼭 1:1로 풀어야할 지도 고민의 대상이 될 것 같아요. null이라면 list에 포함시키지 않고는 문제를 해결할 수 없을까? 도 한 번 고민해보세요. Place와 Video를 하나로 묶은 새로운 클래스를 생성해서 내부에서 id를 묶어서 관리한다던가,, Optional을 활용해서 문제를 해결해볼 수 없을지 등 (물론 Optional의 주의사항도 고려하면서,,)
- 자세한 비즈니스 요구사항은 본인이 제일 잘 아실테니, 요구사항에 맞게 구현하시면 될 것 같아요. (근데 이제 가독성과 유지보수성을 곁들인..ㅎㅎ)
전체적으로 잘 수행해주시고 계시네요~ 벌써 배포도 하시고,, 도메인도 구입하시고,, 👍👍 고생하셨습니다ㅎㅎ
시험 기간 화이팅하시고, 카테캠도 화이팅입니다 🔥🔥
관련해서 또 질문 있으시면 언제든 말씀해주세요~
| import org.springframework.web.bind.annotation.GetMapping; | ||
|
|
||
| @Controller | ||
| public class adminPageController { |
There was a problem hiding this comment.
어드민 페이지도 개발 예정이시군요~ 👍
현업에서도 어드민 페이지를 별도로 개발해서 사용하고 있으니, 재미있게 개발해보시면 좋을 것 같아요!
| import org.springframework.data.jpa.repository.JpaRepository; | ||
| import team7.inplace.crawling.domain.YoutubeChannel; | ||
|
|
||
| public interface YoutubeChannelRepository extends JpaRepository<YoutubeChannel, Long> { | ||
| Optional<YoutubeChannel> findYoutubeChannelByInfluencerId(Long influencerId); |
There was a problem hiding this comment.
youTubeChannel에 influencerId가 unique하다는 제약조건이 필요해보여요~
| Long userId = AuthorizationUtil.getUserId(); | ||
|
|
||
| // 로그인 안된 경우, likes를 모두 false로 설정 |
There was a problem hiding this comment.
로그인 되었다는 여부를 userId가 존재하는지로 판단하는 것도 좋지만,
AuthorizationUtil.isLoginedUser() 처럼 그 의미를 명확하게 드러내는 메서드를 작성하면 어떨까요?
| List<InfluencerInfo> influencerInfos = influencers.stream() | ||
| .map(influencer -> { | ||
| boolean isLiked = likedInfluencerIds.contains(influencer.getId()); | ||
| return InfluencerInfo.from(influencer, isLiked); | ||
| }) | ||
| .sorted((a, b) -> Boolean.compare(b.likes(), a.likes())) | ||
| .toList(); |
There was a problem hiding this comment.
List<Influencer> influencers = influencerRepository.findAll(); 로 불러온 influencer의 size를 N이라고 하고,,
user가 좋아요 누른 influencer를 M이라고 하면,, 시간복잡도가 O(N*M) 이 되겠군요ㅎㅎ
지금 당장은 문제가 없지만,, influencer가 1만명, user가 1만명을 좋아요 누르면,, 꽤나 느린 메서드가 되겠네요~
물론 influencer가 1만명이 될까? user가 1만명을 좋아요 누를 일이 있을까? 라는 생각도 있지만, 이런 코드에서 풀 수 있는 성능적인 문제를 해결해보는 것도 재미있는 경험이 될거에요~
저 같은 경우도 현업에서 그런 고민을 많이 하는데요, 아무리 시간복잡도가 O(N^2) 가 되더라도 N이 작아서 성능에 문제가 없다고 판단되면, 가독성을 더 챙기는 경우도 있습니다.
만약 성능을 포기한다면, 가독성을 조금 더 높일 수 있는 방법을 생각해보시는 것도 좋을 것 같아요.
| String username = AuthorizationUtil.getUsername(); | ||
| if (StringUtils.hasText(username)) { | ||
| if (!StringUtils.hasText(username)) { |
There was a problem hiding this comment.
지금 import org.springframework.util.StringUtils; 의 StringUtils를 사용하고 계신데요~
org.apache.commons.lang3.StringUtils.isEmpty() 를 사용하시면 ! 의 부정을 없애서 가독성 측면에서 더 좋아질 것 같아요~
참고로, 어라 apache.commons 라이브러리를 import하지 않았는데 왜 사용하지? 라는 의문이 드신다면, 어디서 의존성이 추가되었는지 확인할 필요가 있습니다.
./gradlew dependencies 명령어를 통해, 한 번 확인해보세요.
만약 연쇄적으로 의존성이 주입되었고, 이후에 root 의존성이 필요없는 의존성이 될 가능성이 있다면, 따로 StringUtils 클래스를 작성할 수도 있고, 가독성을 포기하더라도 SpringFramework 내부의 StringUtils를 활용해도 좋겠네요~
| favorite.updateLike(param.likes()); | ||
| favoriteRepository.save(favorite); |
There was a problem hiding this comment.
메서드가 트랜잭션으로 감싸져 있기 때문에, 내부적으로 변경 감지(더티 체킹) 가 일어날 것 같아요.
| Video video = null; | ||
| List<Video> videos = videoRepository.findByPlaceId(placeId); | ||
|
|
||
| if (!videos.isEmpty()) { | ||
| video = videos.get(0); | ||
| } | ||
| Influencer influencer = (video != null) ? video.getInfluencer() : null; | ||
|
|
||
| return PlaceDetailInfo.from(place, influencer, video); |
There was a problem hiding this comment.
| Video video = null; | |
| List<Video> videos = videoRepository.findByPlaceId(placeId); | |
| if (!videos.isEmpty()) { | |
| video = videos.get(0); | |
| } | |
| Influencer influencer = (video != null) ? video.getInfluencer() : null; | |
| return PlaceDetailInfo.from(place, influencer, video); | |
| Video video = videoRepository.findByPlaceId(placeId).stream().findFirst().orElse(null); | |
| Influencer influencer = Optional.ofNullable(video).map(Video::getInfluencer).orElse(null); | |
| return PlaceDetailInfo.from(place, influencer, video); |
이렇게 구현할 수도 있어요! 참고 해보시고, 어떤게 더 가독성 좋을지는 판단해서 사용하시면 됩니다 😆
|
|
||
| @Repository | ||
| @AllArgsConstructor | ||
| public class PlaceCustomRepositoryImpl implements PlaceCustomRepository { |
There was a problem hiding this comment.
인터페이스를 구현하는 클래스에서 ~Impl suffix를 붙이는 것은 흔한 패턴인데요~
요런 시각에서 바라볼 수도 있어요.
https://www.baeldung.com/java-interface-naming-conventions#examples-of-incorrect-interface-naming
과연 ~Impl suffix를 사용할지, 아니면 조금 더 직관적인 네이밍을 사용할지 (예를 들면, queryDsl을 사용하는 Repository라는 것을 나타내는..등) 는 팀원분들과 논의해보시고 고민해보세요 😆
| if(Objects.isNull(place)){ | ||
| return new VideoInfo( | ||
| savedVideo.getId(), | ||
| "장소 정보 없음", | ||
| savedVideo.getVideoUrl(), | ||
| PlaceForVideo.of(-1L, "장소 정보 없음") | ||
| ); | ||
| } |
There was a problem hiding this comment.
| if(Objects.isNull(place)){ | |
| return new VideoInfo( | |
| savedVideo.getId(), | |
| "장소 정보 없음", | |
| savedVideo.getVideoUrl(), | |
| PlaceForVideo.of(-1L, "장소 정보 없음") | |
| ); | |
| } | |
| private VideoInfo videoToInfo(Video savedVideo) { | |
| Place place = savedVideo.getPlace(); | |
| String alias = AliasUtil.makeAlias(savedVideo.getInfluencer().getName(), place.getCategory()); | |
| return new VideoInfo(savedVideo.getId(), alias, savedVideo.getVideoUrl(), PlaceForVideo.of(place)); | |
| } |
메서드는 딱 여기까지만 작성하고, 나머지 null에 대한 분기처리는 .makeAlias() 메서드 내부나 PlaceForVideo dto 내부에서 처리하면 캡슐화가 잘 될 것 같아요!
한 번 고민해보세요~ (비즈니스 로직을 정확하게 아는건 아니라서 정답이 아닐 수 있습니다)
| .toList(); | ||
| .map(place -> { | ||
| if (Objects.isNull(place)) { | ||
| return -1L; |
There was a problem hiding this comment.
없다는 것을 나타내기 위해 id를 -1L 로 사용하시고 있는 것 같아요.
이렇게 하는 것도 방법이 될 수 있지만, id가 Long 타입이니 null로 처리하는 것이 일반적이지 않을까요?
서비스 정책이 -1로 하게 되어 있다면, -1을 체크하는 부분을 따로 빼주어야 할 것 같아요.
예를 들면,, 지금 PlaceService에서 -1로 설정해주고 있는데, VideoService에서 -1을 체크하는 부분이 있네요.
private boolean hasNoPlace(Long placeId) {
return placeId == -1;
}
만약 -1을 사용하는 곳이 더 늘게 된다면, 그만큼 체크하는 메서드도 늘어날 것이고,, 이러한 정보를 한 곳에서 관리하면 어떨까요?
PlaceIdUtils 와 같은 클래스를 생성해도 괜찮겠네요. 클래스 내부에서 DEFAULT_ID 를 -1L 처럼 들고 있고, 내부에서 hasNoPlace() 와 같은 메서드에서 체킹할 수 있을 것 같아요.
한 번 고민해보세요 😃
✨ 작업 내용
✨ 질문 사항
[Master] 6주차 과제 Review PR #58 (comment)
지난주에 남겨주신 이 리뷰에 대해 질문드리고 싶습니다.
로직이 저렇게 된 이유는, 유튜브 영상 중 주소를 찾을 수 있는 영상은 coomand가 null이도록 해 VideoCommand와 PlaceCommand가 1:1
대응이 되도록 했기 떄문입니다.
즉 place 정보가 존재하는 비디오라면 해당 인덱스에 place를 저장한 후 id를 반환하도록 했고, place 정보가 없는 비디오라면 id가 -1을 반환
하도록 해 비디오와 장소가 1:1 대응이 되도록 하려 했기 때문에 저런 로직이 탄생(?) 했습니다.
이런 경우에도 수정이 필요할까요?
추가로 이렇게 null이 포함된 정보들을 전달하는게 NPE발생 가능성을 높인다고 생각하긴 하지만 다른 방법이 생각나지 않습니다.
null을 부득이하게 전달해야할 때 어떻게 하면 좋을지 조언 부탁드립니다.