Skip to content

[8, 9, 10단계 - Spring Core] 최지수 미션 제출합니다 #494

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

Open
wants to merge 1 commit into
base: jisoo78
Choose a base branch
from

Conversation

jisoo78
Copy link

@jisoo78 jisoo78 commented Jun 29, 2025

주노 안녕하세요 이번 미션은 많이 늦었네요...
이번 마지막 미션까지 화이팅입니다

고민한 부분

새로운 sql 구문에서 time_id 와 time 테이블의 구조를 보면서
문자열이 오는 경우와 정수가 오는 경우를 확인하면서 진행했어요
요청받는 파라미터를 Map<String, String> 형태로 받을 수 있게 구현했어요
다양한 값을 받을 때 간단하게 처리할 수 있고 DTO를 계속 만들지 않아도 되는 장점이 있더라고요
검증을 하면서 null 값이 들어올 때는 예외 처리를 했습니다

궁금한 점

컨트롤러나 서비스 패키지에서 클래스를 따로 만들어서 예약관리 컨트롤러와 시간관리 컨트롤러를 구분했어요
다른 분들을 보니 예약관리 패키지와 시간관리 패키지를 구분해서 작성한 경우도 있더라고요
서로 장단점이 있는 거 같은데 어떤 방식을 주로 사용하시나요?

Copy link

@Choi-JJunho Choi-JJunho 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 +44 to -41
@Override
public Reservation save(ReservationRequest reservationRequest) {
String sql = "INSERT INTO reservation (name, date, time_id) VALUES (?, ?, ?)";
KeyHolder keyHolder = new GeneratedKeyHolder();
jdbcTemplate.update(con -> {
PreparedStatement ps = con.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS);
ps.setString(1, reservationRequest.name());
ps.setString(2, reservationRequest.date());
ps.setInt(3, reservationRequest.timeId());
return ps;
}, keyHolder);
int reservationId = keyHolder.getKey().intValue();
return findById(reservationId);
}

int id = simpleJdbcInsert.executeAndReturnKey(params).intValue();
return new Reservation(id, reservation.name(), reservation.date(), reservation.time());

Choose a reason for hiding this comment

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

SimpleJdbcInsert를 사용하지 않은 이유가 있나요?

Comment on lines +39 to 51
Time time;
if (checkNum(timeValue)) {
time = timeRepository.findById(Integer.parseInt(timeValue));
} else {
time = timeRepository.findByTime(timeValue);
}
if (time == null) {
throw new BadRequestException("해당 시간은 존재하지 않습니다");
}

ReservationRequest reservationRequest = new ReservationRequest(name, date, time.id());
return reservationRepository.save(reservationRequest);
}

Choose a reason for hiding this comment

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

수정 가능한 변수가 돌아다니는것은 다소 불안정한 구조로 보이네요!
time을 불변으로 다루는 구조로 리팩토링해보시죠!

Comment on lines +37 to +40
public ResponseEntity<Reservation> reservationAdd(@RequestBody Map<String, String> params) {
Reservation reservation = reservationService.addReservation(params);
URI location = URI.create("/reservations/" + reservation.id());
return ResponseEntity.created(location).body(reservation);

Choose a reason for hiding this comment

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

객체를 사용하지 않고 Map을 사용한 이유가 있나요?

import java.util.Map;

@Controller
public class TimeController {

Choose a reason for hiding this comment

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

컨트롤러나 서비스 패키지에서 클래스를 따로 만들어서 예약관리 컨트롤러와 시간관리 컨트롤러를 구분했어요
다른 분들을 보니 예약관리 패키지와 시간관리 패키지를 구분해서 작성한 경우도 있더라고요
서로 장단점이 있는 거 같은데 어떤 방식을 주로 사용하시나요?

도메인을 기준으로 패키지를 구분하는것과 계층을 기준으로 패키지를 구분하는것 서로 어떤 장단점이 있을까요?

Copy link

@dd-jiyun dd-jiyun left a comment

Choose a reason for hiding this comment

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

안녕하세요 지수님! 🙌
이번 미션도 고생 많으셨어요!

리뷰하면서 궁금했던 부분에 대해 코멘트 남겨두었습니다 👍🏻
단순히 미션을 통과하는 데에 그치기보다는,
"이 코드를 다른 사람이 봤을 때 얼마나 쉽게 이해할 수 있을까?" 라는 시선으로
가독성에 대해서도 한 번 더 고민해보시면 좋을 것 같아요 😊

다음 미션도 화이팅입니다!

}

@GetMapping("/time")
public String TimePage() {
Copy link

Choose a reason for hiding this comment

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

메서드 네이밍은 lowerCamelCase로 작성하는 규칙이 있어요! 일관성 있게 수정해주시면 좋을 것 같습니다 👍🏻

아래 참고하면 좋은 블로그입니다!
좋은 코드를 위한 자바 메서드 네이밍

@@ -1,4 +1,4 @@
package roomescape.model;

public record Reservation(int id, String name, String date, String time) {
public record Reservation(Integer id, String name, String date, Time time) {
Copy link

@dd-jiyun dd-jiyun Jul 1, 2025

Choose a reason for hiding this comment

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

id 타입을 int 에서 Integer로 바꾸신 이유가 있나요 ?? 🤔

@@ -0,0 +1,4 @@
package roomescape.model;

public record ReservationRequest(String name, String date, Integer timeId) {
Copy link

Choose a reason for hiding this comment

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

DTO 역할을 하는 클래스 같아요 ! model 패키지에 위치해두신 이유가 있을까요?

Comment on lines 12 to +15
Reservation save(Reservation reservation);

Reservation save(ReservationRequest reservationRequest);

Copy link

Choose a reason for hiding this comment

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

save가 두 개인 이유가 있을까요?

Comment on lines 39 to +42
@Override
public Reservation save(Reservation reservation) {
Map<String, Object> params = new HashMap<>();
params.put("name", reservation.name());
params.put("date", reservation.date());
params.put("time", reservation.time());
return null;
}
Copy link

Choose a reason for hiding this comment

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

불필요한 메서드라면 제거해주시는 게 좋을 것 같아요 🤭

@izzy80
Copy link

izzy80 commented Jul 1, 2025

이번 미션도 수고하셨습니다~ 😊


import java.util.List;

public interface TimeRepository {
Copy link

Choose a reason for hiding this comment

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

추상화를 해주셨네요! interface의 특징은 무엇이 있을까요?

@@ -54,6 +57,7 @@ void reservationList() {
@Test
@DisplayName("예약 추가/취소")
void 삼단계() {
Copy link

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.

4 participants