Skip to content
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-08 마르코] 메인 페이지 조회, 숙소 조회 및 가격 조회 #52

Open
wants to merge 119 commits into
base: team-08
Choose a base branch
from

Conversation

95degree
Copy link

@95degree 95degree commented May 28, 2021

안녕하세요 백엔드 마르코입니다.
부족함이 많은 코드이지만 잘 부탁드립니다. 😥
현재까지 구현한 내용은 아래와 같습니다.

  1. 메인 페이지 조회(도시정보,카테고리 정보)
  • 도시 정보
  • 숙소 카테고리 정보
  • 위도 경도 계산 시 현재는 유저 구현이 되어있지 않아 코드스쿼드를 기준으로 했습니다.
  1. 가격 조회
  • 전체 숙소의 가격 리스트
  • 유저가 날짜와 도시를 선택했을 때 가능한 숙소의 가격 조회
  1. 숙소 조회
  • 예약인원,체크인 체크 아웃 날짜, 도시, 가격 에 부합하는 숙소들 조회
  • 현재 평점, 호스트, 리뷰 등은 구현하지 않아 default 값으로 처리 했습니다.

API 문서 : https://documenter.getpostman.com/view/14894886/TzRYbPW8

감사합니다.

swing-park and others added 30 commits May 17, 2021 15:33
도시에 관한 정보를 담기 위해 Wrapper dto생성
@JsonRootName을 사용하기 위해 objectMapper 설정
CityList를 Wrapper로 감싸주기 위해 CityListWrapper를 생성
모든 도시의 값을 가져오기 위해 findAll 메소드를 생성
City 객체 반환을 위해 만든 Mapper를 사용한 로직을 추가
dataSource를 추가 해서 실행 하기 위해 어노테이션 제거
- db와의 연결을 위해 dataSource 추가
- 쿼리문을 보기 위해 logging 설정 추가
- CityDao 의존성 주입
- 모든 도시의 정보를 찾아오기 위해 readCities() 추가
- City(도시정보) INSERT문 작성
- List<City>를 감싸기 위한 Wrapper 클래스
- json으로 출력될때 어떤 데이터의 정보인지 알 수 있게 Wrapper로 감싸서 리턴하도록 변경
- Image의 정보를 담고 있는 도메인
95degree and others added 22 commits May 25, 2021 15:37
- RoomId로 방과 관련된 이미지 List를 가져온다.
- 저장된 모든 숙소의 정보를 가져오기 위해 findAll 메소드 생성
- db의 컬러명과 같아야 하기 때문에 wifi로 변경
- Room과 관련된 비지니스 로직을 처리하기 위해 RoomService 생성
- 일일이 add로 넣어줄때 생길수 있는 사이드 이펙트 방지를 위해 stream으로 변경
- findAll()로 모든 숙소를 가져와 Price를 오름차순으로 정렬해서 보여준다.
- db에서 정렬된 상태를 받아오는 것이 아니라 서버에서 정렬을 하도록 로직을 변경
- 도시 Id와 스케쥴을 클라이언트에서 받기 위해 Dto생성
- 도시 Id와 스케쥴을 클라이언트에서 받기 위해 Dto생성
- 가격 범위
- 최대 인원
- 도시
- 체크인 체크아웃
- 가격 범위
- 최대 인원
- 도시
- 체크인 체크아웃
- GET , POST만 접근 가능
- 최대 인원이 아닌 예약 인원을 받으므로 maxPeopleCount에서 resrvationPeopleCount으로 변경
- 스트림 findFirst로 메인 이미지 url을 찾아오도록 로직을 변경
[BE] 숙소 조회 및 가격 조회 API
@ksundong ksundong self-requested a review May 28, 2021 16:22
@ksundong ksundong self-assigned this May 28, 2021
Copy link
Member

@ksundong ksundong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요! 마르코! 리뷰어 Dion입니다.
전체적으로 느껴지는 점은 쿼리를 짜기보다는 쿼리 결과를 조립한다는 느낌을 많이 받았습니다.
꼭, JOIN을 활용해서 효율적인 코드를 작성하실 수 있으면 좋을 것 같아요.

전반적으로 아쉬움이 많이 느껴지는 코드였습니다.
제가 이전에 말씀드렸듯, 이게 과연 최선을 다한 코드였는지 확인해보시면 좋을 것 같다는 생각이 들어요.

아무래도 쿼리를 많이 사용하지 않았었고, 그로 인해서 학습 비용이 올라갔기 때문에 전반적으로 신경을 못쓰신 느낌이 있었습니다.
일단, 원하는 대로 동작하도록 먼저 만드시고 꼭 리팩토링을 해보세요. 보다 나은 방향으로의 개선을 계속해서 시도해주세요.
제가 리뷰드린 부분을 반영해주시고 재요청을 주시면 다시 리뷰해드리겠습니다.

고생많으셨어요.

Comment on lines +21 to +23
public List<Category> findAll() {
return categoryDao.findAll();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굳이 이 메서드를 사용할 이유가 있나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 굳이 현재는 나눌 필요가 없었는데 너무 먼 미래를 생각했네요 수정하겠습니다.

Comment on lines +25 to +29
public List<CategoryResponse> createAllToCategoryResponseList() {
List<Category> categories = findAll();
return categories.stream().map(category -> CategoryResponse.of(category, category.findMainImageUrl()))
.collect(Collectors.toList());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정말 생성하는게 맞나요?
내부의 구현을 드러내는 구조는 아닐까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

        return categories.stream().map(CategoryResponse::of)
                .collect(Collectors.toList());

내부구현이 들어나지 않도록 위의 코드처럼 Dto 내부에서 처리하도록 수정하겠습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분 이외의 서비스에서도 내부의 구현이 드러나는 부분을 고치겠습니다.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

내부의 구현이 드러난다는 말은 메서드가 하는 동작을 시그니처로 드러내면 안된다는 의미였습니다!
확인부탁드릴게요!

}

public List<CityResponse> createAllToCityResponseList() {
Location codeSquadLocation = new Location(37.491016774047345, 127.03339554026415);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

매번 생성해줘야 할까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

매번 생성하지 않도록 멤버변수로 따로 빼놓겠습니다.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

멤버변수가 아니라, 상수로 꺼내도 좋을 것 같네요.

Comment on lines +17 to +19
public Image findHeroImage() {
return imageDao.findByType(ImageType.HERO.name()).get(0);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

내부의 구현을 드러내고 있고, get(0)을 사용하는 이유는 잘 모르겠네요.
조회가 되지 않으면 어떻게 될까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findByType()을 가지고 HERO이외의 MAIN,DETAIL에 재활용하기 위해 위와 같은 구조로 구현을 했습니다. 하지만 디온 말씀대로 내부의 구현이 드러나게 되네요.
내부가 드러나지 않도록 findMainImageByCityId, findMainImageByRoomId 등 쿼리를 통해 받아 오도록 수정하겠습니다.

Comment on lines +27 to +29
public List<Room> findAll() {
return roomDao.findAll();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전체적인 구현이 findAll()에만 의존하는 형태인데
과연 이게 맞을까요? 잘 생각해봅시다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 프론트에서 전체 숙소의 정보나 가격을 받아오길 원하셔서 findAll()에 의존이 많이 되어있는 형태입니다.
만약 무조건 전체 데이터를 가져와야 하는 상황이면 페이징 처리를 해서 효율적으로 처리하면 될꺼 같습니다.
맞는 생각일까요?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 findAll은 사실 실제 환경에선 전혀 사용할 일이 없는 메서드죠.
페이징을 이용해주시고, 실제 조회로직은 DB에서 수행해줄 수 있도록 해주는 게 좋을 것 같아요.

Comment on lines +12 to +16
@JsonProperty(value = "mainImage")
private String mainImageUrl;

@JsonProperty(value = "detailImage")
private List<String> detailImageUrls;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굳이 달라야 했나 싶어요.
또, 가급적이면 Collection들은 복수형을 추천드릴게요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

main과 detail을 따로 분리해서 보내면 프론트분들이 편하실꺼 같다고 생각했습니다.
그냥 나눌 필요 없이 한번에 보내는 편이 좋은건가요??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 둘이 다르단 얘기가 아니라
변수명을 의미했습니다. 제가 목적어를 빼먹었네요!


public static ImageResponse of(List<Image> images) {
String mainImageUrl = images.stream().filter(image -> ImageType.MAIN.equals(image.getImageType())).findFirst()
.orElseThrow(NullPointerException::new).getUrl();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NPE는 발생시키지 말아주세요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

쿼리로 받아오도록 수정하겠습니다.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NPE 대신 다른 Exception을 직접 정의해서 반환해주세요!


public class PriceRequest {

private final Long cityId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cityId를 주는 것 보다는 city 명을 주는게 낫지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

city명을 받으면 city 명으로 cityId를 찾고 다시 cityId로 숙소를 검색해야되는 불필요한 상황이 발생할꺼 같아 cityId를 받아오고 있습니다. city명을 받아오도록 수정하겠습니다.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JOIN을 이용해서 cityName으로 검색해서 데이터를 가져오면 되겠죠?
cityId를 이용해도 됩니다. 다만 클라이언트 쪽에서 cityId를 알아내야 하는 상황이 발생하게 되겠죠.
그걸 DB쪽에서 처리해주는 개념으로 생각해주시면 좋을 것 같아요.

Comment on lines +9 to +15
public class CategoryMapper implements RowMapper<Category> {

@Override
public Category mapRow(ResultSet rs, int rowNum) throws SQLException {
return new Category(rs.getLong("id"), rs.getString("name"));
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런류의 인터페이스는 람다를 쓸 수 있습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FunctionalInterface 는 람다를 사용 할수 있다는 것을 알게되었습니다. 감사합니다.


import airbnb.dto.MainPageResponse;

public class MainPageWrapper {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

왜 Wrapping해주나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{"mainPage" : { "cities":[],
                       "categories":[]
                      }
}

이런 씩의 형태로 보내려고 해서 Wrapping을 해주는 형태입니다.
다시 생각해보니 굳이 감싸줄 필요가 없는 것 같네요 수정하겠습니다.

@95degree
Copy link
Author

95degree commented Jun 1, 2021

안녕하세요 디온 :) 마르코입니다.
항상 피가되고 살이 되는 좋은 리뷰를 남겨주셔서 감사합니다.
디온 덕분에 많은 것을 공부하고 더욱 성장 할 수 있을꺼 같습니다. 다시 한번 감사드립니다. 💪💪💪😃
일단은 구현을 하고 수정해볼 생각입니다. 일단은 어떤 씩으로 수정할지 코멘트를 남겨봤습니다.
중간중간에 질문사항도 있습니다.
답변주시면 감사하겠습니다. 디온은 진짜 짱짱 감사드리고 싶은 리뷰어입니다. 🤞🤞🤞

crongro pushed a commit that referenced this pull request Jun 7, 2021
* feat: 인원 모달 UI 작업

- 인원 모달에 필요한 상태를 SearchBar에 context로 생성
- 인원 모달 UI 생성

Dae-Hwa/airbnb/#48

* feat: guest type에 따라 HeadCount 컴포넌트의 내용을 다르게 렌더링

- +,- 버튼을 컴포넌트로 분리
- Counter 컴포넌트 분리

Dae-Hwa/airbnb/#48

* refactor: HeadCount에서 객체 리터럴에 바로 접근할 수 있도록 타입 추가

Dae-Hwa/airbnb/#48

* feat: counter 기본 동작 구현

- 분리해둔 PlusButton, MinusButton에 onClick 이벤트를 넣으려니 또 타입을 선언해줘야 해서
임시로 svg 컴포넌트를 가져와서 onClick 이벤트 등록함
- useReducer 사용하지 않고 그냥 만들어본 버전
- 타입.. 아찔하다.

Dae-Hwa/airbnb/#48

* feat & fix: 인원모달 관련된 상태를 SearchBar 컴포넌트로 끌어올림, 게스트 타입 별 상태 조작 로직 추가, 타입 에러 해결

- 상태 끌어올리고 Counter 컴포넌트에서 useContext를 사용하려고 하니 타입에러가 나서 Counter 컴포넌트가 위치한 곳에서 커스텀훅을 사용하여 해결

Dae-Hwa/airbnb/#48

* refactor: Counter 컴포넌트에서 useContext 부분 타입 에러 다른 방법으로 해결

- HeadCountContextType 정의 부분에서 null 타입 추가
- setGuestCountState null 체크하는 코드 추가 (Counter.tsx 18라인)
- createContext 부분에서 초기값을 key별로 null 설정
- 커스텀훅 삭제

Dae-Hwa/airbnb/#48
wheejuni pushed a commit that referenced this pull request Jun 7, 2021
- 필터링 된 방을 조회하기 위한 findFilteredRooms 메서드 구현
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants