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_이영준_2주차 과제 step2,step3 #426

Open
wants to merge 54 commits into
base: 20jcode
Choose a base branch
from

Conversation

20jcode
Copy link

@20jcode 20jcode commented Jul 7, 2024

wishlist 테스트는 아직 완전히 구현하지 못하였습니다.

테스트를 지속적으로 만들면서 작성을 해보고자 하는데

  1. 테스트에서 항상 중복으로 만들어지는 코드에 대해서 특정 클래스를 만들어서 활용하는 것이 좋은 방법일까요? 테스트끼리의 종속성? 이 생기게 될까요?

  2. 클론코딩 강의에서도 들었던 것 같은데, 하나의 결과에 대해서 스프링에서 지원하는 다양한 방법들이 있는데, 이중 어느 것을 선택해야하는 지에 대해서 기준을 어떤 식으로 세울 수 있을까요?

++
step1 리뷰를 아직 받지 못해서 코드가 중복되어있습니다.
+++
지속적으로 리펙토링을 하고자 하는데, PR을 추가로 더 생성해서 리뷰요청 드려도 될까요?

20jcode added 30 commits July 1, 2024 15:58
test를 위해서 추가
컨트롤러에서 해당 부분에 대해서 @Valid 구현 필요
경로 변경한 것에 대해서 정상 동작 확인
전역적으로 예외 처리를 위한 Advisor 추가

예외관련 내용을 담은 ExceptionResponseDTO 추가
step2를 위해서
예외Advisor에서 응답에 대한 httpstatus 없이 동작할 수 있도록
회원 가입, 중복된 이메일 가입 시도
Copy link

@cmst-steve cmst-steve left a comment

Choose a reason for hiding this comment

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

테스트에서 항상 중복으로 만들어지는 코드에 대해서 특정 클래스를 만들어서 활용하는 것이 좋은 방법일까요? 테스트끼리의 종속성? 이 생기게 될까요?

특정 클래스라는게 무엇을 의미하는지 이해가되지않습니다~. 구제척인 코드나 예시를 들어주시면 제가 다음에 조금 더 수월하게 답변할 수 있을 것 같습니다 :)

어찌됐든 중복된 코드는 하나의 메소드로 분리하거나 혹은 하나의 클래스로 분리하여 중복도를 낮추는 방법은 좋은 것 같습니다 :)

클론코딩 강의에서도 들었던 것 같은데, 하나의 결과에 대해서 스프링에서 지원하는 다양한 방법들이 있는데, 이중 어느 것을 선택해야하는 지에 대해서 기준을 어떤 식으로 세울 수 있을까요?

하나의 결과에 대해서 스프링에서 지원하는 다양한 방법이 무엇을 말하는 걸까요...? 다음 PR에 구체적으로 질문주셔도 좋고 DM주셔도 좋으니 조금만 더 구체적으로 질문주시면 감사하겠습니다 :)

@Target(ElementType.PARAMETER)
@Retention(RetentionPolicy.RUNTIME)
public @interface LoginMember {
//컨트롤러에서 사용을 위해

Choose a reason for hiding this comment

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

파일전반 적인 설명과 관련된 주석은 상단에 적어주시는게 좋을 것 같습니다.
또한 단순 "컨트롤러에서 사용을 위해"라는 것만으로는 내용이 잘 이해가지않을 수 있습니다.
주석이 필요하다면 구체적으로 적어주시면 좋습니다 :)

* @param dto 수정하고자 하는 값 이외 null로 지정
*/
@PutMapping("/products")
public void update(@RequestParam("id") Long id, @RequestBody @Valid ProductDTO dto) {

Choose a reason for hiding this comment

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

이 경우에는 requestParma보다 @PathVariable로 받아 Put, /products/{id}로 url을 표기하는게 rest 스럽습니다.
rest 규격에 맞게 url을 설계하는 방법을 조금 더 학습하시면 좋을 것 같습니다 :)

Choose a reason for hiding this comment

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

이 외에도 어떤 부분이 있을지 한번 찾아보시고 수정해보시면 좋을 것 같습니다~


//상품 리스트 조회
@GetMapping("/{memberid}")
public WishListDTO getWishList(@PathVariable int memberid, @LoginMember MemberDTO memberDTO) {

Choose a reason for hiding this comment

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

변수명은 camelCase를 원칙으로 하고있습니다. memberId로 변경해주세요.

+ "email varchar(255) not null, "
+ "password varchar(255) not null, "
+ "role varchar(255) not null, "
+ "token varchar(255))");

Choose a reason for hiding this comment

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

왜 토큰을 저장해야할까요~? 꼭 필요한 부분일까요?

template.update(
"create table if not exists member("
+ "id long primary key auto_increment, "
+ "email varchar(255) not null, "

Choose a reason for hiding this comment

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

만약 중복된 이메일로 회원가입을 하면 어떻게 될까요?
이를 데이터베이스단에서 막기 위해서는 어떤 방법을 취해야할까요?

var products = jdbcProductRepository.findAll();
List<ProductDTO> productDTOList = new ArrayList<>();

for (var product : products) { //DTO로 전환

Choose a reason for hiding this comment

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

collection.map을 사용하는건 어떨까요~?

}

@Override
public void addNewProduct(long memberId, long productId) {

Choose a reason for hiding this comment

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

add와 new를 같이 사용하기보다는 addProduct만으로 표현해도 충분합니다~

@20jcode
Copy link
Author

20jcode commented Jul 8, 2024

말씀해주신 부분 수정 + step3 wishlist 부분 구현 완료하였습니다. 그리고 질문했던 내용은 제가 더 깔끔하게 정리해서 오겠습니당.
감사합니다.

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

Successfully merging this pull request may close these issues.

2 participants