-
Notifications
You must be signed in to change notification settings - Fork 42
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
[Team21] BE - Bibi, yeon : API와 OAuth 구현 #74
base: team21
Are you sure you want to change the base?
Conversation
close#3
getRoomsByDateAndPriceAndNumberOfPeople 메소드 추가
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.
안녕하세요~ 오랜만에 리뷰 드립니다. ㅎㅎ
전체적으로 일관된 방식으로 코드를 짜려고 하신 게 느껴집니다. 짧게짧게 간결하게 하려고 노력하신 것 같고요! 잘 하셨습니다~ 제가 드린 코멘트가 어느정도 인사이트가 되었음 좋겠네요. 화이팅입니다!
@Bean | ||
public RestTemplate restTemplate(RestTemplateBuilder restTemplateBuilder) { | ||
return restTemplateBuilder | ||
.requestFactory(() -> new BufferingClientHttpRequestFactory(new SimpleClientHttpRequestFactory())) |
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.
요 설정 특별히 하신 이유가 있으신가요~?
|
||
@RestController | ||
@RequestMapping("/api/booking") | ||
@CrossOrigin(origins = "*", allowedHeaders = "*") |
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.
이왕이면 * 말고 제대로 등록해보시면 학습에 도움이 많이 될것 같습니다!
|
||
@RestController | ||
@RequestMapping("/api/rooms") | ||
@CrossOrigin(origins = "*", allowedHeaders = "*") |
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.
자주 나오는 어노테이션이라면 WebConfiguer에서 등록할 수도 있습니다.
return new Rooms(roomService.getRoomDTOS()); | ||
} | ||
|
||
@GetMapping(params = {"checkIn", "checkOut", "minPrice", "maxPrice", "numberOfPeople"}) |
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.
GetMapping 에 params 를 사용하는 것은 처음 보는 것 같은데요! ㅎㅎ 특별히 이유가 있으신가요~ 그렇지 않다면 RequestParam에 있는 것만으로 충분하지 않을까 생각하네요~
public Rooms searchRooms(@RequestParam("checkIn") @DateTimeFormat(pattern = "yyyy-MM-dd") LocalDate checkIn, | ||
@RequestParam("checkOut") @DateTimeFormat(pattern = "yyyy-MM-dd") LocalDate checkOut, | ||
@RequestParam("minPrice") int minPrice, @RequestParam("maxPrice") int maxPrice, | ||
@RequestParam("numberOfPeople") int numberOfPeople) { |
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.
어노테이션 value에camel case를 사용한다면 굳이 적으시지 않아도 값이 잘 넘어옵니다. 한번 지우고 테스트 해보세요!
} | ||
|
||
public ResponseEntity<String> createGetRequest(OAuthToken oAuthToken) { | ||
String url = "https://www.googleapis.com/oauth2/v1/userinfo"; |
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.
요 변수는 static으로 빼는게 좋을 듯 하네요!
String sql = "select room.id, `max`, `name`, `rating`, `latitude`, `longitude`, `bedroom_count`, `bed_count`, " + | ||
"`bathroom_count`, `address`, `detail_address`, `comment_count`, `original_price`, `sale_price`, " + | ||
"`flexible_refund`, `immediate_booking` from `room` " + | ||
"left join `booking` on room.id = booking.room_id " + | ||
"where (`sale_price` between ? and ?) and `max` >= ? and " + | ||
"((? not between check_in and check_out and ? not between check_in and check_out) or " + | ||
"(check_in is null and check_out is null))"; |
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.
필터링 요청에 minPrice나 numOfPeople 등이 null일 수 있을 것 같은데, 몇몇값이 null이어도 동작이 잘 할까요?
private final UserRepository userRepository; | ||
private final OAuthService oauthService; |
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.
실제로 XXXService에 XXXRepository만 들어가 있지 않습니다. ㅎㅎ 필요한 경우에 여러가지 서비스가 의존을 맺고 있습니다.
음.. AuthService라고 이름을 바꾸고 UserRepository와 OAuthService를 넣었다고 해도 되겠네요!
좀 더 개선해본다고 하면, 엔티티 Repository와 CRUD를 주로 수행하는 엔티티 Service는 엔티티와 가장 가까운 로직으로 보고.. 요렇게 의존관계를 맺어볼 수도 있습니다.
class UserService {
private UserRepository userRepository;
public User findById();
public User findAllByXX();
public User update(User user);
public User delete();
.. 등등 순수히 엔티티를 조작하는 서비스
}
class AuthService {
private UserService;
private OAuthService;
public void login(String code);
}
ResponseEntity<String> accessTokenResponse = oauthService.createPostRequest(code); | ||
OAuthToken oAuthToken = oauthService.getAccessToken(accessTokenResponse); | ||
logger.info("Access Token: {}", oAuthToken.getAccessToken()); | ||
|
||
ResponseEntity<String> userInfoResponse = oauthService.createGetRequest(oAuthToken); | ||
GoogleUser googleUser = oauthService.getUserInfo(userInfoResponse); | ||
logger.info("Google User Name: {}", googleUser.getName()); |
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.
요 부분은 OAuthService 안으로 들어가는게 적절해 보입니다~
@Test | ||
@DisplayName("검색 조건에 맞는 room들을 조회한다.") | ||
void checkJoinedAndFilteredTable() { | ||
List<Room> rooms = roomRepository.getFilteredRooms(LocalDate.of(2021, 5, 20), | ||
LocalDate.of(2021, 5, 21), | ||
55000, 80000, 2); | ||
rooms.forEach(System.out::println); | ||
} |
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.
검색조건의 몇개를 Null 로 해본다면..
안녕하세요!
airbnb 프로젝트 21팀의 백엔드 bibi, yeon입니다.
이번 프로젝트 요구사항을 어느정도 구현하여 PR 드립니다😀
💁♂️배포 URL : http://54.180.145.53:8080
💁♀️API 설명 : https://github.com/bibi6666667/airbnb/wiki/%5BBE%5D-API
구현한 부분들
구현하며 궁금했던 점
@Service
단으로 OAuthService라는 클래스로 분리하고 UserService에서 이를 참조하도록 했는데 올바른 방법인지 여쭤보고 싶습니다.시간내어 리뷰해주셔서 감사합니다.