-
Notifications
You must be signed in to change notification settings - Fork 50
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] [팀-18 / Kyu] 리뷰 요청 #51
base: team18
Are you sure you want to change the base?
Conversation
[BE] Add/entity
- Github Oauth 연동
- detail_hash를 id로 설정
[BE] Add/repo
[BE] Add/service
- 구조 변경으로 필요없음
1. 배열 반환을 위해 Entity는 디비와 1:1매칭이 된다. 배열 반환을 위해서 타입을 배열로 주면 MySQL에서 데이터타입으로 배열로 설정할수 없기때문에 데이터를 집어넣을수없다. 2. 디비는 Entity로, 나머지는 DTO로 데이터를 통신.
[BE] Feat/jsonarray
- 컬럼이 Null일 경우 데이터 대처
- "/api" 추가
@kyu-kim-kr DTO에 대한 정성담긴 정리글 잘 읽었습니다. 저도 비슷한 생각 갖고 있습니다. 우리가 작성하는 서버 애플리케이션을 너무 세분화해서 보진 마시고, 프레젠테이션 레이어/애플리케이션 레이어 정도로 분리해서 보면 DTO/Entity 사이의 관계에서도 중심을 잡으실 수 있을 겁니다. 중요한건 사실 어디서 무얼 쓰냐보다 Entity와 DTO가 서로를 완전히 몰라도 자기 기능을 하는 일 이 가장 중요합니다. 글이 너저분하기 길어졌는데 지금 DTO 사용에 대해서 원론적인 차원에서 Kyu가 갖고 있는 생각과 제 생각이 비슷합니다. |
@kyu-kim-kr 아 그리고 디온이 휴가셔서 제가 대신 리뷰하겠습니다. 양해를 구합니다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고 많으셨습니다. 👍 구현에만 초점을 맞춘 것은 이해하지만, 만드시기 전에 내가 구현하고자 하는 방향에 대한 표준, 혹은 컨벤션이 있진 않을까 를 염두에 두시면 더욱 더 좋은 개발자로 성장하실 수 있을 거라 생각됩니다.
코드에 대해 의견 조금 남겨 보았는데요, 아무래도 조별 프로젝트를 진행하는 일정상 반영이 쉽지 않을 수는 있을 것 같습니다.
그렇지만 우선 참고해주세요. 감사합니다.
import org.springframework.security.web.authentication.HttpStatusEntryPoint; | ||
|
||
@SpringBootApplication | ||
public class SidedishApplication extends WebSecurityConfigurerAdapter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security 설정이 별도 클래스로 독립하면 조금 더 좋을 것 같네요.
} | ||
|
||
@GetMapping("/soup/{detail_hash}") | ||
public BanchanDto findOneSoup(@PathVariable String detail_hash) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PathVariable
안에 path variable name을 설정할 수 있는 필드가 있을 거라 생각합니다.
어떠한 순간에도 자바의 기본적 코딩 컨벤션을 계속 준수해주세요.
public static final int OK = 200; | ||
public static final int BAD_REQUEST = 400; | ||
public static final int NOT_FOUND = 404; | ||
public static final int INTERNAL_SERVER_ERROR = 500; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enum
으로 관리해보시면 어떨까요?
private Set<String> delivery_type; | ||
private String title; | ||
private String description; | ||
private String n_price; | ||
private String s_price; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JsonProperty
를 통해 JSON field의 이름 정해주시면 될 것 같아요.
언더스코어는 자바 코드에 없었으면 합니다.
|
||
} | ||
|
||
private BanchanDto(Banchan banchan) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private
으로 가려놨으므로 모든 필드에 해당하는 파라메터를 다 선언해주셔도 됩니다.
또 그렇게 해야 추후 확장성 측면에서도 좋을 거라 생각합니다.
Banchan
을 받아서 하나씩 정보를 꺼내는 책임은 .of()
에게 맡기고 생성자에선 단순 값 할당에 집중하면 어떨까요.
import java.util.HashSet; | ||
import java.util.Set; | ||
|
||
public class StringConvertor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 링크 를 참고해주시고 표준 스펙에 맞는 클래스로 전환을 부탁드릴게요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
데이터 변환을 위한 컨버터에 대한 표준 스펙이 있었군요..스프링 문서 전체적으로 꼭 한번 읽어봐야겠네요.
감사합니다.
|
||
private String github_id; | ||
private String detail_hash; | ||
private int qty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
굳이 약어를 사용하실 필요는 없어 보이기는 합니다.
테이블 상에도 이 이름으로 칼럼이 들어가있나요? 그렇다면 풀 네임인 quantity
로의 전환을 제안드립니다.
for (Category category : categoryList) { | ||
setOfCategoryIds.add(category.getCategory_id()); | ||
} | ||
System.out.println(setOfCategoryIds); //테스트 // null도 들어가나? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger
사용해주세요.
for (String category_id : setOfCategoryIds) { | ||
categoryDtoList.add(findBestBanchansByCategory(category_id)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.stream().map()
사용으로 앞선 for loop이랑 여기까지 없앨 수 있어 보입니다.
for (int i = 0; i < banchanList.size(); i++) { | ||
banchanDtoList.add(BanchanDto.of(banchanList.get(i))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 .stream().map()
으로 바꿀 수 있습니다.
@wheejuni
다음 프로젝트 때 한번 이렇게 신경써보겠습니다. |
@Embedded.Nullable | ||
private BanchanDetail banchanDetail; | ||
|
||
public Banchan(Long id, String detail_hash, String image, String alt, String delivery_type, String title, String description, String n_price, String s_price, String badge, String tag, String category_id, BanchanDetail banchanDetail) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다른 리뷰에서 이런 류의 생성자를 굉장히 싫어하신다고 했었는데 여기도 똑같은 부분 맞나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[BE] Feat/order
fix : 매핑정보
* Feat: Tabs 구현 * Feat: Model 레이아웃 상단 구성 * Feat: Model setImgSources추가 * add: merge * fix: merge * fix: carousel animation render fix * Test: 목업데이터로 요청해보기 setupProxy * Feat: 썸네일 클릭시 이미지 변경 * fix: 캐로셀 코드 개선 애니메이션 및 함수 분리 * Fix: 클릭시 모달창 띄우기 * add: 캐로셀 이벤트 모달 및 애니메이션 추가 * fix: 캐로셀 리팩토링 * fix: 필요없는 함수 삭제 * add: 캐로셀 커스텀 훅, upper 네비게이터, 디폴트 네비게이터 분리 * fix: 캐로셀 컨테이너 리팩토링 * Feat: 에러 이미지 처리 * fix: upper navigator를 위한 padding-top 처리 * fix: 캐로셀 모달 이벤트 꼼수 수정 * fix: useCarousel 커스텀훅 clean 함수 수정 * fix: 강제지정된 높이값 수정 * add: 메인디시 캐로셀 테스트를 위한 목업 작업 * fix: 모달 바텀 테스트 통한 작업 추가, BottomSide 조건부 렌더링 추가를 통한 리렌더링 처리 -> 향후 수정 예정 * fix: 리사이즈 디바운싱 처리 * fix: useCarousel hotfix * fix: 케로셀 width height 계산로직 에러 수정 * Fix: Hearder, Tabs 반응형으로 수정 * fix: fe_dev refactor project #48 import strategy change * Fix: 상대경로 절대경로로 수정하기 fix #48 * Fix: Card Tabs분기처리 fix #51 * Fix: error 처리 useReducer로 수정 fix #50 * Fix: key를 unique하게 수정 fix #50 * Feat: useAsync 함수 구현 fix #50 * add: 전체보기 버튼 추가 * fix: 탭 상단 라벨버튼 퍼블리싱 마진값 관련 개선 * fix: 캐로셀 offset 관련 에러 및 초기 렌더 깨지는 문제 해결 * fix: 버튼 컴포넌트 엔트리 변경 * fix: 팀 API 활용을 위한 엔드포인트 처리, 카테고리 불러오기 로직 초기작업 * Fix: useAsync 수정 * Fix: api수정하여 데이터 받는부분(변수명, split(', ') 등..) 수정 * Feat: 프론트엔드 빌드 파일 추가 * fix: Dish 관리 변경 * fix: z-index 처리 * add: 랜덤 불러오기 함수 추가 * fix: 모달 refetch관련 수정 * fix: modal refetch관련 카드 수정 * Fix: api수정되어 데이터 포맷 수정 * Fix: 모달 Bottom 간격수정, console 제거 * fix: z-index hotfix Co-authored-by: jeonyeonkyu <[email protected]> Co-authored-by: jeonyeonkyu <[email protected]> Co-authored-by: Jaeuk-YOO <[email protected]> Co-authored-by: 빰빰맨 <[email protected]>
{: width="50%" height="50%"}
배포 링크 : http://ec2-15-164-169-189.ap-northeast-2.compute.amazonaws.com
안녕하세요.
저는 구현에만 초점을 두고 코드를 작성했습니다.
언급하고 싶은게 있다면 DTO에 관한 이야기인 것 같습니다.
처음에 Entity/DTO 에 관한 개념을 몰랐습니다.
이걸 찾아보게 된 계기는 JSON 데이터에 있는 배열에서 비롯했습니다.
이걸 어떻게 해야하나 싶었습니다.
왜냐하면 테이블 컬럼의 데이터타입으로 배열을 넣을수가 없었기 때문입니다.
어쨋든 데이터는 넣어야했기 때문에 String 타입으로 테이블과 Entity를 매칭 시켜줬습니다.
그렇기 때문에 Entity 클래스에 배열을 반환해줘야 할 필드의 타입이 String이 들어갔습니다.
해결법은 컨트롤러에서 Entity 객체를 응답하는게 아니라, Entity와 기본적으로 비슷하지만 배열같은 것을 내보낼수 있도록 할 수 있는 다른 무언가를 응답하는 거였습니다.
그게 저한텐 DTO 였습니다.
DTO를 사용했던 이유는 그게 가장 컸지만 조금 더 내용을 학습해보았습니다.
찾아보니 DTO 와 Entity 를 분리해서 사용하는 이유를 읽어 보게 됐습니다.
여러 이유가 있었는데 전체적으로 이해하기 좀 힘들었지만 View Layer와 DB Layer를 철저히 나누기 위해서 그 두개를 분리해서 사용한다는 이유가 그나마 쉽게 넘겨 이해하기 좋았습니다.
출처 : https://gmlwjd9405.github.io/2018/12/25/difference-dao-dto-entity.html
그리고 위에 이미지를 보고 전체적으로 DB, 클라이언트랑 통신할 때 DTO와 Entity를 분리하기로 결정했었습니다. 더불어서 가장 큰 이유는 아까 언급했듯이 배열을 표현하려면 DTO에 다시 담아서 보내야한다고 생각했기 때문이기도 하죠.
어떻게 다시 담아서 보내냐고 물어보신다면 문자열을 특정문자(쉼표 + 띄어쓰기)로 구분해서 데이터를 담아주고 꺼내쓸때 split()을 사용해서 List에 다시 담는 방식으로 했습니다.
관련 커밋 : kyupid@3f268a7
제가 하나 꼭 받고싶은 리뷰가 있다면 지금까지 이야기했던 DTO에 대한 피드백을 받고싶습니다.
리뷰어님이 만약에 개발중에 Entity/DTO 를 나눠서 사용하게 된다면 어떤 이유로 나누실건지 조금만 알려주시면 앞으로 학습에 엄청 도움이 될것 같습니다.
감사합니다.
ps. 아 그리고 Order 에 대한 구현은 하다 말았습니다. 참고해주세요