[1, 2단계 방탈출 예약 대기] 카피(김상혁) 미션 제출합니다. #17

tkdgur0906 · 2024-05-15T06:50:02Z

안녕하세요 영이~!! 카피입니다!

이번 미션은 저의 코드로 이어서 진행했습니다!
잘 부탁드립니다ㅎㅎ

이번 미션 변경사항

링크

이전의 웹 개발 경험

스프링을 통한 프로젝트 개발 경험이 있습니다.
JPA도 사용해본 적이 있지만 흐릿한 기억으로 남아있습니다.
AWS를 통해 배포해본 적이 있습니다.

웹 테스팅 시 로그인 정보

일반 회원

아이디 : user@naver.com
비밀번호 : 1234

관리자

아이디 : admin@naver.com
비밀번호 : 1234

Co-authored-by: tkdgur0906 <tkdgur0906@naver.com>

choijy1705

안녕하세요 카피
미션 빠르게 진행해주셨네요👍
몇가지 코멘트 남겼으니 확인부탁드립니다!

choijy1705 · 2024-05-15T13:18:00Z

src/main/java/roomescape/auth/AdminAuthHandlerInterceptor.java

+    @Override
+    public boolean preHandle(HttpServletRequest request,
+                             HttpServletResponse response,
+                             Object handler) throws Exception {


Suggested change

Object handler) throws Exception {

Object handler) {

choijy1705 · 2024-05-15T13:21:12Z

src/main/java/roomescape/controller/api/MemberApiController.java

+    @GetMapping("/members")
+    public ResponseEntity<List<MemberIdAndNameResponse>> getMembers() {
+        List<Member> members = memberService.findMembers();
+        return ResponseEntity.ok(
+                members.stream()
+                        .map(member -> new MemberIdAndNameResponse(member.getId(), member.getName()))
+                        .toList()
+        );
+    }


이 api는 어디서 호출되는건가요??

관리자 예약 페이지에서 관리자가 직접 예약을 추가할 시 선택할 멤버의 목록을 가져올 때 사용되는 api입니다.
전체 사용자의 목록을 일반 유저가 볼 수 있으면 안되기 때문에 interceptor의 config에 해당 api를 추가해놓았습니다.

reservation-with-member.js에서 확인하실 수 있습니다!

현재 엔드포인트를 "/admin/members"로 변경했습니다!

choijy1705 · 2024-05-15T13:23:03Z

src/main/java/roomescape/auth/AuthenticatedMemberArgumentResolver.java

+        Cookie[] cookies = request.getCookies();
+        String token = tokenProvider.extractTokenFromCookie(cookies);
+
+        if ("".equals(token)) {


Suggested change

if ("".equals(token)) {

if (token.isEmpty()) {

extractTokenFromCookie에서 "" 이 반환되면 안되고 null이 반환되어야 할 것 같네요!

전체적인 flow에서 토큰이 없다면 null을 반환하도록 변경하였습니다!
해당 로직은 null과 isEmpty()를 둘다 검증하도록 했습니다!

choijy1705 · 2024-05-15T13:24:05Z

src/main/java/roomescape/service/auth/TokenProvider.java

+    public String extractTokenFromCookie(Cookie[] cookies) {
+        if (cookies == null) {
+            return "";
+        }
+        return Arrays.stream(cookies)
+                .filter(cookie -> TOKEN.equals(cookie.getName()))
+                .map(Cookie::getValue)
+                .findFirst()
+                .orElse("");
+    }


빈문자열을 반환하기보다는 null을 반환하는게 좋을것 같아요

위와 동일하게 null을 반환하도록 했습니다!

choijy1705 · 2024-05-15T13:24:37Z

src/main/java/roomescape/service/auth/TokenProvider.java

+    }
+
+    public Long parseToken(String token) {
+        if (token == "") {


문자열 == 비교가 유효한가요??

해당 비교는 주소값을 비교하는 것이기 때문에 정상적으로 작동하지 않습니다.
따라서 equals() 또는 isEmpty()를 사용해야합니다!

isEmpty()로 변경하였습니다!

choijy1705 · 2024-05-15T13:44:01Z

src/main/java/roomescape/controller/api/ReservationApiController.java

+        );
+    }
+
+    @GetMapping("/admin/reservations/search")


admin을 위한 api라면 별도의 controller로 관리되는게 좋아보이네요!

동의합니다! admin 예약 전용과 일반 예약을 구분하기 위해 AdminReservationApicontroller를 새로 생성하였습니다

choijy1705 · 2024-05-15T13:45:47Z

src/main/java/roomescape/domain/Member.java

+    public Member(Long id, String name, String email, String password, Role role) {
+        this.id = id;
+        this.name = name;
+        this.email = email;
+        this.password = password;
+        this.role = role;
+    }


도메인에 대한 validation은 필요없을까요?
다른 도메인들도 고려해봐주세요!

email은 이메일 형식에 대한 검증이 필요할 것 같아 Email 객체로 wrapping하였습니다.
하지만 password나 다른 모든 필드에 대해 객체로 감싸는 것은 불필요한 것 같아 하지 않았습니다. (하나의 도메인으로써의 역할이 부족하다고 생각했습니다)

Member를 기준으로 봤을 때 지금 검증할만한 필드는 name, password의 null 체크 정도인 것 같은데, request로 받을 때 @NotNull 을 사용해서 검증하고 있기 때문에 굳이 필요한가? 라는 생각이 들었습니다.
도메인 단의 검증과 dto단의 검증은 다르다는 말도 들었지만 아직 명확한 기준이 서지 않아 영이의 생각을 묻고 싶습니다!

dto의 검증은 빠른 에러를 내보내기 위한 검증입니다. 비즈니스 로직이 복잡한경우 몇초가 걸리는 경우도 있습니다. 이때 로직이 처리된후 에러가 발생하면 고객의 입장에서는 굉장히 짜증날것 같네요

도메인의 검증은 로직이 수행되고 실제 도메인이 생성될때의 검증입니다. dto로 고객의 입력값으로 도메인이 생성될 수 있지만 내부적인 로직으로 인해 값이 변경될 수 있고. 여러가지 이유에서 dto와는 상관없는 값이 도메인으로 만들어질 수 있기 때문에 반드시 도메인의 검증은 필요할것같습니다

dto의 검증과 도메인의 검증이 맡는 역할이 다른 것 같습니다!
dto에서는 요청에 대한 검증을 하고, 도메인에서는 생성 그 자체에 대한 검증을 진행하는군요
현재는 단순하게 검증을 했는데, 도메인이 확장되면 VO로 분리해서 검증 책임을 나누는 것도 좋을 것 같습니다!

choijy1705 · 2024-05-15T14:07:25Z

src/main/java/roomescape/domain/Reservation.java

+    private LocalDate date;
+
+    @ManyToOne
+    private ReservationTime time;


컬럼명이 time_id로 들어갈것 같은데
도메인이 많아지면 정확한 명칭이 아니면 큰 혼란을 야기할 수 있을것 같습니다
모델이 reservationTime이고 테이블명도 reservationTime이기 때문에 reservationTimeId로 저장되도록 하는게 좋을것 같아요🙏

임의로 줄여쓰게 된다면 나중에 헷갈릴 수 있을 것 같네요!
명확히 필드 이름을 reservationTime으로 수정했습니다!

choijy1705 · 2024-05-15T14:11:00Z

src/main/java/roomescape/controller/api/ReservationTimeApiController.java

+                        .keySet()
+                        .stream()
+                        .map(reservationTime -> new ReservationStatusResponse(
+                                reservationTime,
+                                reservationStatus.findReservationStatusBy(reservationTime))


entrySet을 사용하면
reservationStatus.findReservationStatusBy 메서드도 지우고 더 가독성 좋아질것 같네요!

key를 가져오는게 아니라 그냥 entrySet을 가져오면 되는군요😅
훨씬 가독성이 좋은 것 같습니다!

choijy1705 · 2024-05-15T14:14:41Z

src/main/java/roomescape/domain/Reservation.java

+    @GeneratedValue(strategy = GenerationType.IDENTITY)
+    private Long id;
+
+    @ManyToOne


lazy laoding에 대한 부분은 고려해보신걸까요?

기존에 즉시 로딩에 대한 문제점은 들어본적이 있지만, 당시에 학습이 안된 상태여서 적용하지 않았습니다!
오늘 jpa 수업도 듣고 혼자 공부해보며 LAZY를 사용하게 되는 이유를 알게되었습니다.

제가 내린 결론은 조회시 불필요한 객체를 모두 가져오게 될 수 있으므로 EAGER 대신 LAZY + Fetch Join을 사용하자! 입니다.
여기서 고민해본 지점이 그럼 Inner join은 사용하지 않는 것인가? 였습니다.
현재 @Query를 사용하고 있는 2가지 메서드에서 join 대상은 검색 조건으로만 사용하고 반환하지는 않습니다.
따라서 불필요하게 영속성 컨텍스트에 올리지 않기 위해 inner join을 사용하는 것이 좋다 라는 결론을 냇는데, 영이의 생각은 어떠신가요?

어디가 좋다 나쁘다라는 걸 단정지을수 없는 부분인것 같아요!
상황에따라서 적절히 선택하는것이 좋을것 같아요

말씀하신대로 fetch join과 inner join을 상황에 맞게 적절히 사용하는게 좋은 것 같습니다!

tkdgur0906 · 2024-05-17T07:26:36Z

안녕하세요 영이~
리뷰 반영하면서 궁금한 것이 생겨 질문드립니다!

`JPA query method` vs `@Query`

메서드 이름을 가지고 쉽게 query method를 작성할 수 있는 것이 상당히 편리하다고 생각합니다.
하지만 요구 조건이 늘어남에 따라 메서드 이름의 길이가 길어지는 경우 @Query를 사용하여 작성하는게 수고는 들지만 사용할때 명확하다고 생각합니다.
현재 사용중인 existsByDateAndReservationTimeIdAndThemeId 도 개인적으로 너무 길지 않나? 라는 생각이 드는데,
영이는 본인만의 기준을 가지고 계신가요?

choijy1705

카피 피드백 잘 반영해주셨습니다👍
한번만 더 피드백 반영 요청 드려요!
확인 해보시고 반영부탁드립니다

choijy1705 · 2024-05-17T15:37:30Z

src/main/resources/application.properties

+spring.datasource.url=jdbc:h2:mem:database
+spring.jpa.show-sql=true
+spring.jpa.properties.hibernate.format_sql=true
+spring.jpa.ddl-auto=create-drop


개발환경은 로컬에서 코드를 작성하실때를 의미하는 것입니다!
코드를 작성하고 pr을 작성하는 코드는 실제 프로덕션까지 가는 코드입니다

jpa 가 자동으로 테이블을 생성하는걸 프로덕션에 그대로 쓰면 어떤 문제가 있을지는 한번 공부해보시면 좋을것 같아요
너무 많은 이유가 있어서 코멘트로 남길수 없을것 같습니다🙏

choijy1705 · 2024-05-17T15:41:27Z

src/main/java/roomescape/domain/Member.java

+    public Member(Long id, String name, String email, String password, Role role) {
+        this.id = id;
+        this.name = name;
+        this.email = email;
+        this.password = password;
+        this.role = role;
+    }


dto의 검증은 빠른 에러를 내보내기 위한 검증입니다. 비즈니스 로직이 복잡한경우 몇초가 걸리는 경우도 있습니다. 이때 로직이 처리된후 에러가 발생하면 고객의 입장에서는 굉장히 짜증날것 같네요

도메인의 검증은 로직이 수행되고 실제 도메인이 생성될때의 검증입니다. dto로 고객의 입력값으로 도메인이 생성될 수 있지만 내부적인 로직으로 인해 값이 변경될 수 있고. 여러가지 이유에서 dto와는 상관없는 값이 도메인으로 만들어질 수 있기 때문에 반드시 도메인의 검증은 필요할것같습니다

choijy1705 · 2024-05-17T15:45:04Z

src/main/java/roomescape/domain/Role.java

+
+    ADMIN,
+    USER
+    ;


Suggested change

;

이런 컨벤션은 어느 회사도 안쓸것 같아요!
인텔리제이에서도 회색처리되고 있어서 안쓰면 좋을것 같습니다

choijy1705 · 2024-05-17T16:02:22Z

src/main/java/roomescape/domain/Reservation.java

+    @GeneratedValue(strategy = GenerationType.IDENTITY)
+    private Long id;
+
+    @ManyToOne


어디가 좋다 나쁘다라는 걸 단정지을수 없는 부분인것 같아요!
상황에따라서 적절히 선택하는것이 좋을것 같아요

choijy1705 · 2024-05-17T16:07:09Z

src/main/java/roomescape/repository/ReservationTimeRepository.java

+    Optional<ReservationTime> findByStartAt(LocalTime startAt);
+
+    @Query("""
+            SELECT rt 


Suggested change

SELECT rt

SELECT rt.id

로직상 id만 받아되 되지 않나 싶네요!

ReservationTimeApiController의 getReservationTimesIsBooked()에서 response를 보낼 때 startAt을 같이 보내기 때문에 객체 전체의 값이 필요할 것 같습니다!

choijy1705 · 2024-05-17T16:08:48Z

src/main/java/roomescape/repository/ThemeRepository.java

+            GROUP BY t.id 
+            ORDER BY COUNT(t.id) DESC
+            """)
+    List<Theme> findRanksByPeriodAndCount(@Param("dateFrom") LocalDate dateFrom,


jpa가 생성해주는게 아닌 쿼리를 명시해주는 경우 메서드 명을 좀더 유연하게 작성할 수 있을것 같아요
findPopularTheme? 같은 메서드명이 더 가독성이 좋지 않을까요?

쿼리를 직접 작성했기 때문에 메서드 명을 유연하게 작성할 수 있을 것 같습니다!
findPopularThemes로 변경했습니다!

choijy1705 · 2024-05-17T16:11:05Z

src/main/java/roomescape/controller/api/MemberApiController.java

+    @GetMapping("/admin/members")
+    public ResponseEntity<List<MemberIdAndNameResponse>> getMembers() {
+        List<Member> members = memberService.findMembers();
+        return ResponseEntity.ok(
+                members.stream()
+                        .map(member -> new MemberIdAndNameResponse(member.getId(), member.getName()))
+                        .toList()
+        );
+    }
+
+    @PostMapping("/members")
+    public ResponseEntity<MemberIdAndNameResponse> signup(@RequestBody @Valid SignupRequest request) {
+        Member member = memberService.signUp(request);
+        return ResponseEntity.created(URI.create("/members/" + member.getId()))
+                .body(new MemberIdAndNameResponse(member.getId(), member.getName()));
+    }


admin 과 일반 api 는 클래스 분리가 되면 좋을것 같아요

choijy1705 · 2024-05-17T16:18:20Z

src/main/java/roomescape/repository/ReservationRepository.java

+
+    List<Reservation> findByMemberId(long memberId);
+
+    boolean existsByDateAndReservationTimeIdAndThemeId(LocalDate date, long timeId, long themeId);


메서드 이름을 가지고 쉽게 query method를 작성할 수 있는 것이 상당히 편리하다고 생각합니다.
하지만 요구 조건이 늘어남에 따라 메서드 이름의 길이가 길어지는 경우 @query를 사용하여 작성하는게 수고는 들지만 사용할때 명확하다고 생각합니다.
현재 사용중인 existsByDateAndReservationTimeIdAndThemeId 도 개인적으로 너무 길지 않나? 라는 생각이 드는데,
영이는 본인만의 기준을 가지고 계신가요?

딱히 기준같은건 없습니다. @Query 를 사용한다고해서 그렇게 명확해지는지에 대해서는 의문입니다🤔
그래도 jpa의 메서드 명 규칙이 더 이해하기 쉬울것 같긴하네요 개인적으로는
파라미터가 일정 수준 이상 넘어가게되면 테이블의 설계가 문제 이거나 혹은 로직적으로 푸는 방법을 고려해봐야하지 않을가 싶습니다!

그런 관점으로도 볼 수 있겠군요😀
이 부분에 대한 것은 미션을 진행하면서 저만의 기준을 세워봐야겠습니다!

choijy1705

카피 피드백 반영 잘해주셨습니다👍
이번단계는 머지할께요
다음 단계 진행해주세요!

Minjoo522 and others added 13 commits May 14, 2024 15:25

migrate first mission code

3f4a417

Co-authored-by: tkdgur0906 <tkdgur0906@naver.com>

chore: 의존성 추가

a938996

Co-authored-by: tkdgur0906 <tkdgur0906@naver.com>

chore: yml을 properties로 변경

84f3c95

Co-authored-by: tkdgur0906 <tkdgur0906@naver.com>

refactor: 토큰 만료기한 설정

20e8418

Co-authored-by: tkdgur0906 <tkdgur0906@naver.com>

feat: Member 엔티티에 JPA 도입

00b100a

Co-authored-by: tkdgur0906 <tkdgur0906@naver.com>

feat: Reservation, ReservationTime, Theme 엔티티에 JPA 도입

4244018

Co-authored-by: tkdgur0906 <tkdgur0906@naver.com>

feat: [2단계] 내 예약 목록 조회 기능 클라이언트 코드

aec9a5a

feat: 유저 예약 조회 페이지 렌더링

5c12370

Co-authored-by: tkdgur0906 <tkdgur0906@naver.com>

feat: 유저 예약 조회 api 구현

c420dcd

Co-authored-by: tkdgur0906 <tkdgur0906@naver.com>

style: 코드 포멧팅

4f9e9a3

Co-authored-by: tkdgur0906 <tkdgur0906@naver.com>

feat: Member equals, hashCode 재정의

c224b2b

Co-authored-by: tkdgur0906 <tkdgur0906@naver.com>

chore: 불필요한 의존성 제거

7f9ef62

Co-authored-by: tkdgur0906 <tkdgur0906@naver.com>

style: 라인 포맷팅

7e89ab2

choijy1705 requested changes May 15, 2024

View reviewed changes

tkdgur0906 added 12 commits May 16, 2024 11:11

refactor: 미사용 예외 처리 코드 제거

2c4b3b5

refactor: 토큰 예외 시 빈 문자열 대신 null 반환하도록 변경

c7fdaeb

refactor: 쿼리 작성 시 text block 사용

638cd07

chore: properties를 yml로 수정

fed31e7

refactor: reservation 컨트롤러 admin,user로 분리

786db9a

refactor: 예약 시간 컬럼명 reservationTime으로 명확히 변경

6e6ff0c

refactor: ReservationStatus를 response로 변환 시 entrySet을 사용하도록 변경

80e3a60

feat: 회원 가입 페이지 렌더링 기능 구현

18e88f1

refactor: 모든 회원 조회 api 엔드포인트 /admin/members로 변경

53bf940

feat: 회원가입 api 구현

5086391

feat: email을 검증할 수 있도록 wrapping

a12ea64

feat: jpa 지연로딩 적용

668b29f

choijy1705 requested changes May 17, 2024

View reviewed changes

tkdgur0906 added 2 commits May 18, 2024 23:43

refactor: Role에 불필요한 코드 제거

27be7f3

refactor: 엔티티 기본생성자 protected로 변경

47fb0a5

tkdgur0906 added 4 commits May 19, 2024 00:41

feat: 도메인 검증로직 추가

f7f0486

refactor: 인기 있는 테마 순위 가져오는 db 메서드 이름 명확하게 수정

a2a9222

refactor: 예약 api를 admin과 일반 컨트롤러로 분리

2243efd

feat: ddl 관리하는 schema.sql 생성

a62ac90

choijy1705 approved these changes May 19, 2024

View reviewed changes

choijy1705 merged commit fcf4785 into woowacourse:tkdgur0906 May 19, 2024


		List<Reservation> findByMemberId(long memberId);

		boolean existsByDateAndReservationTimeIdAndThemeId(LocalDate date, long timeId, long themeId);

[1, 2단계 방탈출 예약 대기] 카피(김상혁) 미션 제출합니다. #17

[1, 2단계 방탈출 예약 대기] 카피(김상혁) 미션 제출합니다. #17

Conversation

tkdgur0906 commented May 15, 2024 • edited Loading

이번 미션 변경사항

이전의 웹 개발 경험

웹 테스팅 시 로그인 정보

choijy1705 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkdgur0906 May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkdgur0906 May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkdgur0906 May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkdgur0906 commented May 17, 2024

JPA query method vs @Query

choijy1705 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkdgur0906 May 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkdgur0906 May 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choijy1705 left a comment

Choose a reason for hiding this comment

tkdgur0906 commented May 15, 2024 •

edited

Loading

tkdgur0906 May 16, 2024 •

edited

Loading

tkdgur0906 May 17, 2024 •

edited

Loading

tkdgur0906 May 17, 2024 •

edited

Loading

`JPA query method` vs `@Query`

tkdgur0906 May 18, 2024 •

edited

Loading

tkdgur0906 May 18, 2024 •

edited

Loading