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

[Team-10] [BE] [마르코&비비] PR 요청드립니다. #65

Open
wants to merge 78 commits into
base: dahun-lee-daji
Choose a base branch
from

Conversation

95degree
Copy link

안녕하세요, 11팀 웹 백엔드 팀원 Bibi, Marco 입니다.
두 번째 프로젝트인 '온라인 반찬 서비스' 를 완성해 Pull Request를 보냅니다. 감사합니다. 😀


중점을 두고 한 부분들

  • API를 위한 Raw data 편집
  • SpringDataJDBC로 두 테이블을 1:N 관계로 연결하기
  • iOS 측에서 필요로 하는 형태의 API 제작
  • 빠른 API 배포

더 신경썼더라면 좋았을 부분들

  • 한 테이블에 너무 많은 정보를 넣어 완성한 점. 좋은 설계에 대한 지식이 부족해, 깊이 생각하지 않고 설계해 테이블 관계 맵핑을 별로 경험하지 못한 점
  • OAuth를 구현하지 못한 점..

real Erd


위의 ERD처럼 설계를 하고 싶었으나 구현 당시에는 저렇게 진행하면 도저히 프로젝트를 제 시간안에 못할꺼 같다고 판단되어
아래의 스키마로 진행했습니다.

스키마를 보시면 price나 point처럼 int로 되있어야 할 부분들이 varchar로 되어 있습니다.
저희가 위키에 데이터 형태 관련 문서를 꼼꼼히 작성하지 않아 iOS부분에 곤란한 상황이 발생하게 되어 varchar로 진행했습니다.
이 부분 양해부탁드립니다.
그래도 현재는 프로젝트를 진행하면서 공부를 한 덕분인지 다음에는 충분히 구현 할 수 있을꺼 같습니다. 😊😊


create table category (
    id int not null auto_increment,
    name varchar(45) not null,
    type varchar(10) not null,
    primary key (id)
);

create table dish (
    id varchar(5) not null,
    main_image varchar(100) not null,
    alt varchar(45) not null,
    delivery_type varchar(45) not null,
    title varchar(45) not null,
    description varchar(45) not null,
    normal_price varchar(45) not null,
    selling_price varchar(45),
    badge varchar(45),
    category_id int not null,
    top_image varchar(200) not null,
    thumb_images varchar(1000) not null,
    delivery_info varchar(1000) not null,
    delivery_fee varchar(45) not null,
    detail_section varchar(1000) not null,
    stock int not null,
    point varchar(5) not null,
    primary key (id),
    foreign key (category_id) references category (id)
);

질문

  • 어떤 오류가 발견되어 백엔드에서는 조금 고치면 되지만 좋지 않은 설계가 되고 다른 파트에서는 구조 전체를 바꿔야 합니다.
    이런 경우에는 어떻게 수정을 진행해야 할까요??
  • 현재 조회를 하면 [nio-8080-exec-1] o.s.d.j.core.convert.ResultSetAccessor : ResultSet contains id multiple times 경고 메세지
    가 콘솔에 계속 찍힙니다. 아직 정확한 원인을 찾지 못했습니다. 왜 이런 경고가 생기는 걸까요??
  • DB연결 설정을 위해 application.properties 파일에 DB 사용자 이름과 비밀번호를 입력해야 해서, 어쩔 수 없이 커밋&푸시 하며 정보가 노출되게 되었습니다. DB ID,PW가 노출되지 않으면서도 DB연결은 잘 할 수 있게 하려면 어떻게 관리해야 하나요?:생각하는_얼굴:

dahun-lee-daji and others added 30 commits April 19, 2021 15:21
- db schema : sidedish
각 response에 맞는 형태로 보내주기 위해 Dto 생성

- CategoryResponseDto
- DishResponseDto
- DishDetailResponseDto
음식을 시켰을 때 쌓이는 point 컬럼 추가
패키지 이름 생성 규칙을 지키도록 변경
음식 정보를 id로 쉽게 찾을 수있게 Map으로 변경
이전의 여러 price값이 int 였는데 iOS 파트와 맞추기 위해 String으로 변경
로그 확인을 위한 toString() 추가
Response에 넣어서 보내줄 필요가 없기 때문에 categoryId 필드를 삭제 했습니다.
95degree and others added 26 commits April 28, 2021 17:45
- Dish에 checkStock(),updateStock() 추가
- controller에 OrderDish() 추가
주문을 했을 때 수량을 체크하고 stock 변경
# Conflicts:
#	BE/src/main/resources/schema.sql
inner join쿼리를 통해 카테고리를 찾는다.
dish에 필요한 image들을 모아서 객체로 관리하기 위해 embedded entity 생성
@MJbae MJbae added the review-BE BE 리뷰 label Apr 30, 2021
@wheejuni wheejuni self-assigned this May 2, 2021
@wheejuni wheejuni self-requested a review May 2, 2021 01:17
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.

ID/PW 정보가 말씀처럼 노출되었군요. 언제 한 번 접속해 봐야겠어요. 👍
노출되지 않으려면 어떻게 하는 것이 좋을까요? 기본적으로 코드가 아닌 곳에 접속 정보가 보관되면 좋은데요, 아래와 같은 방법들이 있습니다.

  • 호스트 서버(EC2) 의 파일로 보관
  • 호스트 서버(EC2) 의 환경 변수
  • Spring Vault
  • AWS EC2 Metadata

맨 아래 두 가지 방법은 관리해야 할 서버의 댓수가 좀 늘어나고, 규모있는 웹 앱을 구축할때 사용할 방법이고요,
가장 위의 두 방법 중 하나를 검토해보면 좋은데, 간단하게 환경 변수로 구현하시면 좋을 것 같아요.
Spring 기반 애플리케이션에서 호스트 서버의 환경 변수를 갖고 오는 방법에 대한 가이드는 인터넷에 많이 있으니 검색해 보시기를 추천합니다.

이외에도 코드에 대한 피드백을 조금 남겨 보았습니다. 전체적인 방향은 나쁘지 않고요, 피드백은 참고만 해 주세요.
수고 많으셨습니다. 💯


@PutMapping("/{dishId}")
public ResponseEntity<ResponseDto> orderDish(@PathVariable String dishId, @RequestParam("count") int orderSize) {
if (dishService.orderDish(dishId, orderSize)) {
Copy link

Choose a reason for hiding this comment

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

dishService.orderDish() 의 리턴 타입이 Status 라면 어땠을까요?
코드가 조금 더 간단해 지지 않으려나요?

private Dish() {
}

public Dish(String id, String alt, String deliveryType, String title, String description, String normalPrice,
Copy link

Choose a reason for hiding this comment

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

생성자가 굉장히 복잡하게 구현되게 되었네요.
아마 이 엔티티가 좀 비대해지다보니 그렇게 된 것 같은데요, 생성자가 복잡한 것 그 자체가 문제는 아니지만, 문제는 String 타입의 파라메터가 계속 반복되면서 순서가 뒤바뀌어도 타입으로 유효성을 검증할 수 없다는 점입니다.
이런 경우에 Builder 패턴의 도입을 적극적으로 검토해야 합니다.
빌더 패턴을 어떻게 구현할 수 있을지 구글 검색 등으로 일단 참고해 보시고, 적용을 검토해 보시면 좋겠어요.

package com.codesquad.sidedish.domain;

public class Image {

Copy link

Choose a reason for hiding this comment

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

이미지가 어떤 상품에 종속돼야 하는지, FK에 해당하는 필드가 있어야 할 거라고 생각했는데 없네요.

Comment on lines +5 to +7
private String mainImage;
private String topImage;
private String thumbImages;
Copy link

Choose a reason for hiding this comment

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

하나의 Image 엔티티가 이 정보를 다 갖고 있어야 하나요? 그렇다면 클래스 이름이 조금 바뀌어야겠네요.
제가 생각하기에 Image 클래스는 아래의 정보를 각각 갖고 있으면서 독립된 이미지 URL 하나만 관리해야 할 것 같거든요.

  • 매핑된 상품
  • 이미지의 분류(메인인지, 탑인지, 미리보기인지...)
  • URL

상품의 PK로 Image 테이블을 뒤진 다음에, 해당하는 모든 객체를 불러오면,
그 다음부턴 이미지의 분류를 관리하는 필드를 통해 적절히 분류해서 DTO로 내려주면 될 것 같거든요.

return dishes;
}

public static CategoryResponseDto of(Category category) {
Copy link

Choose a reason for hiding this comment

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

좋은 구현입니다. 👍

import org.springframework.http.HttpStatus;
import org.springframework.web.bind.annotation.ResponseStatus;

@ResponseStatus(value = HttpStatus.NOT_FOUND)
Copy link

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.

5 participants