[Team - 06][BE] 에어비앤비 프로젝트 2주차 1회차 PR #140

leejohy-0223 · 2022-05-31T08:01:59Z

안녕하세요 Dion!

우선 저희 팀의 리뷰를 담당해 주셔서 감사의 인사를 전합니다!! 주요 변경사항으로는 Native Query를 JPQL로 변경한 부분입니다.
수정사항이 많지는 않습니다만, 한 가지 질문이 있습니다!

OAuth 로그인을 구현하려고 하는데, 이전 프로젝트에서 리뷰해주셨던 내용에 제가 답변을 달지 못해서 다시 질문 드립니다(죄송합니다..😭)

    @PostMapping("/order")
    public void order(@RequestBody OrderItemDto orderItemDto, HttpServletRequest request) {
        int userId = (Integer) request.getAttribute("userId");
        orderItemDto.setUserId(userId);
        orderService.order(orderItemDto);
    }

   // OrderItemDto 내에서 Order 객체를 생성하는 부분
    public Order toEntity() {
        return new Order(null, amount, LocalDateTime.now(), itemId, userId, false);
    }

주문 시 JWT 토큰으로부터 사용자 Id를 추출한 후에 컨트롤러에서 OrderItemDto에서 set으로 userId 설정을 한 후 order를 진행하는 코드입니다. 해당 리뷰에서 dto에 userId를 set할 필요가 있냐고 남겨주셨습니다.
이번 구현에도 user 정보를 위와 같은 방식으로 처리할 계획이었는데, 앞서 남겨주신 리뷰에서 DTO에 set을 사용한 부분을 질문주신걸까요? 의도로는 DTO의 내부에 toEntity를 통해 엔티티로 변경하는 부분이 있기 때문에, userId를 알고있다면 더 편리하게 변경할 수 있을거라고 의도하였습니다!

ksundong

안녕하세요. Shine & Lucid 리뷰어 dion입니다.

코드 변경하느라 고생많으셨습니다.

NativeQuery를 JPQL로 변경해주셨네요. 고생하셨습니다.
그 밖의 부분에 대해서도 고쳐주셨으면 좋겠다는 생각이 들어 리뷰 남겼습니다.
다음 PR 올리실 때 고쳐서 올려주세요.

질문에 대한 답변

왜 userId를 set 해주는걸까요? 여기서부터 공감대가 형성되지를 않습니다.
보다 객체지향적으로 생각해보시길 바랍니다.

ksundong · 2022-05-31T14:22:01Z

BE/scripts/health_check.sh

+    if [ ${RESPONSE_CODE} -eq 200 ]; then
+        echo "> New WAS successfully running"
+        exit 0
+    elif [ ${RETRY_COUNT} -eq 10 ]; then
+        echo "> Health check failed."
+        exit 1


각 exit code의 의미에 대해서 혹시 알고 계신가요~?

0 이면 정상 종료, 1이면 일반적인 에러로 인한 종료로 알고있습니다!

여러 exit code 들이 있는데요. 약간 교양 수준으로 알아두시면 좋습니다.

ksundong · 2022-05-31T14:22:53Z

BE/scripts/health_check.sh

+for RETRY_COUNT in 1 2 3 4 5 6 7 8 9 10
+do
+    echo "> #${RETRY_COUNT} trying..."
+    RESPONSE_CODE=$(curl -s -o /dev/null -w "%{http_code}"  http://127.0.0.1:${TARGET_PORT}/api/health)


Spring Actuator 프로젝트에 대해서 학습해보시면 좋을 것 같네요~

키워드 감사합니다!!

ksundong · 2022-05-31T14:26:10Z

BE/src/main/java/com/airbnb/api/houses/dto/HouseCountResponse.java

+import com.airbnb.repository.dto.HouseCount;
+
+public class HouseCountResponse {
+    private List<HouseCount> houseCounts;


HouseCount라는 이름에서 약간 의아해졌어요. 무엇을 표현하기 위한 클래스인가요?

�가격대 별 숙소의 수를 반환해서 막대 그래프를 그려주기 위한 응답입니다.
다른 이름을 고려해보도록 하겠습니다!

네 좀 더 좋은 이름이 나오면 좋을 것 같아요!

ksundong · 2022-05-31T14:27:25Z

BE/src/main/java/com/airbnb/repository/dto/HouseCount.java

+    private Long count;
+
+    public HouseCount(Integer price, Long count) {
+        this.price = price * 10000;


10000 을 곱해주는 이유는 무엇인가요?:

DB에서 group By를 통해 10,000원 단위로 개수를 가져오게 됩니다.
예를 들어, 23,000원, 25,000원, 52,000원의 숙소가 있다고 가정하면 10000으로 나눈 몫으로 grouping을 하면 다음과 같습니다.

2 -> 2개

5 -> 1개

이 때 클라이언트에 전달하기 전, 20,000원 대 / 50,000원 대 숙소라는 의미를 명확히 부여하기 위해 10000을 곱해서 전달해주도록 하였습니다.

자~ 그러면 이 10000을 곱하는 이유를 어떻게 코드로 표현할 수 있을까요?
한 번 고민해보시면 좋을 듯 합니다.

ksundong · 2022-05-31T14:28:14Z

BE/src/main/java/com/airbnb/repository/dto/HouseCount.java

@@ -0,0 +1,30 @@
+package com.airbnb.repository.dto;
+
+public class HouseCount {


가급적이면 data 성의 클래스는 equals hashCode 오버라이딩도 해주세요~

data 성의 class같은 경우 단순하게 데이터 전달의 기능도 하지만, 향후, Collections에 저장될 수도 있기에 가급적이면 equals, hashCode 를 구현하도록 하겠습니다.(만... 이게 음 나중에 Collection에 저장할지 아직 의문인 데이터성 Class들도 꼮 만들어 줘야 할까요?)

equals hashCode 에 대해서 다시 한 번 학습해보시고 질문을 주시면 좋을 것 같아요~~

ksundong · 2022-05-31T14:28:58Z

BE/src/main/java/com/airbnb/repository/dto/HouseCount.java

@@ -0,0 +1,30 @@
+package com.airbnb.repository.dto;


repository 안에 DTO가 있는 구조는 굉장히 의아하네요. 이렇게 패키지 설계를 해주신 의도가 무엇일까요?

가격대 별 수량 조회라는 의미를 가진 DTO를 구현하려다보니, 당연히 해당 repository 패키지에서 dto를 관리해야한다고 판단한 것 같습니다.😭
DTO 조회용 repository를 따로 구현하는 방법이 더 나은 방식일까요?

아하 그러한 의도군요. repository용 DTO를 만들어주는 개념이라면 맞는 것 같습니다.

ksundong · 2022-05-31T14:30:23Z

BE/src/main/java/com/airbnb/service/HouseService.java


 @Service
 public class HouseService {

+    private static final int DISTANCE = 1000;


단위가 어떻게 되나요?

1000미터 입니다!!

좀 더 단위를 나타내는 이름이 좋지 않을까요?

ksundong · 2022-05-31T14:32:42Z

BE/src/test/java/com/airbnb/repository/HouseRepositoryTest.java

+    @Test
+    void search_condition_jpql_test() {
+        // when
+        List<House> result = houseRepository.searchByCondition(nowPosition, 1000, 10, 100000);


각 파라미터를 변수명으로 추출해주면, 더 가독성이 좋을 것 같네요~

ksundong · 2022-05-31T14:33:12Z

BE/src/test/java/com/airbnb/repository/HouseRepositoryTest.java

+        House house0 = new House("house0", 85000, new DetailInfo(10, "oneRoom0", "방입니다", 4.8, 10),
+            GeometryUtils.toPoint(37.549, 126.928), host);
+
+        House house1 = new House("house1", 55000, new DetailInfo(10, "oneRoom1", "방입니다", 4.8, 10),
+            GeometryUtils.toPoint(37.5492, 126.9113), host);
+
+        House house2 = new House("house2", 55000, new DetailInfo(10, "oneRoom2", "방입니다", 4.8, 10),
+            GeometryUtils.toPoint(37.5492, 126.9113), host);
+
+        House house3 = new House("house3", 55000, new DetailInfo(10, "oneRoom3", "방입니다", 4.8, 10),
+            GeometryUtils.toPoint(37.5492, 126.9113), host);
+
+        House house4 = new House("house4", 85000, new DetailInfo(10, "oneRoom4", "방입니다", 4.8, 10),
+            GeometryUtils.toPoint(37.552469, 126.933644), host);
+
+        House house5 = new House("house5", 95000, new DetailInfo(10, "oneRoom5", "방입니다", 4.8, 10),
+            GeometryUtils.toPoint(37.552469, 126.933650), host);
+
+        House house6 = new House("house6", 95000, new DetailInfo(10, "oneRoom6", "방입니다", 4.8, 10),
+            GeometryUtils.toPoint(37.5493, 126.9199), host);
+


중복되는 코드는 Factory method를 이용해 제거하면 어떨까요?

ksundong · 2022-05-31T14:33:56Z

BE/src/test/java/com/airbnb/repository/HouseRepositoryTest.java

+@ExtendWith(SpringExtension.class)
+@DataJpaTest
+@AutoConfigureTestDatabase(replace = Replace.NONE)
+class HouseRepositoryTest {


테스트 잘 작성해주셨어요 💯

감사합니다!!!

zbqmgldjfh and others added 16 commits May 30, 2022 11:04

[feat]: Test deploy ver1 push

8a2477c

[feat]: Test deploy ver1 push 2

1e7add1

[feat]: Test deploy ver1 push 3

bb3ba79

[feat]: Test deploy ver1 push 4

2bfb7a8

[feat]: 가격별 집 개수 계산

9316978

[feat]: Health Check 로직 변경

3e8f887

[style]: Health Class 이름 변경

44c7ae6

[fix]: run_new_was.sh 에서 인자 순서의 변경

713d697

[refactor]: Native Query를 사용하던 부분을 JPQL로 변경

14c099f

[feat]: 환경 변수에 datasource 추가

6a6fb77

[feat]: 테스트 데이터용 data.sql 추가

b25366c

[chore]: database platform 변경

2676e73

[feat]: 숙소 개수 조회 쿼리에 범위정보 추가 및 order by 적용

642f00d

[feat]: 가격 그래프용 api 작성

854c663

[test]: 가격 그래프용 api test

31c7d2b

Merge pull request #18 from leejohy-0223/feat-16

d4e5b38

leejohy-0223 requested a review from ksundong May 31, 2022 08:02

leejohy-0223 assigned zbqmgldjfh and leejohy-0223 May 31, 2022

leejohy-0223 added the review-BE Improvements or additions to documentation label May 31, 2022

ksundong approved these changes May 31, 2022

View reviewed changes

ksundong merged commit 47dfe7a into codesquad-members-2022:team-06 May 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Team - 06][BE] 에어비앤비 프로젝트 2주차 1회차 PR #140

[Team - 06][BE] 에어비앤비 프로젝트 2주차 1회차 PR #140

leejohy-0223 commented May 31, 2022

ksundong left a comment

ksundong May 31, 2022

leejohy-0223 Jun 2, 2022

ksundong Jun 2, 2022

ksundong May 31, 2022

leejohy-0223 Jun 2, 2022

ksundong May 31, 2022

leejohy-0223 Jun 2, 2022

ksundong Jun 2, 2022

ksundong May 31, 2022

leejohy-0223 Jun 2, 2022

ksundong Jun 2, 2022

ksundong May 31, 2022

zbqmgldjfh Jun 2, 2022

ksundong Jun 2, 2022

ksundong May 31, 2022

leejohy-0223 Jun 2, 2022

ksundong Jun 2, 2022

ksundong May 31, 2022

zbqmgldjfh Jun 2, 2022

ksundong Jun 2, 2022

ksundong May 31, 2022

ksundong May 31, 2022

ksundong May 31, 2022

zbqmgldjfh Jun 2, 2022

		@@ -0,0 +1,30 @@
		package com.airbnb.repository.dto;

		public class HouseCount {

[Team - 06][BE] 에어비앤비 프로젝트 2주차 1회차 PR #140

[Team - 06][BE] 에어비앤비 프로젝트 2주차 1회차 PR #140

Conversation

leejohy-0223 commented May 31, 2022

안녕하세요 Dion!

OAuth 로그인을 구현하려고 하는데, 이전 프로젝트에서 리뷰해주셨던 내용에 제가 답변을 달지 못해서 다시 질문 드립니다(죄송합니다..😭)

ksundong left a comment

Choose a reason for hiding this comment

질문에 대한 답변

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment