-
Notifications
You must be signed in to change notification settings - Fork 79
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
[team-14 BE] airbnb 프로젝트 2주차 1번째 PR 입니다. #3 #187
Conversation
- WishRepository 추가 - Review, Room, Reservation Repository 메서드 추가
restTemplate사용으로 인해서 응답 속도가 너무 느림 개선 필요
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.
수고 많으셨습니다!
몇가지 개선사항 제안 드립니다. 검토 부탁드려여 💯
public boolean isSame(Member member) { | ||
return this.equals(member); | ||
} |
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.
equals
로 충분하기는 합니다. 뭔가 별도의 로직도 담겨 있지 않은 기분이군요?
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.
equals
라는 메서드 명보다 좀 더 명시적으로 표현하고 싶어서 별도의 로직이 담겨있지 않더라도 작성하게 되었습니다!
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.
equals
와 다른 메서드로 사용하려고 했던 것 같은데...어떤 식이었는지...
수정하겠습니다 ㅎㅎ
@@ -9,7 +9,7 @@ | |||
|
|||
@Entity | |||
@ToString | |||
@NoArgsConstructor | |||
@NoArgsConstructor(access = AccessLevel.PROTECTED) |
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.
👍
public Address(String allAddress, String state, String city, | ||
String street, String zipcode, Point coordinate) { |
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.
@AllArgsConstructor
로 안될까요?
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 static final int M_TO_KM = 1000; | ||
private static final int SEC_TO_MIN = 60; |
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.
정확한 값을 주는 것이 필수적인 로직이 아니라고 생각해서 단순하게 구현했었습니다.
라이브러리를 확인 후 리팩토링해보겠습니다~ 감사합니다
this.duration = toMin(duration); | ||
} | ||
|
||
private double toKm(double distance) { |
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.
타입에 관해서 조금 더 고민해보고 수정해보도록 하겠습니다!
} | ||
|
||
public DistanceInfo caculateDistanceAndDuration(Position position, double destinationX, double destinationY) throws JsonProcessingException { | ||
RestTemplate restTemplate = new RestTemplate(); |
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.
그때그때 새로 만들어서 쓰라고 설계된 클래스가 아니긴 합니다.
정해진 URL에서 정해진 포맷으로, 정해진 엔드포인트를 때려서 대답을 가져와야 하는 로직 아닌가요?
base path를 설정해서 빈 등록을 하시는 것이 좋겠습니다.
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.
넵! 말씀주신대로 빈으로 등록해서 재사용할 수 있도록 리팩토링하겠습니다!
public DistanceInfo caculateDistanceAndDuration(Position position, double destinationX, double destinationY) throws JsonProcessingException { | ||
RestTemplate restTemplate = new RestTemplate(); | ||
String url = getUrl(position.getX(), position.getY(), destinationX, destinationY); | ||
ResponseEntity<String> response = restTemplate.exchange(url, HttpMethod.GET, new HttpEntity<>(getHeaders()), String.class); |
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.
String
이 아닌 응답 포맷에 맞는 클래스로 바로 받는 방법은 없을까요?
서울(126.97857310672501, 37.56654037462486), | ||
용인(127.17751600488093, 37.2409718113933), | ||
수원(127.02869106496286, 37.263501338400935), | ||
강릉(128.8759059060531, 37.752098625346775), | ||
부산(129.075073717621, 35.17975315226952), | ||
전주(127.14807326809175, 35.82363477515946), | ||
여수(127.66230030492281, 34.76021870014728), | ||
경주(129.22480648691675, 35.8561148324664); |
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.
취향이긴 한데...어쨌거나 main
하위의 코드엔 영어와 숫자만 있어야 하지 않나 생각합니다.
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.
클라이언트로 데이터를 전달할 때, 지역 이름을 한글로 전달하는 편이 좋을 것 같아서 한글로 name을 정의했습니다.
말씀대로라면, enum에 한글 이름 필드를 추가하는 편이 맞을까요??
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.
@CMSSKKK 네 그 편을 추천합니다.
2주 1차 PR 요청
안녕하세요! brian
team-14 에서 백엔드를 담당한 로니와 리아코입니다!
한 일
api 목록
Room
- GET rooms/{id} 숙소정보 상세 조회
- GET rooms/{region} 숙소목록 지역정보만으로 조회
- GET rooms/ 숙소 목록 상세 조건으로 조회 (다음 PR 에 구현)
- GET rooms/prices 숙소 가격대 분포(빈도값) 조회 (다음 PR 에 구현)
Reservation
- POST /reservations 숙소 예약하기
- DELETE /reservations/{id} 숙소 예약 취소하기
- GET /reservations/{id} 예약 상세 조회
- GET /reservations 예약 리스트 조회
Wish
- GET /wishes 위시 리스트 목록 조회
- POST /wishes 위시 리스트에 추가
- DELETE /wishes/{id} 위시 리스트에서 제거
해야할 일