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 Core] 이준민 미션 제출합니다 :) #384

Open
wants to merge 6 commits into
base: jermany17
Choose a base branch
from

Conversation

jermany17
Copy link

No description provided.

Copy link

@TaeyeonRoyce TaeyeonRoyce left a comment

Choose a reason for hiding this comment

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

안녕하세요, 준민님
계층별로 잘 나누어 수정하신 것 같지만, 한가지 아쉬운 부분이 있어 코멘트 남겨두었습니다.

별다른 질문이 없으셔서 코드 위주로만 리뷰 남겼어요.
고생하셨어요~


return ResponseEntity.created(URI.create("/reservations/" + id)).body(newReservation);
Reservation newReservation = reservationService.addReservation(request);
return ResponseEntity.created(URI.create("/reservations/" + newReservation.getId())).body(newReservation);

Choose a reason for hiding this comment

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

괄호가 깊게 중첩되어 있어요.
풀어내면 가독성이 좋아질 것 같아요

Comment on lines +15 to +16
@Repository
public class ReservationDao {

Choose a reason for hiding this comment

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

어노테이션은 Repository를,
클래스는 Dao라는 네이밍을 사용하셨군요.

둘의 차이가 어떤것이라 생각하시고, Dao라는 네이밍을 적용하신 이유가 궁금해요

@@ -32,12 +28,24 @@ public String getName() {
return name;
}

public void setName(String name) {

Choose a reason for hiding this comment

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

setter는 어떤 이유로 추가되었나요?

Comment on lines +50 to 70
public void validateAndSetTimeId(JdbcTemplate jdbcTemplate) {
if (name == null || name.isEmpty() || date == null || date.isEmpty() || time == null || time.isEmpty()) {
throw new InvalidReservationException("필수 입력값이 누락되었습니다.");
}

try {
timeId = Long.parseLong(time);
} catch (NumberFormatException e) {
throw new InvalidReservationException("유효하지 않은 timeId입니다: " + time);
}

String timeQuery = "SELECT time FROM time WHERE id = ?";
try {
// timeId로 유효성 검사
String resolvedTime = jdbcTemplate.queryForObject(timeQuery, String.class, timeId);
if (resolvedTime == null) {
throw new InvalidReservationException("유효하지 않은 timeId입니다: " + timeId);
}
} catch (EmptyResultDataAccessException e) {
throw new InvalidReservationException("유효하지 않은 시간입니다: " + timeId);
}

Choose a reason for hiding this comment

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

이 코드는 문제가 많아보이네요.
레이어드 아키텍쳐를 활용하여 각 계층별 관심사를 분리하였는데, ReservationRequest(presentation)에서 jdbcTemplate을 의존하는 것은 수정해야 할 것 같아요.

요청에 대한 모든 검증이 이곳에서 이루어 지고 있는것이 문제인데요.
요청에 대한 검증을 단계별로 나누어 계층에 맞게 배치시키면 어떨까요?

  1. null 값 검증
  2. 포맷 검증
  3. 존재하는 자원인지에 대한 검증 (DB 의존)

각 검증을 적절한 계층에서 이루어지도록 나눠주세요.

return time;
}

public void setTime(String time) {

Choose a reason for hiding this comment

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

전체적으로 객체 내의 setter는 제거해주세요
현재 setTime은 사용 하지 않는 것 같아요.

}

public Time addTime(Time time) {
if (time.getTime() == null || time.getTime().trim().isEmpty()) {

Choose a reason for hiding this comment

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

값에 대한 단순 검증은 어디에서 이루어지면 좋을까요?
꼭 service 영역에서 해야하는걸까요?

추가로 time.getTime().trim().isEmpty()time.getTime().isBlank()로 제공 되는 기능이 있습니다

time_id BIGINT,
PRIMARY KEY (id),
FOREIGN KEY (time_id) REFERENCES time(id)
);

Choose a reason for hiding this comment

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

EOF 신경 쓰세요!

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