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

[Spring Data JPA] 정다영 미션 제출합니다. #55

Open
wants to merge 13 commits into
base: day024
Choose a base branch
from

Conversation

day024
Copy link

@day024 day024 commented Jul 2, 2024

✔️안녕하세요 리뷰어님! 미션 진행 고생 많으셨습니다.
저는 현재 6단계까지 테스트는 모두 통과했으나 요구사항을 만족하지 못하고 있습니다.
4,5단계는 금방 진행했는데 6단계가 가장 어렵게 느껴지네요😢

요구 사항을 만족시키기 위해 수정 중인데 여전히 기능이 제대로 작동하지 않네요 ㅠㅠ
더 공부해보고 수정하여 커밋하겠습니다.

여러 부분 조언 해주시면 반영해보도록 하겠습니다.

🍀Spring Data JPA


✅중점으로 진행한 점

  • 기능 구현 및 미션테스트 통과

✅미션 수정해야할 사항들

  • 예약 대기 요청 기능 / 취소문제
    Caused by: java.lang.NumberFormatException: For input string: "undefined" error 발생 수정
  • 복잡한 의존관계 및 구조에 대한 리팩터링
  • 시크릿키 숨기기

Copy link

@zhy2on zhy2on left a comment

Choose a reason for hiding this comment

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

미션 너무 수고하셨습니다!!
문제를 꼼꼼히 읽고 해결하신 것 같아서 인상 깊었어요!
스터디 때 배운 의존성 정리, Optional 반환, 익셉션 처리에 대해서도 고민해보고 리팩토링 하시면 더 좋을 것 같습니다..!
제가 아직 많이 부족해서 리뷰도 부족한 것 같습니다ㅠㅠ... 잘못된 부분이 있다면 언제든지 다시 의견 남겨주시면 감사하겠습니다!
다시 한 번 수고하셨습니다!

@@ -71,8 +79,6 @@ public String extractTokenFromCookie(Cookie[] cookies) {
return cookie.getValue();
}
}

return "";
Copy link

Choose a reason for hiding this comment

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

토큰을 찾지 못 했을 때 빈 문자열을 반환하는 것보다 null을 반환하는 것이 좋지 않을까요??


@Service
public class MemberService {
private MemberDao memberDao;
@Autowired
Copy link

Choose a reason for hiding this comment

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

필드 주입과 생성자 주입이 혼용돼서 사용되고 있는데 생성자 주입으로 통일 시키는 것이 좋을 것 같습니다!

return member; // Member 객체의 getter 메서드 추가
}

public void setMember(Member member) {
Copy link

Choose a reason for hiding this comment

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

엔티티에서는 setter 사용을 지양하는 것이 좋다고 합니다!
setter를 없애거나, 의도가 명확히 들어나는 메소드로 적는 것이 좋다고 해요..!
아래 블로그를 읽어 보시면 좋을 거 같습니다!
https://velog.io/@langoustine/setter-지양-이유

import jakarta.persistence.GenerationType;
import jakarta.persistence.Id;

@Entity
Copy link

Choose a reason for hiding this comment

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

LoginMember는 엔티티가 아니어서 @Entity를 붙이지 않는 게 맞지 않나 싶습니다..!
엔티티는 실제로 데이터베이스와 일대일 매핑이 되는 영속성을 띄는 객체이기 때문에 LoginMember는 엔티티가 아니라 DTO 같습니다!

아래에 있는 JPA 관련 어노테이션들도 다 붙이면 안 될 것 같습니다!

@@ -0,0 +1,57 @@
package roomescape.reservation;

public class MyReservationResponse {
Copy link

Choose a reason for hiding this comment

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

record를 사용하면 더 간결할 것 같습니다!
DTO도 불변 객체로 만드는 것이 좋기 때문에 setter 메서드를 없애고, 필드를 final로 선언하는 것이 좋은데 record를 사용하면 간단하게 이 둘을 해결할 수 있어 좋다고 봅니다!

Copy link

@zhy2on zhy2on Jul 4, 2024

Choose a reason for hiding this comment

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

reservationRequest도 불변객체로 사용하는 게 더 좋다고 생각합니다!
record로 구현하면 더 좋을 것 같습니다!

Copy link

Choose a reason for hiding this comment

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

entity의 setter를 제거하는 것이 좋을 것 같습니다!

@RestController
public class WaitingController {
@Autowired
private WaitingService waitingService;
Copy link

Choose a reason for hiding this comment

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

생성자 주입이 되고 있기 때문에 필드 주입을 없애도 될 것 같습니다! @Autowired를 빼면 좋을 것 같습니다! 또 final로 선언하면 좋을 것 같습니다!

Copy link

Choose a reason for hiding this comment

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

record를 사용하면 더 좋을 것 같습니다!

Copy link

Choose a reason for hiding this comment

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

record를 사용하면 더 좋을 것 같습니다!

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