-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor: 전반적인 서비스 코드 리팩터링 진행 #48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fix: CD 배포 중 certbot 인증서 발급 한도 초과 오류 해결
Release: 모니터링 도구 적용 및 접근성에 대한 보안을 추가한 2차 배포
- 사용자 편의성을 위해서 회원 기능을 사용하지 않기로 결정했습니다. - 따라서, 회원 기능과 관련된 파일 및 디렉터리를 삭제합니다. issue #32
- RecommendationPlace Entity를 place 디렉터리 생성 후, 그 하위로 이동시켰습니다. - 역할을 정확하게 분리하기 위함입니다. issue #32
- 거대힌 RetripService를 구성하는 기능 중 일부를 Util 클래스로 추출했습니다. - CoordinateUtil: 위도, 경도 값과 관련된 Util 클래스 - DateUtil: 날짜 관련된 Util 클래스 - DistanceUtil: 거리 관련된 Util 클래스 - ImageMetaDataUtil: 업로드한 이미지의 메타 데이터와 괸련된 Util 클래스 issue #32
- RetripService 파일에 내장되어있던 클래스를 별도의 클래스로 분리했습니다. - 새롭게 생성한 info 디렉터리 하위에 위치시켰습니다. issue #32
- 작성은 되어있지만, 사용하지 않는 코드들을 제거했습니다. - 충분히 흐름을 통해서 알 수 있는 주석들을 제거했습니다. issue #32
- Util 클래스로 묶을 수 있는 기능들을 별도의 Util 클래스로 추출했습니다. - Date, Coordinate, Distance, ImageMetaData - 흐름을 통해서 알 수 있는 주석들을 제거했습니다. - 내장 클래스를 외부로 분리했습니다. - 이제는 사용되지 않는 회원 관련 코드들을 제거했습니다. issue #32
- 사용하지 않는 회원 엔티티와 연관 관계 코드를 제거했습니다. - 딱 봐도 알 수 있는 주석을 제거했습니다. - Setter 어노테이션을 제거하고, 데이터 수정 로직을 추가했습니다. issue #32
- 코드로도 이해할 수 있는 주석을 제거합니다. - 매직 스트링을 별도의 상수로 추출하여 이해하기 쉽도록 이름을 부여합니다. issue #32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR performs a broad refactoring by extracting common logic into utility classes, removing outdated or unused code (especially around OAuth and member handling), and breaking down large service methods into smaller, focused steps.
- Extracted image metadata, distance, date, and coordinate logic into dedicated util classes
- Refactored
RetripServiceto separate validation, image preparation, metadata extraction, and response building - Cleaned up controllers and filters (removed obsolete OAuth handlers, simplified session filter, updated exception handling in controller)
Reviewed Changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ssafy/retrip/utils/DateUtil.java | Added utility methods for earliest/latest takenDate calculations |
| src/main/java/ssafy/retrip/utils/ImageMetaDataUtil.java | Extracted EXIF metadata parsing into a standalone utility |
| src/main/java/ssafy/retrip/utils/DistanceUtil.java | Added Haversine distance calculation and total distance utility |
| src/main/java/ssafy/retrip/utils/CoordinateUtil.java | Introduced clustering and main location analysis utilities |
| src/main/java/ssafy/retrip/api/controller/retrip/RetripController.java | Updated service call signature and replaced printStackTrace with proper logging |
Comments suppressed due to low confidence (1)
src/main/java/ssafy/retrip/utils/DistanceUtil.java:14
- Utility methods like calculateDistance currently lack unit tests. Adding tests for known coordinate pairs and edge cases will improve confidence in this calculation.
public static double calculateDistance(double lat1, double lon1, double lat2, double lon2) {
| return metadataList.stream() | ||
| .map(ImageMetaData::getTakenDate) | ||
| .filter(Objects::nonNull) | ||
| .reduce((first, second) -> second) |
Copilot
AI
Jul 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using reduce to pick the last element does not guarantee the chronologically latest date. Consider using stream.max(Comparator.naturalOrder()) on the takenDate values.
| .reduce((first, second) -> second) | |
| .max(Comparator.naturalOrder()) |
| return metadataList.stream() | ||
| .map(ImageMetaData::getTakenDate) | ||
| .filter(Objects::nonNull) | ||
| .findFirst() |
Copilot
AI
Jul 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findFirst() on an unsorted stream does not reliably return the earliest date. Use stream.min(Comparator.naturalOrder()) to find the minimum takenDate value.
| .findFirst() | |
| .min(Comparator.naturalOrder()) |
| public static ImageMetaData extractMetadata(InputStream inputStream) { | ||
| ImageMetaData metadata = new ImageMetaData(); | ||
| try { | ||
| Metadata rawMetadata = ImageMetadataReader.readMetadata(inputStream); | ||
| extractDateTimeInfo(rawMetadata, metadata); | ||
| extractGpsInfo(rawMetadata, metadata); | ||
| } catch (Exception e) { | ||
| log.error("메타데이터 추출 오류", e); | ||
| } | ||
| return metadata; |
Copilot
AI
Jul 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Catching a generic Exception and only logging it may hide failures. Consider returning an Optional or rethrowing a custom exception so callers can handle extraction errors explicitly.
| public static ImageMetaData extractMetadata(InputStream inputStream) { | |
| ImageMetaData metadata = new ImageMetaData(); | |
| try { | |
| Metadata rawMetadata = ImageMetadataReader.readMetadata(inputStream); | |
| extractDateTimeInfo(rawMetadata, metadata); | |
| extractGpsInfo(rawMetadata, metadata); | |
| } catch (Exception e) { | |
| log.error("메타데이터 추출 오류", e); | |
| } | |
| return metadata; | |
| public static Optional<ImageMetaData> extractMetadata(InputStream inputStream) { | |
| ImageMetaData metadata = new ImageMetaData(); | |
| try { | |
| Metadata rawMetadata = ImageMetadataReader.readMetadata(inputStream); | |
| extractDateTimeInfo(rawMetadata, metadata); | |
| extractGpsInfo(rawMetadata, metadata); | |
| return Optional.of(metadata); | |
| } catch (Exception e) { | |
| log.error("메타데이터 추출 오류", e); | |
| return Optional.empty(); | |
| } |
| return ResponseEntity.ok(retripService.createRetripFromImages(images, memberId)); | ||
| return ResponseEntity.ok(retripService.createRetripFromImages(images)); | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); |
Copilot
AI
Jul 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Avoid using printStackTrace() in controllers. Use a logger (e.g., log.error) to record exceptions and return a structured error response if appropriate.
| public static Map<String, Object> analyzeMainLocation(List<ImageMetaData> metadataList) { | ||
|
|
||
| List<ImageMetaData> locData = metadataList.stream() | ||
| .filter(m -> m.getLatitude() != null && m.getLongitude() != null) | ||
| .toList(); | ||
|
|
||
| if (locData.isEmpty()) { | ||
| return Map.of("latitude", DEFAULT_LATITUDE, "longitude", DEFAULT_LONGITUDE); |
Copilot
AI
Jul 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Returning a raw Map makes it harder to track keys and types. Consider returning a typed object (e.g., GpsCoordinate) or a small DTO for clarity and type safety.
| public static Map<String, Object> analyzeMainLocation(List<ImageMetaData> metadataList) { | |
| List<ImageMetaData> locData = metadataList.stream() | |
| .filter(m -> m.getLatitude() != null && m.getLongitude() != null) | |
| .toList(); | |
| if (locData.isEmpty()) { | |
| return Map.of("latitude", DEFAULT_LATITUDE, "longitude", DEFAULT_LONGITUDE); | |
| public static GpsCoordinate analyzeMainLocation(List<ImageMetaData> metadataList) { | |
| List<ImageMetaData> locData = metadataList.stream() | |
| .filter(m -> m.getLatitude() != null && m.getLongitude() != null) | |
| .toList(); | |
| if (locData.isEmpty()) { | |
| return new GpsCoordinate(DEFAULT_LATITUDE, DEFAULT_LONGITUDE); |
- 기존에 있던 OAuth2 관련 설정을 제거하여 이와 관련된 config 코드를 제거했습니다. - 회원과 세션 관련 필요없는 코드를 삭제했습니다. issue #32
- 클래스 전역으로 선언되어있던 Setter, Builder 어노테이션을 제거했습니다. - Builder 어노테이션의 경우 필요한 매개변수들만을 포함한 메소드 단위로 분리하였고, private 접근 제한자로 변경하여 외부와 차단시켰습니다. - 기본 생성자의 경우 protected 접근 제한자를 설정하여 쉽게 접근을 못하도록 했습니다. - Builder.Default 어노테이션을 final 키워드로 대신하였습니다. issue #32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이미지 메타데이터를 추상화한 접근법 좋네요! ㅎㅎ
#️⃣ 연관된 이슈
#32
📝 작업 내용
💬 리뷰 요구사항
리팩터링이 70% 이루어진 것 같습니다. Util 클래스 분리한 부분을 확인해주시면 감사하겠습니다 :>