[Team-18 새리,Starve][BE] 숙소 조회 및 예약에 관한 기본 API 구현 #62
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
기본적으로 사용할 도메인 class들을 생성하였다.
DB에 테이블과 database를 생성하는 schema를 생성하였다.
DB에 기본 데이터 생성을 위한 insert query 생성
[BE]초기 세팅작업 진행
main 화면에서 보여 줄 CollectionAnywhere에 관련된 table 작성
DB에 CollectionAnywhere에 관한 데이터 생성을 위한 insert query 생성
메인에서 띄워줄 카테고리 항목 추가
[BE] property_category에 관련된 데이터 추가
domain 수정
일단은 findById를 resultSet으로 받아와 생성자에 넣도록 구현했다. 로케이션의 경우 DTO가 크게 필요하지 않아 빌더를 쓰지는 않았고 세터를 쓰는 것 보다 처음 객체 생성 시 파라미터로 리저트 셋의 값을 가져오는 게 더 나을 것 같다는 판단으로 구현했다.
테스트 데이터로 넣어놓은 로케이션을 아이디로 찾는다.
여러 객체를 받아올 때 쓸 매퍼(LocationMapper)를 이너클래스로 구현하고 findAll에서 여러 리저트 셋을 매퍼를 이용해 객체로 변환
findAll만 구현해놓은 상태라 매퍼를 따로 뺴지 않아도 될 것 같아서 두었다.
미리 넣어둔 카테고리 네개로 잘 불러오는 지 확인했다.
title, image_url, bookmark, pricePerNight, totalPrice, reviewCount, rating을 갖는 PropertyDto를 List로 저장해, 반환하는 PropertiesResponseDto 생성
Id 값으로 Property 찾기, 전체 Property 찾기, 조건에 따른 PropertiesDtoResponse 찾기 가능
Image가 별도의 List에 담아져야하는데, Dto를 이미지의 개수만큼 만드는 문제 때문에 별도로 DB에서 추출해 List에 넣는 방식으로 변경.
DAO에서 리저트셋으로 데이터를 가져와 내보낼 때 id 지정
ksundong
requested changes
Jun 1, 2021
There was a problem hiding this comment.
안녕하세요! 새리 & Starve!! 리뷰어 Dion입니다.
프로젝트 진행하시느라 고생많으십니다.
먼저, 코드를 보면서 아직까지 기본적인 부분이 부족한 느낌을 받았습니다. 서로가 서로의 코드를 리뷰해주고 있나요?
꼭 짝 프로그래밍 만이 협업은 아닙니다. 코드리뷰를 통해서 부족한 부분을 보완하고 새로운 부분을 학습하는 과정을 충분히 경험할 수 있습니다.
더군다나 글이기 때문에 자연스럽게 기록으로 남길수도 있습니다. 꼭 해보셨으면 좋겠어요.
- DAO에서 ResultSet으로 꺼내온 데이터를 도메인 타입을 거치지 않고 DTO로 바로 매핑한다면 쓰이지 않는 도메인 클래스는 삭제해도 괜찮은가요?
- 해당 Domain이 정말 Domain이 맞는지 생각해보세요. 사실 만들어주신 도메인 클래스들은 그냥 테이블과 매핑하는 용도로밖에 보이지 않습니다.
- 강의를 보던 중 DTO 클래스는 생성자 없이 getter, setter만을 넣는다는 이야기를 들었는데 지금까지 세터의 사용은 지양해야한다고 알고있어서 이 부분이 다소 헷갈립니다. 게터, 세터(+기본생성자)는 로직을 가지지 않는 DTO라 가능한 조합일까요? 또, DTO여도 세터 없이 모든 필드를 파마리터로 받는 생성자를 만들어줘도 괜찮은가요?
- DTO는 객체지향 원칙을 적용시키지 않아도 되지만 제가 알기론 Brian같은 경우에는 DTO를 불변으로 관리해주신다고 알고 있는데 확실하지는 않네요. 생성자는 넣어주시는 편이 좋습니다. 이는 스프링에서 데이터 매핑을 할 때 사용하는 매퍼들이 제각각 달라서 사용되는 위치마다 형태가 다 달라지게 되는데, 나중에 더 학습해보시는게 좋을 것 같아요.
- 선택적 파라미터의 갯수가 많아지면 Builder 패턴을 사용하는 것이 코드의 가독성을 높일 뿐만 아니라 같은 자료형의 파라미터가 여러개일 때 실수를 줄여준다고 알고는 있으나,setter 혹은 생성자만으로 충분할 것 같다는 판단을 내려 현재의 코드를 작성했지만, 저희의 코드에서 Builder를 쓰는 것이 훨씬 더 나은 방법일지 궁금합니다.
- 일단 파라미터가 5개를 넘어가는 순간부터는 빌더패턴의 사용을 고려하는게 낫습니다. 또한, 빌더 패턴은 은총알이 아니기 때문에 안티패턴이 될 수도 있습니다. https://dev.to/siy/when-builder-is-anti-pattern-3j92 이 글과 댓글을 꼭 읽어보세요.
코멘트 남긴 부분 확인해주시고, 코멘트 남겨주세요. 확인 후 머지하도록 하겠습니다!
고생하셨습니다~
| @@ -0,0 +1,26 @@ | |||
| .gradle | |||
There was a problem hiding this comment.
.gitignore는 초기단계에 직접 정의해주시기보다는
gitignore.io 같은 곳에서 preset을 가져오신다음
필요한 부분을 조정해서 사용해주시길 추천드리겠습니다~
Comment on lines
13
to
18
| private MainService mainService; | ||
|
|
||
| @Autowired | ||
| public MainController(MainService mainService) { | ||
| this.mainService = mainService; | ||
| } |
There was a problem hiding this comment.
Suggested change
| private MainService mainService; | |
| @Autowired | |
| public MainController(MainService mainService) { | |
| this.mainService = mainService; | |
| } | |
| private final MainService mainService; | |
| public MainController(MainService mainService) { | |
| this.mainService = mainService; | |
| } |
Comment on lines
20
to
23
| @GetMapping("/main") | ||
| public ResponseEntity<MainDTO> getMain() { | ||
| return ResponseEntity.ok().body(mainService.browseMainDTO()); | ||
| } |
There was a problem hiding this comment.
되도록이면 ResponseEntity의 사용은 정말 필요한 경우가 아니라면 지양해주세요.
| @RestController | ||
| public class PropertyController { | ||
|
|
||
| PropertyService propertyService; |
| } | ||
|
|
||
| @GetMapping("/{propertyId}") | ||
| public ResponseEntity<PropertyDetailResponseDTO> propertiesAverageValue(@PathVariable Long propertyId) { |
Comment on lines
+36
to
+44
| int[] getPropertyCountsByPriceRange(Long locationId) { | ||
| int[] priceCounts = new int[20]; | ||
| List<Integer> prices = propertyDao.findPricesByLocationId(locationId); | ||
|
|
||
| for (Integer i : prices) { | ||
| priceCounts[i / 50000]++; | ||
| } | ||
|
|
||
| return priceCounts; |
| } | ||
|
|
||
| public PropertiesResponseDTO findBy(Long locationId, LocalDate checkIn, LocalDate checkOut, int minPrice, int maxPrice, int adult, int children, int infant) { | ||
| long diff = 1; |
Comment on lines
+69
to
+74
| propertyDTOS | ||
| .forEach(propertyDTO1 -> { | ||
| propertyDTO1.setImages(propertyDao.findImageByPropertyId(propertyDTO1.getPropertyId())); | ||
| propertyDTO1.setTotalPrice(finalDiff); | ||
| } | ||
| ); |
There was a problem hiding this comment.
그냥 enhanced for 문을 사용해보는건 어땠을까요?
| spring.datasource.password=codesquad1 | ||
|
|
||
| logging.level.com.codesquad.airbnb.*=debug | ||
| logging.level.org.springframework.jdbc.*=trace |
Comment on lines
+14
to
+26
| @SpringBootTest | ||
| class CategoryDAOTest { | ||
|
|
||
| private Logger logger = LoggerFactory.getLogger(CategoryDAOTest.class); | ||
|
|
||
| @Autowired | ||
| private CategoryDAO categoryDAO; | ||
|
|
||
| @Test | ||
| void CategoryDAO_findAll() { | ||
| List<Category> categories = categoryDAO.findAll(); | ||
| assertThat(categories.size()).isGreaterThanOrEqualTo(4); | ||
| } |
There was a problem hiding this comment.
DB와 연결된 테스트는 깨지기도 쉬울뿐더러 그리 의미있는 테스트가 되기는 어렵습니다.
물론 JDBCTemplate을 사용하니 검증을 위해서 사용하는건 나쁘지않은 것 같아요.
Refactor: JdbcTemplate -> NamedParameterJdbcTemplate으로 변경
- ResponseEntity 사용 지양 - 접근제어자 설정 - 메서드 네이밍 변경 - 어노테이션 추가
- 로우매퍼 분리 - 로우매퍼를 싱글턴으로 - DAO에서 싱글턴 매퍼의 getInstance() 이용
refactor: RequestParam 을 Class로 Mapping 하여 수정
wheejuni
pushed a commit
that referenced
this pull request
Jun 7, 2021
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.
안녕하세요! API구현을 마쳐 PR을 보내게 된, Team-18 BE 새리,Starve 입니다.
부족한 부분이 많은 코드지만, 이번 리뷰도 잘 부탁드립니다!
API 링크
구현 사항
구현 할 사항
궁금한 점
감사합니다!