-
Notifications
You must be signed in to change notification settings - Fork 3
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
[BE] refactor: 발자국 리팩터링 #92
Conversation
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.
리팩토링 좋아요~~
@@ -1,5 +1,7 @@ | |||
package com.woowacourse.friendogly.footprint.domain; | |||
|
|||
import static java.lang.Math.*; |
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.
인텔리제이에서 자동으로 와일드카드를 사용하게 하는 옵션이 켜져있는 걸까요?
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.
reformat 단축키 누르니까 잘 되네요 ,...!
private double radToDeg(double rad) { | ||
return (rad * 180 / Math.PI); | ||
dist = toDegrees(acos(dist)); | ||
return abs(dist * 60 * 1.1515 * 1609.344); |
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.
return abs(dist * 60 * 1.1515 * 1609.344);
이 부분 공식에서 각 값에 의미를 부여하기 힘들다면,
해당 반환문을 메서드로 따로 빼서 네이밍 해주면 이해하기 쉬울 것 같은데 어떠신가요?
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.
이 부분은 어차피 공식이 워낙 복잡해서 하나의 메서드로 가져가도 될 것 같아요...!
@AutoConfigureRestDocs | ||
@WebMvcTest(FootprintController.class) | ||
class FootprintControllerTest { | ||
|
||
@Autowired | ||
private MockMvc mockMvc; | ||
|
||
@Autowired | ||
private ObjectMapper objectMapper; | ||
|
||
@MockBean | ||
private FootprintCommandService footprintCommandService; |
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.
이거 RestDocs Base 코드 있어요. 추상 클래스라서 예시 Member 문서 테스트 참고해서 다시 작성해주시면 감사하겠습니다.
@@ -20,7 +20,7 @@ public class FootprintCommandService { | |||
|
|||
public Long save(SaveFootprintRequest request) { | |||
Member member = memberRepository.findById(request.memberId()) | |||
.orElseThrow(() -> new IllegalArgumentException("멤버 없음")); | |||
.orElseThrow(() -> new IllegalArgumentException("존재하지 않는 사용자 ID입니다.")); |
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.
이 부분도 저희 커스텀 예외 만들어둔걸로 대체해주세요.!
예외처리 repo로 내리는건 추후에 service 두꺼워지면 고려해봅시다
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.
반영했습니다 👍
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.
트레~ 고생했어요
코멘트 확인 부탁드려요!
.orElseThrow(() -> new IllegalArgumentException("멤버 없음")); | ||
.orElseThrow(() -> new IllegalArgumentException("존재하지 않는 사용자 ID입니다.")); |
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.
#93
develop 브랜치 rebase 후에 IllegalArgumnetException
대신 Custom Exception 사용해주세요!
private static final int HOURS_AGO = 24; | ||
private static final int FOOTPRINT_REMAIN_HOURS = 24; |
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.
해당 상수는 실제로 현재 시간으로 부터 n시간 이내
를 나타내는데 사용되는 반면,
REMAIN_HOURS
라는 네이밍은 남은 시간
이라는 의미가 강한 것 같아요. 조금 더 적절한 네이밍을 고민해보면 좋겠습니다!
private double degToRad(double deg) { | ||
return (deg * Math.PI / 180.0); | ||
} | ||
private double distanceAsMeter(Location other) { |
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.
private double distanceAsMeter(Location other) { | |
private double calculateDistanceAsMeter(Location other) { |
메서드명은 동사로 시작하는게 좋겠습니당
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.
동사로 통일!! 👍
private double latitude; | ||
private double longitude; |
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.
약어만 사용하거나, 정확한 표현만 사용하거나 둘 중 하나로 통일하면 좋겠어요.
추가로 약어를 사용하는게 과연 좋을지 잘 모르겠어요.
오랜 시간이 지나고 해당 도메인을 유지보수할 때, 경도
를 나타내는 표현이 lon
이었는지 lng
였는지 헷갈리지 않을까요?
또 여러 개발자들과 협업하는 상황에서는 인지 비용을 줄이기 위해서라도 정확한 표현을 사용하는 것이 좋지 않을까 생각합니다. 쿼리 파라미터에서도 사용될 표현이기 때문에 안드로이드 크루들도 인지해야 한다는 점 또한 고려해야겠네요.
개인적으로, 적어도 협업에 있어서는 개발의 간편함 보다는 이해하기 쉬운 코드를 작성하는 것이 더 높은 우선순위를 가져야하지 않나 싶습니다.
투기장 오픈, 아무나 참전 가능(1/8)
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.
query parameter를 짧게 가져가고 싶은 욕심이 있었는데, 일관성까지 챙기기는 확실히 어렵네요.
- Swaager UI , api 테스트 가능하도록 하기 위해 CORS 설정 추가
Test Results3 tests 3 ✅ 0s ⏱️ Results for commit 451caa7. ♻️ This comment has been updated with latest results. |
이슈
개발 사항
전달 사항
비고