Skip to content
This repository has been archived by the owner on Aug 13, 2022. It is now read-only.

[#66],[#65] 식당insert기능, 로그인체크기능 추가 #68

Merged
merged 10 commits into from
Feb 9, 2022

Conversation

ypr821
Copy link
Collaborator

@ypr821 ypr821 commented Jan 27, 2022

구현내용 요약

리뷰 포인트

  • AOP 테스트 코드 작성에 피드백이 필요합니다.
  • controller에 요청이 들어올때 Request body의 String 데이터를 enum클래스로 바로 매핑할 수 있는 방법을 고민중입니다.

체크 포인트

  • annotation 설명 주석 달기
  • 코드 컨벤션 지키기
  • 과도한 축약용어 사용하지 않기
  • 객체지향적인 코드 작성

build.gradle Outdated
@@ -34,6 +34,7 @@ jacoco {
dependencies {
/* spring-boot-starter-web : Spring MVC 를 사용한 restful 서비스를 개발하는데 사용합니다. */
implementation 'org.springframework.boot:spring-boot-starter-web'
implementation 'org.springframework.boot:spring-boot-starter-aop:2.6.2'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

피드백 감사합니다 연재님 수정하겠습니다 ㅎㅎ

Copy link
Collaborator

@yeoonjae yeoonjae left a comment

Choose a reason for hiding this comment

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

정말 고생많으셨습니다 !! 👍

AND CATEGORY = #{category}
AND OPEN_YN = #{openYN}
AND RESTAURANT_NAME = #{restaurantName}
AND INTRODUCTION = #{introduction}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ypr821
조건문에 INTRODUCTION 가 들어있으면 전체 문자열을 비교하는 것이니 성능상 안좋지 않을까 생각이 듭니다.

Copy link
Collaborator Author

@ypr821 ypr821 Jan 28, 2022

Choose a reason for hiding this comment

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

@yeoonjae 넵! 생각해보니 INTRODUCTION을 꼭 비교하지 않아도 될 거 같네요!
아 연재님 MINIMUM_ORDER_AMOUNT도 뺄가 하는데 어떻게 생각하세요??
select 도중에 주문 수가 늘어날 경우 fail 날 수도 있지 않을 까 하는 의문이 들었습니다

this.category = category;
}

@JsonCreator
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ypr821
푸름님 질문이 있습니다. 여기서 @JsonCreator@JsonProperty("category")을 사용하신 이유가 있으신가요 ? ENUM 클래스에서만 사용하셔서 다른 이유가 있는지 궁금합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yeoonjae
Json body로 들어오는 값을 enum 클래스로 매핑할 수 있는 방법을 고민하고 있습니다.!! 이부분은 제가 좀 더 공부하고 수정해서 다시 공유드려도될까요?? 아직 고민중인코드인데 같이 올라갔네요 !!

@ypr821 ypr821 requested a review from f-lab-jd January 28, 2022 10:53
Copy link
Collaborator

@f-lab-jd f-lab-jd left a comment

Choose a reason for hiding this comment

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

@ypr821 고생하셨습니다~ 이해가 안되시는 리뷰내용이 있다면 말씀해주세요!


Restaurant selectRestaurantByRestaurantSeq(Long restaurantSeq);

Long selectRestaurantSeq(Restaurant restaurant);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ypr821 restaurantSeq 를 restaurant 객체로 조회는 어떤 경우하나요?

Copy link
Collaborator Author

@ypr821 ypr821 Feb 6, 2022

Choose a reason for hiding this comment

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

@f-lab-jd
selectRestaurantSeq 메서드는 테스트를 위해서 만든 메소드입니다.
addRestaurantSuccessTest 에서 Service의 addRestaurant() 메소드 실행후 저장을 확인하기 위해서 만들었습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ypr821
테스트시에 꼭 필요한 메소드인가요? 다른 방법은 없을까요~?
실제 비즈니스로직에 사용되지 않고, 테스트에서만 사용되는 메소드를 만드는것은 지양하는것이 좋습니다.

Copy link
Collaborator Author

@ypr821 ypr821 Feb 9, 2022

Choose a reason for hiding this comment

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

@f-lab-jd
넵 멘토님 !! 지양하겠습니다.

멘토님 Read 기능에 사용할 메서드를 생성하고 테스트에 사용하는 방법은 어떨까요??
아니면 Response DTO를 생성해서 입력받은 값을 리턴하는 것도 방법일 거같습니다.

그리고 insert한 값을 test하는 방법으로 select하여 값을 비교하는 방법 말고 다른 방법이 있나요???

private Long ownerSeq;

@NotNull
private String category;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ypr821 category 는 enum 으로 표현할 수 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@f-lab-jd
제가 요부분이 고민이었습니다
클라이언트 요청 body부분에서 바로 enum 클래스 타입으로 매핑해서 받아 올 수 있을 거 같습니다.
대신 변수명이 조금이라도 틀리면 에러가 발생하는 걸로 알고 있습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ypr821 넵 맞습니다. 말씀하신 내용이 enum 을 사용할때 얻을 수 있는 장점이기도 합니다 ㅎㅎ

@@ -21,4 +21,6 @@
int deleteAddress(Long addressSeq);

boolean isExistsAddress(Long addressSeq);

Long selectAddressSeq(Address address);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ypr821
RestaurantMapper 코드에 드렸던 질문과 같은 내용입니다.

Copy link
Collaborator Author

@ypr821 ypr821 Feb 9, 2022

Choose a reason for hiding this comment

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

@f-lab-jd selectAddressSeq 메소드는 RestaurantService의 addRestaurant메소드에서 사용하고 있습니다. useGeneratedKeys 설정을 사용하여 수정할때 없애도록 하겠습니다.

Address address = addRestaurantRequest.getRestaurantAddressRequest().toEntity();
try {
userAddressMapper.insertAddress(address);
Long addressSeq = userAddressMapper.selectAddressSeq(address);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ypr821 useGeneratedKeys 설정을 하였는데 select 문이 꼭 필요한가요?

Copy link
Collaborator Author

@ypr821 ypr821 Feb 7, 2022

Choose a reason for hiding this comment

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

@f-lab-jd
네 필요합니다 ! ADDRESS 테이블에 insert를 먼저하고 입력한 주소의 seq 값을 가지고와서
RESTAURANT테이블에 insert할때 ADDRESS_SEQ 컬럼의 값으로 담아주기 위해서는 필요하다고 생각합니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ypr821 insertAddress 에 매핑된 mybatis 설정에 useGeneratedKeys 설정이 있어서 seq 값을 가져올 수 있지 않나요~?

Copy link
Collaborator Author

@ypr821 ypr821 Feb 9, 2022

Choose a reason for hiding this comment

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

아하 적용했습니다. 감사합니다.ㅎㅎㅎㅎ

Copy link
Collaborator

@f-lab-jd f-lab-jd left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 👍

@ypr821 ypr821 merged commit 8aa4382 into develop Feb 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants