feat: 대한민국을 2km단위로 나눠 해당 지역의 충전소 개수를 계산한다 #880
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
sosow0212
requested changes
Oct 18, 2023
Comment on lines
15
to
17
| private Point top; | ||
| private Point bottom; | ||
| private List<Point> points; |
There was a problem hiding this comment.
세 필드에 final을 붙일 수 있지 않나요?
| @@ -26,7 +25,6 @@ public class StationService { | |||
| private final PeriodicCongestionCustomRepository periodicCongestionCustomRepository; | |||
| private final AtomicBoolean isRunning = new AtomicBoolean(false); | |||
|
|
|||
| @Scheduled(cron = "0 0/10 * * * *") | |||
|
|
||
| import java.util.List; | ||
|
|
||
| @RequiredArgsConstructor |
There was a problem hiding this comment.
해당 클래스에서 기본 생성자가 필요하지 않아 보입니다!
| grid.addCount(gridWithCount.count()); | ||
| } | ||
| } | ||
|
|
Comment on lines
+45
to
+49
| return gridWithCounts.stream() | ||
| .skip(lowerBound) | ||
| .limit(upperBound - lowerBound + 1) | ||
| .filter(grid -> grid.longitude().compareTo(minLongitude) >= 0 && grid.longitude().compareTo(maxLongitude) <= 0) | ||
| .toList(); |
There was a problem hiding this comment.
이게 어떤 행위인지 메서드 분리를 통해 명시해주는 건 어떨까요?
이해하는데 한참 걸렸네요 ㅠ
There was a problem hiding this comment.
아이코.. 메서드 분리하고 이름도 적절하게 명시했습니다!
| List<GridWithCount> gridWithCounts = stationGridCacheService.findGrids(request); | ||
| List<Grid> displayGrid = createDisplayGrid(request); | ||
| List<Grid> grids = stationGridService.assignStationGridsWithCount(displayGrid, gridWithCounts); | ||
| List<GridWithCount> list = grids.stream() |
There was a problem hiding this comment.
grids.stream().. 로직이 중복되어 보입니다 메서드로 분리해주면 더 보기 좋을 것 같아요
| class GridGeneratorTest { | ||
|
|
||
| @Test | ||
| void 좌표_2개로_구역을_나눠준다() { |
There was a problem hiding this comment.
모든 테스트에 given - when - then이 빠져 있습니다 ㅠ
| assertThat(assignedGrids).map(it -> it.getPoints().size()) | ||
| .isEqualTo(List.of(0, 0, 0, 1, 0, 0, 0, 0, 0)); | ||
| } | ||
|
|
| ) { | ||
|
|
||
| public static GridWithCount createCenterPoint(Grid grid, int count) { | ||
| BigDecimal centerLatitude = grid.getTop().getLatitude().add(grid.getBottom().getLatitude()).divide(BigDecimal.valueOf(2), 4, RoundingMode.CEILING); |
There was a problem hiding this comment.
체이닝이 너무 긴데, 가독성을 위해서 개행을 추가하거나 어떤 행위를 하는지 메서드로 분리해주시면 안될까요 혹은 grid안에 메서드로 뺄 순 없을까요?
| while (size == 1000) { | ||
| log.info("page : {}", page); | ||
| List<StationPoint> stationPoint = stationQueryService.findStationPoint(page, size); | ||
| stationGridService.assignStationGrids(grids, stationPoint); |
There was a problem hiding this comment.
아~ 모든 충전소를 꺼내서 Grid로 구역을 나눠주는 로직이었군요!
drunkenhw
requested changes
Oct 18, 2023
| while (size == 1000) { | ||
| int finalPage = page; | ||
| List<StationPoint> stationPoints = stationQueryService.findStationPoint(finalPage, size); | ||
| CompletableFuture<Void> future = CompletableFuture.runAsync(() -> stationGridService.assignStationGrids(grids, stationPoints), executor); |
There was a problem hiding this comment.
이 작업에 computablefuture를 사용한 이유가 있을까요? 무엇때문에 사용했는지 궁금합니다
저는 저 클래스에 대해 잘 모르기 때문에 사용 이유를 알고 싶어요
Comment on lines
15
to
18
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| @AllArgsConstructor | ||
| @EqualsAndHashCode | ||
| @Embeddable |
There was a problem hiding this comment.
필요 없는 어노테이션인 것 같아요 그리고 해당 객체를 불변으로 만들어주면 좋을 것 같아요
kiarakim
changed the title
sosow0212
approved these changes
Oct 19, 2023
| public BigDecimal calculateCenterLatitude() { | ||
| Latitude topLatitude = top.getLatitude(); | ||
| Latitude bottomLatitude = bottom.getLatitude(); | ||
| return topLatitude.add(bottomLatitude).divide(BigDecimal.valueOf(2), 4, RoundingMode.CEILING); |
There was a problem hiding this comment.
이 부분도 어떤 로직인지 한 눈에 보이지가 않아요 ㅠ
한 번 더 메서드 분리 해서 어떤 행위인지 뜻을 부여해주면 좋을 것 같아요
| public BigDecimal calculateCenterLongitude() { | ||
| Longitude topLongitude = top.getLongitude(); | ||
| Longitude bottomLongitude = bottom.getLongitude(); | ||
| return topLongitude.add(bottomLongitude).divide(BigDecimal.valueOf(2), 4, RoundingMode.CEILING); |
| if (lowerBound == -1 || upperBound == -1) { | ||
| int lowerBound = searchBoundaryLatitude(minLatitude, START_INDEX); | ||
| int upperBound = searchBoundaryLatitude(maxLatitude, lowerBound); | ||
| if (lowerBound == NO_LATITUDE_FOUND || upperBound == NO_LATITUDE_FOUND) { |
drunkenhw
approved these changes
Oct 26, 2023
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.
📄 Summary
🕰️ Actual Time of Completion
🙋🏻 More
close #819