-
Notifications
You must be signed in to change notification settings - Fork 142
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] 박희원 미션 제출합니다. #431
base: bamsanchaeg
Are you sure you want to change the base?
Conversation
TimeDAO 인터페이스 생성으로 각 기능 오버라이딩 및 메서드 구현
지금 커밋으로 적용해야 데이터베이스 정상작동이 되기 때문에 기준점으로 삼아야 함
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아무래도 테스트 코드를 통과한 후 차근차근 커밋을 하게 되어 이전 단계가 맞는지에 대한 확신이 없어 커밋을 자주 진행하지 못했습니다.
보통 롤백 단위를 기준으로 커밋을 한다고 알고 있는데, 이 흐름을 어떻게 계획하시는지가 궁금합니다.
저는 만약 팀 컨벤션이 있다면 팀 컨벤션에 따르고 없다면 기능 단위로 커밋을 합니다! 커밋을 하는 이유는
- 개발자 중에는 커밋을 통해 코드의 흐름을 이해하는 사람도 있어요. 그리고 그편이 더 보기 좋기도 하고요.
- 나중에 프로젝트를 하게 되면 기능상의 오류나 문제로 인해 롤백을 해야 하는 경우가 있어요! 이때 가능한 많이 커밋할 수록 롤백한 후에 다시 코드를 짜야되는 양이 줄어들겠죠?
너무 커밋에 얽매이는 건 힘들지만 그래도 습관을 들여놓으면 좋을 것 같아요!
Json 데이터를 주입받을때 변경된 클래스의 의존관계를 이해하고 삽입하는지에 대한 난관이 있어서
Json을 이용한 테스트 코드를 사용할 시 의존성이 있는 객체 주입을 어떻게 해야하는지.. 에 대한 고민이 있습니다.
이 질문은 조금 더 설명이 필요할 것 같아요! Json 데이터를 주입받을때 변경된 클래스의 의존관계를 이해하고 삽입하는지
이 말과 Json을 이용한 테스트 코드를 사용할 시 의존성이 있는 객체 주입
이 말이 무슨 의미인지 더 자세히 설명해주세요!
import roomescape.service.TimeService; | ||
|
||
@RestController | ||
@RequestMapping("/times") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RequestMapping으로 묶으면 어떤 장단점이 있나요?😊😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 @RequestMapping으로 time과 reservation의 기능들을 하나의 카테고리처럼 묶어주는 용도로 사용했는데요(/time, /reservation) , @RequestMapping은 하위 어노테이션을 다 포괄하고, 다양한 HTTP 메서드를 처리할 수 있는 장점이 있습니다. 다만 단일 메서드에 대한 명확성이 부족하기 때문에 메서드의 의도를 파악하기 어렵다는 단점이 있습니다.
그래서 코드를 사용함에 있어 명확성이 부족하기 때문에 관련된 하위 동작들은 @GetMapping, @PostMapping, @PutMapping, @DeleteMapping 으로 해결했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전 개인적으로 이렇게 클래스 상단에서 RequestMapping로 묶어주는 걸 지양하는 편이지만 이것도 도요님이 경험으로 뭐가 더 좋을 지 판단해도 좋을 것 같아요😊
} | ||
|
||
@PostMapping | ||
public ResponseEntity<Reservation> createReservation(@RequestBody Reservation reservation) { | ||
|
||
Reservation newReservation = reservationJdbcDAO.create(reservation); | ||
Reservation newReservation = reservationService.createReservation(reservation); | ||
return ResponseEntity.created(URI.create("/reservations/" + newReservation.getId())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResponseEntity를 사용하는 기준이 궁금해요!😊😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RestController를 사용한 상태에서 JSON 데이터를 반환받을때는 데이터만을 반환받으면 되기 때문에 따로 responseEntity를 사용하지는 않았고, 따로 객체를 생성해줘야 하는 Post 방식을 사용할때는 리소스의 상태코드나 응답 본문이 반환이 필요하다고 생각해 ReseponseEntity를 사용했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상태코드는 ResponseStatus를 활용할 수 있고 응답본문도 ResponseEntity를 사용하지 않아도 반환이 가능하지 않나요?🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DAO는 무엇을 의미하고 해당 용어를 사용한 이유는 무엇인가요?😊😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data Access Object 라는 의미로 알고있고, 직접 DB에 접근하여 데이터를 삽입, 삭제 조회 등 조작할 수 있는 기능을 수행한다고 알고있습니다. JDBC를 사용하며 클래스 자체에서 데이터를 삽입, 삭제, 조회를 할 수 있어 해당 이름을 사용하게 되었습니다.
@@ -10,22 +10,24 @@ | |||
import org.springframework.jdbc.core.simple.SimpleJdbcInsert; | |||
import org.springframework.stereotype.Repository; | |||
import roomescape.entity.Reservation; | |||
import roomescape.entity.Time; | |||
import roomescape.exception.DataInvalidException; | |||
|
|||
|
|||
@Repository | |||
public class ReservationJdbcDAO implements ReservationDAO { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전 개인적으로 repository 계층을 인터페이스로 묶는 걸 별로 안 좋아하기 한데 이 부분은 이전에 리뷰어와 충분한 얘기를 나누셨나요? 만약 나눴다면 넘어갈려고 합니다.😊😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
불필요한 코드를 작성할수도 있다, 라는 부분에 동의합니다. 제가 이전에 인터페이스를 사용했던 이유는 이전 mybatis를 사용할때 인터페이스로 기능의 이름들을 추상화 하고 세부 쿼리를 정리해 나갔던 적이 있어 사용하게 되었습니다. 어떻게 보면 한 기능에 들어가는 쿼리와 세부 기능을 보고, 인터페이스로 컨트롤 하는 방법에 익숙해져 있었던 것 같습니다.
보내주신 [코드리뷰 코멘트] (#417 (review)) 를 보면 아직 제가 JPA를 사용해보지 않았기 때문에 그 불편함을 충분히 인지하지 못하고, 아직까지는 인터페이스 사용에 대해 긍정적으로 생각하고 있는 것 같아요. 이 부분에 대해서는 직접 경험해보며 기준을 쌓아가는 과정이 좀 더 필요한 것 같습니다.
private final SimpleJdbcInsert simpleJdbcInsert; | ||
|
||
public ReservationJdbcDAO(JdbcTemplate jdbcTemplate, DataSource dataSource) { | ||
|
||
public ReservationJdbcDAO(JdbcTemplate jdbcTemplate, DataSource dataSource, RowMapper<Reservation> rowMapper) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DataSource를 주입받도록 구현한 이유가 궁금해요!😊😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DataSource는 데이터베이스 커넥션을 획득할 때 사용하는 객체라고 알고있고, JdbcTemplate이 SQL 쿼리를 연결할때 필요로 합니다. 스프링 부트가 데이터베이스 커넥션 정보를 바탕으로 DataSource를 생성하고 스프링 빈으로 만들어두는걸로 알고있어서 이번에 빈을 등록할때 Jdbc에 DataSource를 주입하게 되었습니다. 이미 application 과 build.gradle 에 설정을 다 해놓는데 주입을 받을 이유가 있을까? 에 대해서 생각을 해봤지만, 아직 다른 방법이나 이유를 찾지는 못한 상태입니다 🫨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 알기론 JdbcTemplate가 이미 스프링에서 빈으로 등록되어 있어서 DataSource없이 그냥 JdbcTemplate만 주입받아도 될 것처럼 보여 단 리뷰였습니다!
} | ||
|
||
@Override | ||
public Reservation mapRow(ResultSet rs, int rowNum) throws SQLException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
객체지향 생활체조원칙 줄임말을 사용하지 않는다.😊😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 수정했습니다 😄
|
||
try { | ||
return jdbcTemplate.queryForObject(sql, rowMapper, id); | ||
return jdbcTemplate.queryForObject(sql, new Object[]{id}, (rs, rowNum) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object배열로 감싸준 이유가 궁금해요!😊😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
스키마를 봤을때 Time 객체에는 id 가 포함되어 있어서 바인딩 변수가 여러개라고 생각해 배열 객체를 넣게 되었는데,
수정하는 과정에서 TimeRowMapper 객체를 제대로 활용하지 못하고 있었던 부분이 눈에 보여서, 아래와 같이 수정하게 되었습니다.
private Time getTimeById(Long timeId) {
return jdbcTemplate.queryForObject("SELECT * FROM time WHERE id = ?",
new TimeRowMapper(),
timeId
);
}
@@ -39,7 +36,7 @@ private void validateDate(LocalDate reservation_date) { | |||
} | |||
} | |||
|
|||
private void validateTime(LocalTime reservation_time) { | |||
private void validateTime(Time reservation_time) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reservation_time
네이밍 컨벤션이 잘못된 것 같아요!😊😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 수정했습니다..! 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도메인이나 서비스, 레포지토리에 대한 테스트도 작성해볼까요? 만약 시간이 없으면 도메인만 작성해도 좋습니다!😊😊
@JsonFormat(pattern = "HH:mm") | ||
private final LocalTime time; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도메인에서 뷰영역도 담당하는 것으로 보이네요! 이러면 어떤 장단점이 있나요?😊😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 사실 프론트 영역을 건들지 않은 상태에서 데이터 양식을 맞추는 방법을 찾다보니 @jsonformat(pattern = "HH:mm")을 사용하게 되었습니다.
데이터의 형식을 한 객체 안에서 결정하고 컨트롤 할 수 있다는 점에서 간단한 구조의 도메인에서는 사용이 용이하지만, 이외에 도메인이 뷰영역을 당담하게 된다면 책임이 과중되고, 유연성이 부족해질 것 같습니다. 관련해서는 테스트도 복잡해지는 경우가 생기지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞아요! 그래서 저는 간단한 구조여도 일단 분리하고 보는 편입니다! 하지만 이미 문제점을 인지하고 있는 건 같아 넘어가도 좋을 것 같아요!😊
안녕하세요 비토님! 생각할 수 있는 키워드와 질문들을 주셔서 고민해보며 코멘트에 답글을 달아보았습니다.
위와 같이 말한 이유는 Time 객체를 Reservation 객체와 연결지어 설명하다보니 이러한 표현을 쓰게 된 것 같습니다. JDBC 테스트에서 테스트코드를 통과할때는, 아래와 같이 쿼리를 직접적으로 작성해서 테스트 코드에 주입을 했는데요, jdbcTemplate.update("INSERT INTO time (time) VALUES (?)", "15:40");
Long timeId = jdbcTemplate.queryForObject("SELECT id FROM time WHERE time = ?", new Object[]{"15:40"},
Long.class);
assertThat(timeId).isNotNull();
jdbcTemplate.update("INSERT INTO reservation (name, date, time_id) VALUES (?, ?, ?)", "브라운", "2023-08-05",
timeId); 이전에 실패했던 삼단계와 칠단계의 경우 아래와 같이 직접적으로 Map에 데이터를 넣어 테스트를 진행하고 있는데, 칠단계가 성공한 반면에 삼단계에서는 getAll을 사용해서 전체 예약 조회를 진행하는 과정에서 데이터가 조회되지 않아 어떤 부분이 문제인지 찾고 있습니다 😞 조인 쿼리 문제일까 적용을 해봐도 postman 내에서 get 메서드를 사용하면 스키마 내에서 데이터를 삽입해 둔 상황이면 전체 조회가 되는 상황입니다. (테스트 코드만 실패) Map<String, String> timeParams = new HashMap<>();
timeParams.put("time", "15:40");
Map<String, Object> reservationParams = new HashMap<>();
reservationParams.put("name", "브라운");
reservationParams.put("date", "2023-08-05");
reservationParams.put("time", timeParams); 이외에는
를 더해서 다음 리뷰 요청에 추가해보도록 하겠습니다. 추가질문
감사합니다 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드를 작성하고 지우다보면 코드가 이전단계로 돌아가도 깃 내역에 코드가 변경된 내역이 추가가 되는데요(띄어쓰기나 소소한 부분들),
보통 이 경우에는 커밋 내역에 파일을 포함시키시나요?
네, 어떤 사람은 그런건 style로 해서 따로 커밋하는 사람도 있는데 저는 팀컨벤션으로 강제하지 않는한 그렇게까지 세부적일 필요는 없다고 생각해서요!
코드에 질문을 주시는 기준이 궁금합니다
이번에 키워드에 대해서 질문을 주실때 정확히 이 기술을 알고 사용하는지에 대한 고찰을 많이 하게 된 것 같아요.ㅍDataSource의 주입 등, 매체나 강의를 보고 무의식적으로 사용한게 아닐까 되물어보는 방향성도 많았는데, 아무래도 부족한 부분이 많은 코드라 질문을 주시는 기준들이 궁금해서 질문드립니다.
저는 딱히 기준은 없습니다. 그냥 쭉 읽어보면서 나와 코드 기준이 다른 부분, 일관성이 없는 부분, 뭔가 불필요한 로직이지 않나 싶은 부분 위주로 질문하는 것 같아요!
|
||
private final DataSource dataSource; | ||
|
||
public AppConfig(DataSource dataSource) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 클래스를 만든 이유가 궁금해요! 제가 보기에 여기서 빈으로 만든 클래스들은 전부 생성자 주입이 가능하거나 Component 어노테이션을 사용해도 될 것 같아서요.😊
} | ||
|
||
@PostMapping | ||
public ResponseEntity<Reservation> createReservation(@RequestBody Reservation reservation) { | ||
|
||
Reservation newReservation = reservationJdbcDAO.create(reservation); | ||
Reservation newReservation = reservationService.createReservation(reservation); | ||
return ResponseEntity.created(URI.create("/reservations/" + newReservation.getId())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상태코드는 ResponseStatus를 활용할 수 있고 응답본문도 ResponseEntity를 사용하지 않아도 반환이 가능하지 않나요?🤔
import roomescape.service.TimeService; | ||
|
||
@RestController | ||
@RequestMapping("/times") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전 개인적으로 이렇게 클래스 상단에서 RequestMapping로 묶어주는 걸 지양하는 편이지만 이것도 도요님이 경험으로 뭐가 더 좋을 지 판단해도 좋을 것 같아요😊
private final SimpleJdbcInsert simpleJdbcInsert; | ||
|
||
public ReservationJdbcDAO(JdbcTemplate jdbcTemplate, DataSource dataSource) { | ||
|
||
public ReservationJdbcDAO(JdbcTemplate jdbcTemplate, DataSource dataSource, RowMapper<Reservation> rowMapper) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 알기론 JdbcTemplate가 이미 스프링에서 빈으로 등록되어 있어서 DataSource없이 그냥 JdbcTemplate만 주입받아도 될 것처럼 보여 단 리뷰였습니다!
t.id as time_id, | ||
t.time as time_value | ||
FROM reservation as r | ||
INNER JOIN time as t ON r.time_id = t.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그냥 JOIN을 사용하면 기본적으로 INNER JOIN을 사용하는데 INNER를 명시해준 이유가 궁금해요!😊
} | ||
|
||
@Override | ||
public Time getById(long id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기는 reservationDAO와 달리 예외처리를 안해준 이유가 있나요?😊
|
||
public Reservation(long id, String name, LocalDate date, LocalTime time) { | ||
public Reservation(long id, String name, LocalDate date, Time time) { | ||
validateName(name); | ||
validateDate(date); | ||
validateTime(time); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 리뷰는 도요님의 생각을 듣고싶어서 적은 리뷰이니 편하게 답변해주세요!
지금 도요님이 처리해주는 예외는 크게 2가지가 있는 것 같아요!
- 클라이언트에게 값을 전달받았는지(클라이언트가 값을 넘겨줬는지 null이나 빈값을 넘겨줬는지)
- 클라이언트가 넘겨준 값이 비즈니스 로직에 위배되지 않는지
각각의 예외처리 로직을 어디에 위치시키는지 그 기준이 궁금합니다!😊
Long timeId1 = jdbcTemplate.queryForObject("SELECT id FROM time WHERE time = ?", new Object[]{"10:00"}, | ||
Long.class); | ||
assertThat(timeId1).isNotNull(); | ||
Long timeId2 = jdbcTemplate.queryForObject("SELECT id FROM time WHERE time = ?", new Object[]{"11:00"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트에서도 Object배열을 제거해 볼까요? 지금 Object배열을 사용하는 방법은 JdbcTemplate에서 deprecated한 메소드라서 사용하지 않는 걸 권장드려요😊
@@ -39,8 +36,8 @@ private void validateDate(LocalDate reservation_date) { | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 네이밍 컨벤션이 안 맞는 건 같아요!😊
@JsonFormat(pattern = "HH:mm") | ||
private final LocalTime time; | ||
|
||
public Time(long id, LocalTime time) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기선 LocalTime에 대한 validate가 없는 이유가 있나요?😊
안녕하세요 비토님! 도요입니다. 2주간 잘 부탁드립니다.
구현기능
입력시의 예외처리를 진행하지 못해 추후 추가하도록 하겠습니다 😢
진행하면서 고려했던 점과 질문