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

[1 - 3단계 방탈출 사용자 예약] 재즈(함석명) 미션 제출합니다. #4

Open
wants to merge 48 commits into
base: seokmyungham
Choose a base branch
from

Conversation

seokmyungham
Copy link
Member

다들 수고 많으셨습니다 ㅎㅎ

seokmyungham and others added 30 commits May 2, 2024 17:58
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
- 시작 시간이 입력되지 않은 경우
- 시작 시간의 형식이 잘못된 경우

Co-authored-by: hyxrxn <gpfls981220@gmail.com>
- 시작 시간으로 확인

Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
- 아이디로 확인

Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
- 이름이 입력되지 않은 경우
- 날짜가 입력되지 않은 경우
- 날짜의 형식이 잘못된 경우
- 시간 아이디가 입력되지 않은 경우
- 시간 아이디가 자연수가 아닌 경우

Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
- 이름이 입력되지 않은 경우
- 설명이 입력되지 않은 경우
- 썸네일이 입력되지 않은 경우

Co-authored-by: hyxrxn <gpfls981220@gmail.com>
- 모든 테마 조회
- 아이디로 테마 조회

Co-authored-by: hyxrxn <gpfls981220@gmail.com>
- 삭제 시 아이디 존재 검증
- 삭제 시 해당 테마의 예약 존재 검증

Co-authored-by: hyxrxn <gpfls981220@gmail.com>
seokmyungham and others added 18 commits May 2, 2024 18:07
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
- 날짜와 테마 선택 이후 예약 가능한 시간 조회

Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Co-authored-by: hyxrxn <gpfls981220@gmail.com>
Copy link
Member

@Libienz Libienz left a comment

Choose a reason for hiding this comment

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

재즈 고생 👍

내가 스프링 초보라 재즈에게 도움이 되는 코멘트를 남겼을 지 의문이네 😂
다만 최선을 다했으

테스트에 생각이 깊은 것 같던데 관련해서 다음에 재즈랑 따로 얘기해보고 싶네 👍
이번 미션 고생했으 👊👊

다음 미션도 화이팅!

Comment on lines +48 to +49
# API 명세

Copy link
Member

Choose a reason for hiding this comment

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

👍👍

Comment on lines +32 to +36
@ResponseStatus(HttpStatus.CREATED)
@PostMapping
public ReservationResponseDto createReservation(@RequestBody ReservationRequestDto requestDto) {
return reservationService.createReservation(requestDto);
}
Copy link
Member

@Libienz Libienz May 3, 2024

Choose a reason for hiding this comment

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

우리가 응답을 커스텀하는 정도는 현재에선 상태 코드뿐인 것 같아.
그런 점에서 객체를 던지면서 @ResponseStatus 이용하는 현재의 코드는 미션 볼륨에 핏한 구현이라고 생각되네.

그런데 ResponseEntity와 비교해서 어떤 장점이 있는지는 잘 모르겠네 🤔
재즈는 어떻게 생각??

Copy link
Member Author

Choose a reason for hiding this comment

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

난 저번 레벨 미션 리뷰어로부터 리뷰를 받고,
ResponseEntity가 정말 필요한 경우가 아니라면 객체를 바로 반환하고 @ResponseStatus를 사용하는 것이 낫다. 라는 기준이 생겼어.

나도 전에는 리비랑 같은 궁금증이 있었는데, 리뷰어로부터 근본적인 답변을 듣지는 못했지만
ResponseEntity를 사용하는 목적을 생각하면 응답 시 Status code, HttpHeader, Body를 더 유연하게 제어할 수 있다.
이러한 상황이 필요한게 아니면 사용할 필요가 없다. 라는 의견이었고,

이제는 나도 ㅋㅋ 컨트롤러 코드에 ResponseEntity가 섞여있는 것 보다, 확실하게 필요한 부분에만 적절히 사용하는게 코드를 볼 때 더 명확하지 않나 생각이 들어.

특히 단순히 void를 반환 한다거나, 상태 코드 200 같은 경우엔 더더욱 사용할 필요가 없다는 생각이야

Comment on lines +34 to +38
@GetMapping("/availability")
public List<ReservationTimeResponseDto> findReservationTimesAvailability(@RequestParam String date,
@RequestParam Long themeId) {
return reservationTimeService.findReservationTimesAvailability(new AvailabilityOfTimeRequestDto(date, themeId));
}
Copy link
Member

Choose a reason for hiding this comment

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

api 엔드포인트를 /times/availability로 정의했군 👍
availability 같은 경우는 자원이라고 해석하기엔 위화감이 잇는 것 같아
URI가 유니크하게 식별할 수 있는 자원을 의미한다면 api 설계가 바뀌어도 좋을 것 같다는 개인적인 생각

어떤 크루는 /times/available?date=...&themeId=...와 같이 times 자원에 대한 필터링을 쿼리파라미터로 두었는데

나는 이쪽이 훨씬 합리적인 것 같더라
내가 REST를 잘 모르기도 해서 재즈의 의견이 궁금하군 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

나도 잘 모르는데.. ㅎㅎ
/times/availability로 정했을 때 리뷰어에게 피드백이 오진 않았는데,
/times/availabilty, /times/available 등등
path를 어떤 식으로 지어야 할 지 모르겠다고 여쭤보니, 함수나 변수 이름을 짓는 것과 다를 게 없다고 말해주셨어

REST 규칙 안에서 가져오려고 하는 대상이 무엇인지만 명확하게 알 수 있다면 뭐든 괜찮은 것 같아
나도 지금은 생각이 바껴서 times/available으로 수정했어 😀

Comment on lines +14 to +18
private void validateNotEmpty(String value) {
if (value == null || value.isBlank()) {
throw new IllegalArgumentException("이름은 공백을 허용하지 않습니다.");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

dto에 있을법도 한 검증인데 도메인에 위치시켰군

도메인을 안전하게 사용할 수 있도록 하는 장치라고 생각 👍

Comment on lines +15 to +27
public Reservation(Long id, Name name, Theme theme, ReservationDate date, ReservationTime time) {
this.id = id;
this.name = name;
this.theme = theme;
this.date = date;
this.time = time;
}

public Reservation(Long id, String name, Long themeId, String themeName, String description, String thumbnail,
String date, Long timeId, String time) {
this(id, new Name(name), new Theme(themeId, themeName, description, thumbnail), new ReservationDate(date),
new ReservationTime(timeId, time));
}
Copy link
Member

Choose a reason for hiding this comment

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

주생성자 부생성자 굳 👍
근데 이쯤 되면 빌더가 마려운 것 같아ㅋㅋ
언제 롬복쓸 수 있으려나 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋㅋ 전 미션 리뷰어가 공유해주신 글인데 이런 얘기도 있는 것 같아
https://dev.to/siy/when-builder-is-anti-pattern-3j92

나도 잘 몰라서 사용하게 되는 날이 왔을 때 같이 얘기해보면 좋을듯 👍

Comment on lines +17 to +20
@ExceptionHandler
public ResponseEntity<ErrorResult> handleException(HttpMessageNotReadableException ex) {
return new ResponseEntity<>(new ErrorResult(ex.getMostSpecificCause().getMessage()), HttpStatus.BAD_REQUEST);
}
Copy link
Member

Choose a reason for hiding this comment

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

getMostSpecificCause 처음 봄 ㄷㄷ
찾아봤는데 InnerMost cause를 가져오나보네

다만 위 메서드의 책임이 궁금하네 시간되면 재즈가 설명해줄 수 있으려나 ㅎㅎ 🙏

Comment on lines +43 to +44
INNER JOIN reservation_time AS t ON r.time_id = t.id
INNER JOIN theme AS th ON r.theme_id = th.id;
Copy link
Member

Choose a reason for hiding this comment

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

그냥 궁금한건데 레이지로딩은 우리가 지금 구현하기에는 많이 힘들겠지..?

Copy link
Member Author

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋㅋ JPA를 말하는거겠지??
생각 못해봤는데 일반 SQL로도 가능할지 모르겠네 리비가 보여줘 💪

Comment on lines +64 to +82
@Override
public List<Theme> findTopBookedThemes(LocalDate startDate, LocalDate endDate, int themeCount) {
String sql = """
SELECT th.id, th.name, th.description, th.thumbnail
FROM theme AS th
INNER JOIN reservation AS r ON th.id = r.theme_id
WHERE r.date BETWEEN :startDate AND :endDate
GROUP BY th.id
ORDER BY COUNT(th.id) DESC
LIMIT :themeCount;
""";

SqlParameterSource paramMap = new MapSqlParameterSource()
.addValue("startDate", startDate)
.addValue("endDate", endDate)
.addValue("themeCount", themeCount);
return jdbcTemplate.query(sql, paramMap, rowMapper);
}

Copy link
Member

Choose a reason for hiding this comment

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

서비스 맥락 (날짜 기간, 페이지 수)을 repository에서 잘 분리한 것 같군

나는 쿼리안에 일주일이라는 기간과 10개를 가져온다는 하드 코딩 숫자가 다 박혀있는데 ㅋㅋ...

오늘 강의를 보니까 여러 변경 요구사항이 있을 수 있겠더라
한 수 배웠으 💪🏻

Copy link
Member

Choose a reason for hiding this comment

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

themeCount 분리 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

한 술 더떠서 아예 클라이언트로부터 받아오게 바꿔봤는데 좋은 것 같아
JS를 조금 수정해야 하긴 하지만 😅

Comment on lines +38 to +41
ReservationTime time = reservationTimeRepository.findReservationTimeById(requestDto.getTimeId());
if (LocalDateTime.of(reservation.getDate(), time.getStartAt()).isBefore(LocalDateTime.now())) {
throw new IllegalArgumentException("지나간 날짜와 시간에 대한 예약은 불가능합니다.");
}
Copy link
Member

Choose a reason for hiding this comment

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

도메인 관련 규칙인 것 같은데 서비스로직에 위치시킨 이유가 있을까!?

Copy link
Member Author

Choose a reason for hiding this comment

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

오 정말 그렇네 좋은 지적 👍👍👍
책임이 이상한 곳에 가 있었군

Copy link
Member

@leegwichan leegwichan left a comment

Choose a reason for hiding this comment

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

하이 재즈! 커찬이야~ (이번에도 재즈 리뷰 하는군...)

일단 보이는 부분 달아봤는데, 코멘트 남겨줘~!! (그리고 내가 리뷰하는데 봐줬으면 하는 부분 있으면 코멘트 남겨줘~)

Copy link
Member

Choose a reason for hiding this comment

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

API 명세서 너무 깔끔하게 정리했는데? 👍 👍 👍

Copy link
Member

Choose a reason for hiding this comment

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

Repository interface만 domain 패키지에 있는 이유가 있을까?
service나 repository 패키지에 있어도 될 것 같은데?

Copy link
Member Author

Choose a reason for hiding this comment

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

woowacourse#80 (comment)

질문에 대한 답변은 리뷰어와 대화로 가능할 것 같은데, 추가로 같이 나눠보면 좋은 내용이 있어서 공유 👍

Copy link
Member

Choose a reason for hiding this comment

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

깔끔한 예외 처리 👍 👍 👍

Comment on lines +68 to +75
@Override
public boolean hasReservationOfTimeId(long timeId) {
String sql = "SELECT 1 FROM reservation WHERE time_id = :timeId";
SqlParameterSource parameterSource = new MapSqlParameterSource()
.addValue("timeId", timeId);
List<Integer> result = jdbcTemplate.queryForList(sql, parameterSource, Integer.class);
return !result.isEmpty();
}
Copy link
Member

Choose a reason for hiding this comment

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

List를 가져오는 것보다는 EXIST 사용해서 Boolean 값을 가져오는게 좋아 보이는데?

    public boolean isExistReservationByTimeId(Long timeId) {
        String sql = """
                SELECT EXISTS (
                    SELECT 1
                    FROM reservation
                    WHERE time_id = ?
                )
                """;
        return jdbcTemplate.queryForObject(sql, Boolean.class, timeId);
    }

Comment on lines +6 to +13
@Controller
public class HomeController {

@GetMapping("/")
public String getIndexPage() {
return "index";
}
}
Copy link
Member

Choose a reason for hiding this comment

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

HomeController 따로 명시 안해줘도 될 것 같은데, 한 이유가 있나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

시간이 없어 미처 변경하지 못했는데 지금은 UserController로 옮겼어 👍

Comment on lines +23 to +27
public Reservation(Long id, String name, Long themeId, String themeName, String description, String thumbnail,
String date, Long timeId, String time) {
this(id, new Name(name), new Theme(themeId, themeName, description, thumbnail), new ReservationDate(date),
new ReservationTime(timeId, time));
}
Copy link
Member

Choose a reason for hiding this comment

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

이 정도면, Theme, ReservationTime 정도는 외부에서 주입 받는 것이 좋지 않을까?

Copy link
Member Author

@seokmyungham seokmyungham May 6, 2024

Choose a reason for hiding this comment

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

원시 값을 모두 열어서 더 다양한 상황에 유연하게 대처하려고 했어
위에 주 생성자가 있는데 못봤나... 😅

Comment on lines +21 to +27
public JdbcReservationRepository(DataSource dataSource, RowMapper<Reservation> rowMapper) {
this.jdbcTemplate = new NamedParameterJdbcTemplate(dataSource);
this.jdbcInsert = new SimpleJdbcInsert(dataSource)
.withTableName("reservation")
.usingGeneratedKeyColumns("id");
this.rowMapper = rowMapper;
}
Copy link
Member

Choose a reason for hiding this comment

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

RowMapper를 외부에서 주입받는 이유가 있어? 어자피 JdbcReservationRepository에서만 사용하면, JdbcReservationRepository에서 생성해서 사용하는 편이 좋지 않을까?

AS 값 등이 RowMapper에서 사용된 값에 종속되어서, 하나의 변경 포인트의 2개의 파일을 봐야되서 힘들 듯;;

Copy link
Member Author

Choose a reason for hiding this comment

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

전 리뷰를 받고 JPA를 위한 JDBC의 동작을 학습하는 과정에서 파생된 결과물인데,
변경 된 사항을 굳이 다시 되돌릴 필요는 없을 것 같아서 그대로 뒀어. 설명이 없었다보니 보는 입장에서 헷갈렸겠네 😅

Comment on lines +64 to +82
@Override
public List<Theme> findTopBookedThemes(LocalDate startDate, LocalDate endDate, int themeCount) {
String sql = """
SELECT th.id, th.name, th.description, th.thumbnail
FROM theme AS th
INNER JOIN reservation AS r ON th.id = r.theme_id
WHERE r.date BETWEEN :startDate AND :endDate
GROUP BY th.id
ORDER BY COUNT(th.id) DESC
LIMIT :themeCount;
""";

SqlParameterSource paramMap = new MapSqlParameterSource()
.addValue("startDate", startDate)
.addValue("endDate", endDate)
.addValue("themeCount", themeCount);
return jdbcTemplate.query(sql, paramMap, rowMapper);
}

Copy link
Member

Choose a reason for hiding this comment

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

themeCount 분리 👍

Comment on lines +21 to +31
@ExtendWith(MockitoExtension.class)
class ThemeServiceTest {

@Mock
private ThemeRepository themeRepository;

@Mock
private ReservationRepository reservationRepository;

@InjectMocks
private ThemeService themeService;
Copy link
Member

Choose a reason for hiding this comment

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

Service 테스트 코드 잘 가져 갑니다~

Copy link
Member Author

@seokmyungham seokmyungham May 6, 2024

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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants