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

[BE][Team17] Cooper,Robin 2차 PR 보냅니다 #61

Open
wants to merge 60 commits into
base: team17-final
Choose a base branch
from

Conversation

malaheaven
Copy link

안녕하세요 team17의 쿠퍼, 로빈입니다. 2차 PR 요청드립니다. 😄
배포 URL: http://ec2-15-164-123-251.ap-northeast-2.compute.amazonaws.com/

저희가 기존에 보냈던 PR 브랜치가 다른 조의 commit과 합쳐져 불가피하게 다른 PR브랜치를 생성하였습니다.
그래서 첫 번째 PR내용과 겹쳐지는 점 말씀드립니다. 😢

2주차 구현목록:

부족한 코드, 리뷰를 위해 시간을 내어주셔서 진심으로 감사를 드립니다 😄

pbg0205 and others added 30 commits April 20, 2021 16:45
- 스프링 부트와 mysql을 연동해라
- 반찬 item, image, best_category, dish_category 테이블을 생성해라
Feat: 스프링 데이터 jdbc으로 셋팅하라
- Item domain을 생성해라
- Image domain을 생성해라
- DishCategory domain을 생성해라
- BestCategory domain을 생성해라
- ItemRepository를 생성해라
- ImageRepository를 생성해라
- DishCategoryRepository를 생성해라
- BestCategoryRepository를 생성해라
- 변경된 image, item 컬럼에 따라 domain 변경
- best_category 및 dish_category 에 대한 insert구문 추가
Fix : DB 스키마 변경하라
- discription -> description으로 변수명을 변경하라.
- 리턴타입을 List로 한 findAll 메서드를 생성하라.
- 베스트 카테고리 전체 목록을 조회하라.

- 베스트 카테고리 세부 목록을 조회하라.
- ItemService를 추가하라.

- ImageService를 추가하라.

- BestCategoryService를 추가하라.
- itemId -> item_id로 변경하라.
style : ItemDto item 변수명을 변경하라.
- DishCategoryController 구현
- DishCategoryService 구현
- DishCategoryDto 구현
Feat: DishCategory 조회 기능을 구현해라
- 사용하지 않는 ItemService 제거

- 사용하지 않는 ImageService 제거
- 사용하지 않는 ImageService를 제거하라.
- 현재 단계에서 imageService의 큰 사용이 없어 조회기능을
imageRepository로 주입해서 조회하도록 변경하라.
- Java Naming Convention에 맞게 camel case로 변경하여라.

- 정확한 의미를 부여할 수 있도록 변수명을 구체적으로 작성하라.
- itemDetail : description 2500원 내용 삭제

- badge 빈 문자열 제거
- badge 베스트 요소 제거

- 배포 URL 완성
- URL 추가 : recommandedItemDto, ItemDto

- delivery-type 순서 변경
pbg0205 and others added 25 commits April 27, 2021 16:43
- 중복된 URL을 제거하라.

- recommanded -> recommended로 변경하라.
fix : 오탈자 수정 및 URL 추가 수정
style : 중복된 URL을 제거하라.
- 이벤트 특가, 론칭 특가 순서 변경
refactor : RequestBody 형태로 데이터를 전달받아라.
- best 카테고리
- dish 카테고리
- 이미지
- 아이템
- 재고부족
Feat: 커스텀 이셉션 핸들러를 이용하여 예외를 처리
feat : CORS 설정에 PUT METHOD를 추가하라.
chore : jackson databind dependency를 추가하라.
style : 코드에 개행과 정렬을 맞춰라.
@malaheaven malaheaven changed the title [BE] Team 17 Cooper,Robin 2차 PR 보냅니다 [BE][Team17] Cooper,Robin 2차 PR 보냅니다 Apr 30, 2021
@snowjang24 snowjang24 added the review-BE BE 리뷰 label May 1, 2021
@wheejuni wheejuni self-requested a review May 2, 2021 01:17
@wheejuni wheejuni self-assigned this May 2, 2021
Copy link

@wheejuni wheejuni left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다. 👍 전체적으로 잘 구현해 주셨습니다.
배포 스크립트 같은 경우에도.. 노션도 좋은 플랫폼이지만, GitHub Gist를 활용하시는 것을 추천해 드립니다.

코드에 간단한 피드백들 남겨 보았으니 확인해 주시면 좋겠어요. 수고하셨습니다. 💯


@GetMapping("/main")
public ResponseEntity<DishCategoryDto> getMainDishCategory() {
return new ResponseEntity(dishCategoryService.getDishCategoryDto(1L), HttpStatus.OK);
Copy link

Choose a reason for hiding this comment

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

1L, 2L, 3L 이 어떤 의미를 갖는 숫자인지 따로 관리되면 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

넵넵~! 어떤의미의 숫자인지ㅜ 코드 상으로 잘 보이지 않았네용 ㅜ

}

@PutMapping("/{itemId}/order")
public ResponseEntity orderItem(@PathVariable Long itemId, @RequestBody HashMap<String, Integer> body) {
Copy link

Choose a reason for hiding this comment

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

RequestBody 를 해시맵 스타일로 받기보다는 확실한 DTO 클래스로 받는 것이 더 안전할 거라고 생각됩니다.

Copy link
Author

Choose a reason for hiding this comment

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

넵넵 DTO 클래스로 받아야겠어요~! 리뷰 감사합니다

private String deliveryDescription = "(40,000원 이상 구매 시 무료)";

@JsonProperty("delivery_info")
private String deliveryInfo = "서울 경기 새벽배송 / 전국택배 (제주 및 도서산간 불가) [월 · 화 · 수 · 목 · 금 · 토] 수령 가능한 상품입니다.";
Copy link

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.

헉..! 그렇네요

private final int salePrice;

@JsonProperty("delivery_type")
private List<String> deliveryType = Arrays.asList(new String[]{"새벽배송", "전국택배"});
Copy link

Choose a reason for hiding this comment

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

꼭 새로운 배열을 선언할 필요는 없습니다. 자바독 참고

Suggested change
private List<String> deliveryType = Arrays.asList(new String[]{"새벽배송", "전국택배"});
private List<String> deliveryType = Arrays.asList("새벽배송", "전국택배);

Copy link
Author

Choose a reason for hiding this comment

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

오.. 감사합니다

@JsonPropertyOrder({"item_id", "title", "image", "price", "detail_url"})
public class RecommendedItemDto {

private static final String URL = "http://ec2-15-164-123-251.ap-northeast-2.compute.amazonaws.com:8080";
Copy link

Choose a reason for hiding this comment

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

Phase나 환경에 따라 URL 이 바뀔 염려는 없으려나요?

Copy link
Author

@malaheaven malaheaven May 3, 2021

Choose a reason for hiding this comment

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

json에 url도 포함시켜 보내주는 방식에 대해 어떠한 방식으로 구현하면 좋을지 고민하다가.. 이렇게 하드 코딩으로 작성하게 되었네요.. 이부분을 HttpServletRequest 을 사용해서 controller를 거쳐 url을 넣어주는 방식으로 변경하는게 좋을까요?ㅜ

this.image = image.getUrl();

this.price = entity.getNormalPrice().intValue();
this.detailUrl = URL + "/dish" + "/detail" + "/" + itemId;
Copy link

Choose a reason for hiding this comment

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

이 부분은 물론 FE와 협의를 하셨겠지만 호스트 네임은 FE에서 FE의 로직으로 결정되게 하는 편이 항상 안전하긴 합니다.
워낙 변수가 많은 부분이기도 해서...

Copy link
Author

Choose a reason for hiding this comment

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

그렇군요.. 이번에 리뷰를 통해 배웠습니다 이 부분이 고민이 들었었는데, 리뷰 감사합니다


public interface ItemRepository extends CrudRepository<Item, Long> {

List<Item> findAll();
Copy link

Choose a reason for hiding this comment

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

이것은 이미 CrudRepository 에 있어서, 재정의하실 필요는 없을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

넵넵 이부분 수정하겠습니다

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-BE BE 리뷰
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants