Skip to content

[4기 원건희] Springboot-jpa weekly 미션 1차 PR입니다. #305

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

Open
wants to merge 67 commits into
base: weonest
Choose a base branch
from

Conversation

weonest
Copy link

@weonest weonest commented Jul 30, 2023

📌 과제 설명

  • 미션1 (단일 엔티티 (Customer) 를 이용한 CRUD 구현)
  • 미션3 (연관관계매핑 (order, order_product, product의 연관관계 매핑 실습)

👩‍💻 구현 내용 & 구현 예정

  • 페어 프로그래밍을 진행하면서 상대적으로 보완해야할 점이 많다고 생각하여 단순히 연관관계 매핑을 실습하는 것 보다는 주어진 entity들을 활용하여 기초적인 스프링 프로젝트를 만들어보는 쪽으로 진행할 계획입니다.

  • CRUD 중 모든 엔티티의 Create 부분의 구현과 테스트를 완료하였습니다.

  • 도메인 로직들을 구현할 예정입니다. (주문 상태 변경, 유저 정보 변경 등)

  • C를 제외한 RUD를 구현할 예정입니다.

  • RestController를 구현할 예정입니다.

Copy link
Member

@dojinyou dojinyou left a comment

Choose a reason for hiding this comment

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

코멘트 확인 부탁 드립니다!

Copy link
Member

@dojinyou dojinyou left a comment

Choose a reason for hiding this comment

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

코멘트 확인 부탁 드립니다!

Copy link

@seung-hun-h seung-hun-h left a comment

Choose a reason for hiding this comment

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

건희님 수고하셨습니다. 리뷰 남겼으니 확인해주세요

pom.xml Outdated

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 27 to 33
public void changeFirstName(String firstName) {
this.firstName = firstName;
}

public void changeLastName(String lastName) {
this.lastName = lastName;
}

Choose a reason for hiding this comment

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

이렇게 되면 setter와 크게 다르지 않은 메서드인 것 같아요.
변경 되는 메서드를 하나로 만들어줘도 괜찮을 것 같습니다

Copy link
Author

Choose a reason for hiding this comment

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

2acc0c9
다음과 같이 변경하였습니다!

Comment on lines 26 to 36
@GetMapping
public ResponseEntity<List<CustomerResponse>> customerList() {
List<CustomerResponse> customers = customerService.findCustomers();
return ResponseEntity.ok(customers);
}

@GetMapping("/page")
public ResponseEntity<Page<CustomerResponse>> customerPage(@PageableDefault() Pageable pageable) {
Page<CustomerResponse> customers = customerService.findCustomersWithPaging(pageable);
return ResponseEntity.ok(customers);
}

Choose a reason for hiding this comment

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

이 부분은 페이지네이션을 사용하는 api로 통일하는 것이 좋겠네요

Copy link
Author

Choose a reason for hiding this comment

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

34399ea
반영하였습니다!

Choose a reason for hiding this comment

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

validation은 없어도 되나요??

this.orderProductRepository = orderProductRepository;
}

public Long createOrder(OrderCreateRequest request) {

Choose a reason for hiding this comment

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

readOnly라서 생성이 안될 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

531b0ae
반영하였습니다!

private UserService userService;

@Test
void order_create_test() {

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.

c00a0f1
반영하였습니다!

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.

3db114a
반영하였습니다!

weonest added 30 commits August 3, 2023 20:31
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