[4기 홍혁준] Mission 1, 3 PR 제출합니다 #268

hongdosan · 2023-07-26T17:57:14Z

📌 과제 설명

고객(Member) 엔티티를 생성하고 그에 따른 CRUD 기능 테스트 구현했습니다.
고객, 주문, 상품 사이의 연관관계를 설정해보았습니다.

👩‍💻 요구 사항과 구현 내용

자세한 명세서는 docs 파일에 있습니다.
Mission 1 : 고객(Member)의 CRUD를 구현한다.
Mission 3 : 연관관계를 설정해본다.

✅ PR 포인트 & 궁금한 점

현업에서의 mock Test에 대해 궁금합니다.
현업에서의 Controller Test에 대해 궁금합니다.

Shin-Jae-Yoon

혁준님 고생하셨습니다 ~! 사실 리뷰보단 질문의 장이 되어버린 것 같네요 ㅋ-ㅋ

Shin-Jae-Yoon · 2023-07-27T10:58:47Z

src/main/java/kr/co/springbootjpaweeklymission/global/config/JpaConfig.java

+@Configuration
+@EnableJpaAuditing
+public class JpaConfig {
+}


(질문) SpringbootJpaWeeklyMission에 @EnableJpaAuditing을 추가한 것이 아닌 따로 JpaConfig를 만드셨는데, 여기에는 어떤 설정들이 추가될 수 있나요 ??

QueryDSL 설정이 추후에 추가될 수 있다고 생각했습니다 ㅎㅎ

Shin-Jae-Yoon · 2023-07-27T11:06:57Z

src/main/java/kr/co/springbootjpaweeklymission/global/error/exception/DuplicateException.java

+
+@Getter
+public class DuplicateException extends RuntimeException {
+    private ErrorResult errorResult;


이 부분과 this.errorResult 부분이 sonarLint에서 밑줄이 나왔던 것 같은데
반드시 필요한 부분인가요 !?

나중에 ExceptionHandler 처리할 때, 꺼내서 확인해보는 상황이 필요할 때가 있었습니다!

Shin-Jae-Yoon · 2023-07-27T11:07:08Z

...in/java/kr/co/springbootjpaweeklymission/global/error/exception/EntityNotFoundException.java

+
+@Getter
+public class EntityNotFoundException extends RuntimeException {
+    private ErrorResult errorResult;


이 부분도 마찬가지입니당

Shin-Jae-Yoon · 2023-07-27T11:14:45Z

src/main/java/kr/co/springbootjpaweeklymission/global/error/model/ErrorResult.java

+import org.springframework.http.HttpStatus;
+
+@Getter
+@AllArgsConstructor(access = AccessLevel.PRIVATE)


(질문) 이 부분은 @requiredargsconstructor가 아닌 @AllArgsConstructor(access = AccessLevel.PRIVATE)로 하신 이유가 있을까요 ??

이유가 있다기보다! Required를 해도 모든 필드에 대해 생성자가 생길테니, 크게 생각하지 않은 부분입니다!

Shin-Jae-Yoon · 2023-07-27T11:17:20Z

src/main/java/kr/co/springbootjpaweeklymission/member/application/MemberService.java

+
+        return members.stream()
+                .map(MemberSimpleResponse::toDto)
+                .toList();


(복습) Stream.toList() vs Stream.collect(toList()) 에 대해서 복습 한번 하고 가면 좋을 것 같슴다 ~!

한 번 더 공부하라고... 리뷰까지 남겨주시는.. 흑흑

Shin-Jae-Yoon · 2023-07-27T11:18:40Z

src/main/java/kr/co/springbootjpaweeklymission/member/domain/repository/MemberRepository.java

+public interface MemberRepository extends JpaRepository<Member, Long> {
+    Optional<Member> findByEmail(String email);
+
+    boolean existsByEmail(String email);


(복습) existsBy~ 의 쿼리가 어떤식으로 날라가는지 복습해보면 좋을 것 같슴다 ~!

복습글 추천

Shin-Jae-Yoon · 2023-07-27T11:22:17Z

src/main/java/kr/co/springbootjpaweeklymission/orderitem/domain/entity/OrderItem.java

+@NoArgsConstructor(access = AccessLevel.PROTECTED)
+public class OrderItem {
+    @Id
+    @GeneratedValue(strategy = GenerationType.IDENTITY)


현재 MySQL을 사용하는 상황인지 궁금함니다

Shin-Jae-Yoon · 2023-07-27T11:26:21Z

src/main/java/kr/co/springbootjpaweeklymission/member/domain/model/Address.java

+    private Address(String street, String detail) {
+        this.street = street;
+        this.detail = detail;
+    }


이 부분은 저도 헷갈리는데, 파라미터 개수가 3개보다 작은 경우 Builder를 쓰지 않는 것이 낫다고는 하던데
가독성, 통일성을 챙기는게 나을지, 위 규칙을 지킬지 생각해보면 좋을 것 같아용

kmebin

혁준님 과제하시느라 고생많으셨습니다! 굿굿

kmebin · 2023-07-27T13:15:34Z

src/main/java/kr/co/springbootjpaweeklymission/global/common/BaseTimeEntity.java

+    @CreatedDate
+    @Column(name = "created_at", length = 20, nullable = false, updatable = false)
+    protected LocalDate createdAt;


LocalDate 타입에 length 조건을 추가한 이유가 있나요? 또 protected로 선언한 이유가 궁금합니다!

앗 length는 생각없이 넣었네요!
protected로 선언한 이유 : BaseTimeEntity는 상속해서 사용하기 때문에 protected로 선언해봤습니다!
희빈은 의견은 어떤가요??

kmebin · 2023-07-27T13:20:01Z

src/main/java/kr/co/springbootjpaweeklymission/global/common/BaseTimeEntity.java

+package kr.co.springbootjpaweeklymission.global.common;
+
+
+import jakarta.persistence.Column;


package와 import 사이 줄바꿈 1번 or 2번으로 컨벤션 통일하면 좋을 것 같아요!

kmebin · 2023-07-27T13:24:28Z

src/main/java/kr/co/springbootjpaweeklymission/global/error/model/ErrorResult.java

+    INVALID_PARAMETER("파라미터 값이 유효하지 않습니다.", HttpStatus.BAD_REQUEST),
+    ;


오 이런식으로 쓰는게 새로운 값 추가할 때 더 좋을 것 같네요! 굿굿

kmebin · 2023-07-27T13:34:29Z

src/main/java/kr/co/springbootjpaweeklymission/item/domain/entity/Item.java

+    @Id
+    @GeneratedValue(strategy = GenerationType.IDENTITY)
+    @Column(name = "item_id", length = 4, nullable = false, unique = true)
+    private Long itemId;


@Id가 충분히 pk로써 의미를 가지고 있고, 잘 통용되고 있다고 생각하는데 추가로 @Column 붙이는 이유는 더 명시적이고 통일성을 위함일까요?
추가로 여기서 Long 타입에 length 조건을 걸어준 이유가 있을까요?!

사실상 name 속성만 있어도 될 것 같네요 ㅎㅎ
@column을 붙인 이유는 희빈님 말 그대로 더 명시적이고 통일성을 위함입니다!

kmebin · 2023-07-27T13:36:05Z

src/main/java/kr/co/springbootjpaweeklymission/member/application/MemberService.java

+        isEmail(putRequest.getEmail());
+        isCellPhone(putRequest.getCellPhone());


메서드 네이밍만으로는 해당 이메일/번호의 중복을 체크하는 메서드인지 파악하기 어려운 것 같습니다

kmebin · 2023-07-27T13:57:23Z

src/main/java/kr/co/springbootjpaweeklymission/member/presentation/MemberController.java

+    @PostMapping
+    public ResponseEntity<Long> createMember(@RequestBody @Validated MemberPutRequest putRequest) {


메서드 파라미터 검증에 @Valid가 아닌 @Validated를 사용하신 이유가 있을까요?

예전이 스터디 했던 팀원분이 @validated가 더 좋다. 라는 말을 들었어서 사용했는데, 지금 다시 찾아보니, 딱히 좋은 점을 찾지 못하겠네요.
무작정 말만 듣고 적용했던 과거의 나에게 반성문을 쓰도록 하겠습니다 ㅠ

kmebin · 2023-07-27T14:07:09Z

src/test/java/kr/co/springbootjpaweeklymission/member/application/MemberServiceTest.java

+        assertThat(actual.getModifiedAt()).isEqualTo(LocalDate.now());
+    }


modifiedAt과 LocalDate.now()이 시간적 차이가 발생하는 경우는 없을까요?

LocalDateTime이 아닌 LocalDate이기 때문에, 괜찮다고 생각했습니다!
근데 만약 23시 59분에 테스트를 돌렸다면..? 이점을 고려해봐야겠네요 ㅠ

kmebin · 2023-07-27T14:13:37Z

src/test/java/kr/co/springbootjpaweeklymission/member/application/MemberServiceUnitTest.java

+    @Test
+    @CsvSource(value = {"", "[email protected]", "[email protected]"})


@CsvSource는 사용하지 않고 있는 것 같습니다!~

kmebin · 2023-07-27T14:18:06Z

src/test/java/kr/co/springbootjpaweeklymission/member/domain/entity/MemberUnitTest.java

+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+@DataJpaTest
+@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)


메서드에 @DisplayName을 적용하면 불필요한 부분이라고 생각하는데, 추가하신 이유가 있으실까요?!

실수입니다!

kmebin · 2023-07-27T14:22:11Z

...java/kr/co/springbootjpaweeklymission/member/domain/repository/MemberRepositoryUnitTest.java

+        assertThat(savedMember.getAddress().getStreet()).isEqualTo(member.getAddress().getStreet());
+        assertThat(savedMember.getAddress().getDetail()).isEqualTo(member.getAddress().getDetail());


Member 엔티티에 getStreet, getDetail 메서드를 구현하신걸로 기억합니다.!

아맞다 ㅋㅋ

parksey

혁준님 작성하시느라 고생하셨습니다!
이런 사소한 것 까지 꼼꼼히 하시는 모습 너무 보기 좋은 거 같습니다.

리뷰는 공통된 부분은 빼고 나머지 부분만 질문 드렸습니다.

코드 잘 봤어요!

parksey · 2023-07-27T15:15:50Z

...ain/java/kr/co/springbootjpaweeklymission/global/error/handler/NotValidExceptionHandler.java

+
+@Slf4j
+@RestControllerAdvice
+public class NotValidExceptionHandler {


저도 유사하게 exceptionhandler를 사용했었는데 하나의 클래스에서 한 것이 아닌 여러 클래스로 나눠서 처리한 이유가 궁금합니다!

사실 한 프로젝트당 하나의 ControllerAdvice만 관리하는 것이 좋은데, 객체를 분리하는게 습관이 되어서 고쳐보도록 하겠습니다 ㅠ

parksey · 2023-07-27T15:23:56Z

src/test/java/kr/co/springbootjpaweeklymission/member/application/MemberServiceUnitTest.java

+import static org.mockito.BDDMockito.given;
+
+@ExtendWith(MockitoExtension.class)
+class MemberServiceUnitTest {


UnitTest와 단위테스트를 나눈 기준과 이유가 어떤건가요?

mock을 너무 많이 사용하게 되는 부분은 통합으로 빼서 테스트를 돌렸습니다 ㅎㅎ

parksey · 2023-07-27T15:28:20Z

src/main/java/kr/co/springbootjpaweeklymission/order/domain/entity/Order.java

+@Entity
+@Table(name = "tbl_orders")
+@NoArgsConstructor(access = AccessLevel.PROTECTED)
+public class Order {


Order와 OrderItem은 어떤 기준으로 패키지를 분리하셨나요?
패키지 전체가 엔티티기준으로 분리된 건가요? 아님 도메인으로 분리하신건가요??

저는 둘다입니다. 처음에는 도메인 기준으로 분리를 했지만, orderItem을 어디에서 관리하는게 좋을 지 고민하다가, 도저히 고를 수가 없어서 따로 뺐습니다! 추후에 개발을 더 진행한다면 또 변경될 수 있는 부분이라고 생각합니다 ㅎ

ymkim97

과제 하느라 고생하셨습니다!!!
조금 늦게 달아서 다른 분들과 겹치거나 이미 잘 해주셔서 많지 않습니다 ㅠ.ㅠ

ymkim97 · 2023-07-27T15:55:15Z

build.gradle

+    runtimeOnly 'com.h2database:h2'
+
+    //MySQL
+    runtimeOnly 'com.mysql:mysql-connector-j'


Dependency를 이렇게 나누니까 정말 좋은거 같아요! 배워갑니다 >.<

ymkim97 · 2023-07-27T16:03:38Z

src/main/java/kr/co/springbootjpaweeklymission/item/domain/entity/Item.java

@@ -0,0 +1,17 @@
+package kr.co.springbootjpaweeklymission.item.domain.entity;
+
+import jakarta.persistence.*;


와일드 카드 import는 가급적 쓰지 않는 것이 좋다고 합니다!
아마 인텔리제이가 자동으로 이렇게 만들어줬을거고 저도 순간 잊어서 이렇게 했지만🥺...
세팅에서 예방할 수 있습니다!

오홍 넵~

ymkim97 · 2023-07-27T16:27:33Z

src/main/java/kr/co/springbootjpaweeklymission/order/domain/entity/Order.java

+@Table(name = "tbl_orders")
+@NoArgsConstructor(access = AccessLevel.PROTECTED)
+public class Order {
+    @Id


오 저도 Order와 OrderItem은 어떤 기준으로 패키지를 분리하셨는지 궁금합니다!
코드를 작성하면서 궁금했는데 Order, OderItem, Item과 같은 연관관계가 있으면 어떻게 패키지를 분리하는 것이 좋을까요?

OrderItem 은 사실 어느 곳에 들어가도 상관없을 것 같습니다! 하지만 저 같은 경우엔 둘 중 어디에 넣을 지 계속 고민하다가 분리를 했는데, 추후에 변경될 수도 있는 부분입니다 ㅎ 만약 넣는다면 책임이 더 많은 order 패키지에 넣을 것 같네요 ㅎ

hongdosan added 18 commits July 24, 2023 22:09

init : Project 초기화

a184c3a

test : 유저 회원 엔티티 생성 테스트

c0eef79

docs : Member 명세서 작성

796bd16

test : Member Repository save 테스트

5d361c1

test : Member Email 유효성 검증 테스트

6c92946

test : Member Cell Phone 유효성 검증 테스트

55b744b

test : Member Address가 잘 생성되는 지 검증

c2eb1c0

test : Member Service createMember Test

87e7891

test : Member Controller 등록 API Test

b95fcb8

refactor : Member Test DisplayName 설정

629942a

test : Member Auditing CreateAt Test

305bac1

refactor : Member Test Method Name

d6195b4

test : email에 대한 회원을 조회하는 기능 테스트 및 구현

1655256

test : member 전체 조회 테스트

0fa4e3a

refactor : 테스트 DisplayName 변경

d241dec

test : 회원 삭제 기능 테스트 및 구현

c5f21c1

test : 회원 수정 시 수정일 검증 테스트 추가

ff17b25

feat : 연관관계 설정

ae1cda5

hongdosan added 🔥 매운맛 Team-Joseph labels Jul 26, 2023

hongdosan requested review from parksey, kmebin, ymkim97 and Shin-Jae-Yoon July 26, 2023 17:57

hongdosan self-assigned this Jul 26, 2023

fix : @transactional 추가

505dffd

Shin-Jae-Yoon reviewed Jul 27, 2023

View reviewed changes

kmebin reviewed Jul 27, 2023

View reviewed changes

parksey reviewed Jul 27, 2023

View reviewed changes

ymkim97 reviewed Jul 27, 2023

View reviewed changes

		package kr.co.springbootjpaweeklymission.global.common;


		import jakarta.persistence.Column;

		INVALID_PARAMETER("파라미터 값이 유효하지 않습니다.", HttpStatus.BAD_REQUEST),
		;

		isEmail(putRequest.getEmail());
		isCellPhone(putRequest.getCellPhone());

		@PostMapping
		public ResponseEntity<Long> createMember(@RequestBody @Validated MemberPutRequest putRequest) {

		assertThat(actual.getModifiedAt()).isEqualTo(LocalDate.now());
		}

		@Test
		@CsvSource(value = {"", "[email protected]", "[email protected]"})

		assertThat(savedMember.getAddress().getStreet()).isEqualTo(member.getAddress().getStreet());
		assertThat(savedMember.getAddress().getDetail()).isEqualTo(member.getAddress().getDetail());

		@@ -0,0 +1,17 @@
		package kr.co.springbootjpaweeklymission.item.domain.entity;

		import jakarta.persistence.*;

[4기 홍혁준] Mission 1, 3 PR 제출합니다 #268

Are you sure you want to change the base?

[4기 홍혁준] Mission 1, 3 PR 제출합니다 #268

Conversation

hongdosan commented Jul 26, 2023

📌 과제 설명

👩‍💻 요구 사항과 구현 내용

✅ PR 포인트 & 궁금한 점

Shin-Jae-Yoon 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

kmebin 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

kmebin Jul 27, 2023 • edited Loading

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

kmebin Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmebin Jul 27, 2023 • edited Loading

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

parksey left a comment • edited Loading

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

ymkim97 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

kmebin Jul 27, 2023 •

edited

Loading

kmebin Jul 27, 2023 •

edited

Loading

kmebin Jul 27, 2023 •

edited

Loading

parksey left a comment •

edited

Loading