[Master] 6주차 과제 Review PR #58
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- /places request시, 좌상단 우하단 좌표 추가해서 해당 바운더리 안에있는 장소만 반환하도록함 - 관련된 test코드 수정하여 적용함 관련이슈 : #7
- placeService: getPlacesByDistanceAndFilters에 넣는 파라미터에서 longitude <-> latitude 순서 변경 - placeController: modelAttribute로 지정한 searchParam에 파라미터가 매핑 안되는 오류로 Requestparam으로 받도록 수정 특이사항: controllerTest가 제대로 작동하지 않음. 직접 sql문 입력시에는 제대로 동작함. 추후 테스트코드 보완 예정 관련 이슈: #7
- PlaceInfo: menuImgUrl을 추가했습니다. 관련 이슈: #7
- /places/{id}로 장소 세부정보 조회하는 기능을 구현하였습니다.
- Menu VO에 menuImgUrl 필드를 추가하고, 관련된 코드를 수정했습니다.
- PlaceDetailInfo 레코드를 구현하여 PlaceService에서 정보를 컨트롤러에 반환할수있도록 했습니다.
- PlaceDetailResponse 레코드를 구현해서 최종적으로 반환형식을 정했습니다.
관련 이슈: #7
관련 이슈: #7
- 역직렬화가 계속 오류나서 json형식의 facilities를 일반 boolean으로 변경하였습니다. 관련 이슈: #7
User마다 Role을 부여하여 admin authorization을 구현함. 관련 이슈: #2
- Facility를 String으로 반환하도록 했습니다. 관련 이슈: #7
-관련 이슈: #7
인플루언서 비디오 조회 로직을 변경하여 해당 부분 삭제
- Place: facility에서 columndefinition=json 제거해서 역직렬화 오류 고침 - PlaceDetailInfo-MenuInfos: menuImgUrls를 따로 리스트 만들어 저장함 관련 이슈: #7
- PlaceController에 Swagger를 적용했어요 관련 이슈: #7
PageDefault 어노테이션 사용으로 변경
Error 처리 방식을 변경
- PlaceDetailInfo, Response of메서드명을 from메서드명으로 변경하였습니다. 관련 이슈: #7
Authorization Cookie가 없을 경우 무조건 exception을 발생시키는 경우를 제거하였습니다. 이제, Authorization Cookie가 있으면 검증해 user를 SecurityContext에 등록합니다. -Authorization Cookie 가 없으면 : Anonymous User -Authorization Cookie 가 있으면, 토큰 유효 : User -Authorization Cookie 가 있고, 토큰 이상 : Token Invalid 관련 이슈: #2
anonymousUser면 null을 리턴하도록 설정 관련 이슈: #2
…tep3/Team7_BE into feat/#2-authorization
- menuList 제거함, menu VO 변경에 맞춰, menuImgUrl, description 필드 추가 관련 이슈: #7
…rize-youtube [Fix] 크롤링 시 발생할 수 있는 문제를 수정합니다.
getmethod 빼먹은 부분 을 추가 하였습니다. 관련 이슈 : #44
…nRefresh [Feat] #44 TokenRefresh하기~
[refactor] #7 MenuInfos-menuList 제거
…VideoIntegrationRefactor [Refactor] #49 Integration 과정에서 발생하는 문제들을 해결해보아요
- Menu.price가 Long -> String으로 바뀜에 따라 parseInt로 처리 메서드 변경했습니다. 관련 이슈: #7
[fix] #7 MenuInfos.of(), menu.getPrice() 처리방식 변경
redis.yaml 파일로 config 관리 관련 이슈 : #44
…nRefresh [Chore] redis.yaml 파일로 config 관리
[Weekly] 6주차 과제 PR
[Develop] 6주차 PR
nimunsang
approved these changes
Oct 12, 2024
There was a problem hiding this comment.
안녕하세요~ 3번째 PR도 잘 제출해주셨네요 👍 고생 많으셨습니다.
몇 가지 코멘트 달아드렸으니, 한 번 확인해보시고 팀원분들과 논의해보세요~
지난주에 리뷰드렸던 내용에서도 반영된 것이 있고, 반영되지 않은 것도 있는데, 한 번 팀원분들과 논의해보세요! 물론 리뷰드렸다고 해서 반드시 반영해야하는 것은 아니지만,, 한 번쯤 고민해보셨으면 좋을 내용들이라서요!
이번 주에는 크게 리뷰드릴 내용이 없는 대신, 다른 이야기를 해보자면,, 저희 팀의 10년차 이상 시니어분들이 공통적으로 하시는 말이 있습니다.
- 무조건 읽기 편한 코드가 최고다. (10초였는지 20초였는지 기억은 안나지만) 코드를 봤을 때, 그 내용이 이해가 되는 코드가 좋은 코드다.
- 코드를 작성하고나면, 곧바로 그 코드는 레거시 코드가 됩니다. 레거시 코드를 읽었을 때, 쉽게쉽게 읽히는 코드가 짱입니다.
- RedisTemplate를 사용하는 것과 CrudRepository 두개의 장단점을 알고싶습니다.
- CrudRepository 방식은 말 그대로 CRUD에 특화된, DB(여기서는 Redis) 에서의 작업을 도와주는 API를 제공해주는데요. RedisTemplate은 보다 더 저수준의, 복잡한 연산을 수행할 수 있습니다. 예를 들면,, 파이프라이닝, Redis에서의 atomic한 연산 등을 수행해야 할 경우가 있습니다. 다만 crudRepository 만으로는 이를 구현하기는 힘들죠. 하지만 단순 crud만을 사용한다면 crudRepository를 사용하는 것이 좋을 수 있습니다, 편하니까요. 결론적으로는 Redis에서 제공해주는 수많은 기능을 활용하기 위해서는 RedisTemplate를 활용하는 것이 좋고, CrudRepository는 단순히 CRUD기능을 빠르게 구현할 때 사용합니다.
- gitaction과 jenkins 중 어느게 현업에서 많이 사용되나요? 그리고 그걸 사용하는 이유가 궁금합니다!
- 저희는 jenkins를 사용합니다. 특별한 이유는 없구요, github enterprise 버전에서 github actions를 지원하지 않고 있기 때문이에요.... 만약 지원한다면 넘어갈 수도 있을 것 같습니다. 별개로, 사내에서 제작한 CI/CD 를 지원해주는 툴을 사용하기도 합니다.
- production 서버와, develop 서버가 분리되어있을 경우 .env는 어떻게 관리되는지 궁금합니다.
- https://wonyong-jang.github.io/spring/2022/08/11/Spring-Profile.html
- 위 방식과 동일하게 관리합니다. (yaml 파일에서 spring.active.on-profile 속성으로 관리합니다)
이번 주도 정말 고생 많으셨고, 다음 주도 화이팅입니다 🔥 질문이 있다면 언제든 알려주세요~
Comment on lines
+10
to
+28
| @NoArgsConstructor(access = PRIVATE) | ||
| public final class AddressUtil { | ||
| private static final String ADDRESS_REGEX = "[가-힣0-9]+(?:도|시|구|군|읍|면|동|리|로|길)\\s*[0-9]*(?:동|읍|면|리|로|길)?\\s*[0-9]+"; | ||
|
|
||
| public static String extractAddressFromJsonNode(JsonNode snippet) { | ||
|
|
||
| String videoDescription = snippet.path("description").asText(); | ||
|
|
||
| return extractAddressFromString(videoDescription); | ||
| } | ||
|
|
||
| public static String extractAddressFromString(String string) { | ||
| Pattern pattern = Pattern.compile(ADDRESS_REGEX); | ||
| Matcher matcher = pattern.matcher(string); | ||
| if (matcher.find()) { | ||
| return matcher.group(); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
👍👍 Utility class 잘 구현해주셨네요
다만, 사용하는 쪽에서 null체크를 하고 utility class를 사용하는 것 보다, snippet이나 string에 대한 null check가 선행되면 더 좋을 것 같아요~
Comment on lines
+43
to
+48
| public ResponseEntity<Long> createInfluencer(@RequestBody InfluencerRequest request) { | ||
| InfluencerCommand influencerCommand = new InfluencerCommand( | ||
| request.influencerName(), | ||
| request.influencerImgUrl(), | ||
| request.influencerJob() | ||
| ); |
There was a problem hiding this comment.
influencerRequest 내부에서 InfluencerCommand로 변환하는 메서드가 있어도 좋겠네요~
Comment on lines
+107
to
+131
| public List<Long> createPlaces(List<Create> placeCommands) { | ||
| var places = placeCommands.stream() | ||
| .map(command -> { | ||
| if (Objects.isNull(command)) { | ||
| return null; | ||
| } | ||
| return command.toEntity(); | ||
| }) | ||
| .toList(); | ||
| var nonNullPlaces = places.stream() | ||
| .filter(Objects::nonNull) | ||
| .toList(); | ||
| placeRepository.saveAll(nonNullPlaces); | ||
|
|
||
| var savedPlacesId = places.stream() | ||
| .map(place -> { | ||
| if (Objects.isNull(place)) { | ||
| return -1L; | ||
| } | ||
| return place.getId(); | ||
| }) | ||
| .toList(); | ||
|
|
||
| return savedPlacesId; | ||
| } |
There was a problem hiding this comment.
Suggested change
| public List<Long> createPlaces(List<Create> placeCommands) { | |
| var places = placeCommands.stream() | |
| .map(command -> { | |
| if (Objects.isNull(command)) { | |
| return null; | |
| } | |
| return command.toEntity(); | |
| }) | |
| .toList(); | |
| var nonNullPlaces = places.stream() | |
| .filter(Objects::nonNull) | |
| .toList(); | |
| placeRepository.saveAll(nonNullPlaces); | |
| var savedPlacesId = places.stream() | |
| .map(place -> { | |
| if (Objects.isNull(place)) { | |
| return -1L; | |
| } | |
| return place.getId(); | |
| }) | |
| .toList(); | |
| return savedPlacesId; | |
| } | |
| public List<Long> createPlaces(List<Create> placeCommands) { | |
| var places = placeCommands.stream() | |
| .filter(Objects::nonNull) | |
| .map(Create::toEntity) | |
| .toList(); | |
| List<Place> savedPlaces = placeRepository.saveAll(places); | |
| return savedPlaces.stream() | |
| .map(Place::getId) | |
| .toList(); | |
| } |
조금 더 간단하게 이렇게 작성하면 어떨까요?
| facilityTree(place.getFacility()), | ||
| MenuInfos.of(place.getMenus()), | ||
| OpenHour.of(place.getOpenPeriods(), place.getOffDays()), | ||
| PlaceLikes.of(null), //추후 추가 예정 |
There was a problem hiding this comment.
이렇게 // 추후 추가 예정 과 같은 주석은, // TODO 를 활용하시면 나중에 intellij에서 모아보실 수 있습니다!
Comment on lines
+26
to
+31
| private Address(String address1, String address2, String address3) { | ||
| this.address1 = address1; | ||
| this.address2 = address2; | ||
| this.address3 = address3; | ||
| } | ||
|
|
There was a problem hiding this comment.
@AllArgsConstructor 를 지우는 대신, @AllArgsConstructor(access = PRIVATE) 처럼 작성해도 되겠네요~ (가독성 측면에서 작성하신 방법이 더 편하다면 편하신 방법으로 하면 됩니다)
Comment on lines
+35
to
+42
| @RequestParam String longitude, | ||
| @RequestParam String latitude, | ||
| @RequestParam String topLeftLongitude, | ||
| @RequestParam String topLeftLatitude, | ||
| @RequestParam String bottomRightLongitude, | ||
| @RequestParam String bottomRightLatitude, | ||
| @RequestParam(required = false) String categories, | ||
| @RequestParam(required = false) String influencers, |
There was a problem hiding this comment.
modelAttribute 를 사용하셨다가 각각 받기로한 이유가 궁금해요~
제 생각으로는 이렇게 파라미터가 많을 때는 하나의 class로 묶는게 좋을 것 같아서요~
Comment on lines
+25
to
35
| public SecurityConfig( | ||
| CustomOAuth2UserService customOAuth2UserService, | ||
| CustomSuccessHandler customSuccessHandler, | ||
| ExceptionHandlingFilter exceptionHandlingFilter, | ||
| AuthorizationFilter authorizationFilter | ||
| ) { | ||
| this.customOauth2UserService = customOAuth2UserService; | ||
| this.customSuccessHandler = customSuccessHandler; | ||
| this.authorizationFilter = authorizationFilter; | ||
| this.exceptionHandlingFilter = exceptionHandlingFilter; | ||
| this.authorizationFilter = authorizationFilter; | ||
| } |
There was a problem hiding this comment.
어차피 모두 final로 선언되어 있고, 생성자에서 추가적인 행위를 하지 않는다면 @RequiredArgsConstructor 를 활용하시면 어떨까요?
| .addFilterBefore(exceptionHandlingFilter, AuthorizationFilter.class) | ||
| //authentication 경로 설정 | ||
| .authorizeHttpRequests((auth) -> auth | ||
| .requestMatchers("/login").permitAll() | ||
| .requestMatchers("/hello").authenticated() | ||
| .requestMatchers("/admin/**").hasRole("ADMIN") |
Comment on lines
+16
to
+19
| @Bean | ||
| public CookieUtil cookieUtil() { | ||
| return new CookieUtil(); | ||
| } |
There was a problem hiding this comment.
CookieUtil을 bean으로 등록해주신 이유가 궁금해요~
CookieUtil 클래스를 확인해보니 단순히 k, v 로 구성된 cookie를 생성하는 메서드밖에 없는 것 같아서요~ util 클래스를 인스턴스화 하지 않고, static method를 활용할 수도 있지 않을까요?
| private boolean hasNoTokenCookie(HttpServletRequest request) { | ||
| return Optional.ofNullable(request.getCookies()) | ||
| .flatMap(cookies -> Arrays.stream(cookies) | ||
| .filter(cookie -> cookie.getName().equals("access_token")) |
There was a problem hiding this comment.
"access_token" , "refresh_token" 이라는 문자열이 다른 곳에서도 쓰이고 있는데요~
이런 문자열을 한 곳에서 관리하면 어떨까요?
적절한 enum 이나 별도의 클래스에서 관리하면 좋을 것 같아요~
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✨ 작업 내용
✨ 질문 사항