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] 김서현 미션 제출합니다. #277

Open
wants to merge 3 commits into
base: seobbang
Choose a base branch
from

Conversation

seobbang
Copy link

Spring Core 8,9단계 미션 제출합니다.

🚀 8단계 - 시간 관리 기능

  • 시간 조회, 추가, 삭제 API 구현

🚀 9단계 - 기존 코드 수정

  • 바뀐 명세에 따라 예약 조회, 추가 API 수정

따로 주신 예약 조회, 추가 명세에 맞게 구현했습니다!

Copy link

@kyY00n kyY00n left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 서현님!! 제가 조금 늦었는데요..!
보시고 궁금한게 있으면 언제든 질문주세요~~

우선순위는,

  1. 테스트가 전부 통과하는지
  2. 애플리케이션 실행해봤을 때 잘 동작하는지 (localhost:8080)
  3. 이외의 코멘트 반영이 되었는지

이렇게 두시면 좋을 것 같습니다!! 파이팅 :+1


@PostMapping("/times")
public ResponseEntity<ResponseDto> create (@RequestBody TimeRequestDto request) {

Copy link

Choose a reason for hiding this comment

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

시간 생성 API 응답 스펙을 보고 리턴 타입을 고쳐주시길 바랍니다!

}else {
throw new NotFoundReservationException("Time with id " + id + " not found." );
}
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
}
if (rowsAffected == 0) {
throw new NotFoundReservationException();
}
return ResponseEntity.noContent().build();

그냥 생각나서 적어보는 것인데요 저는 보통 else 를 쓰지 않는 것이 가독성이 좋다고 느껴지더라구요.
+) 그리고 미션 제출 전 포맷팅 한 번씩 해주세요~


@Controller
public class TimeController {
private TimeQueryingDAO queryingDAO;
Copy link

Choose a reason for hiding this comment

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

멤버변수에 final 키워드를 적용했을 때의 장점을 찾아보고 적용해볼까요?

// this.date = date;
// this.time = time;
// }

Copy link

Choose a reason for hiding this comment

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

사용하지 않는 코드는 지워주세요!

// this.date = date;
// this.time = time;
// }

Copy link

Choose a reason for hiding this comment

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

마찬가지!

public ResponseEntity<ResponseDto> create (@RequestBody TimeRequestDto request) {

String time = request.getTime();
long id = queryingDAO.createTime(time);
Copy link

Choose a reason for hiding this comment

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

DAO에서 Time을 반환하는 것이 자연스럽지 않을까요? 그렇지 않으면 DAO를 사용하는 코드에서 항상 Time 객체를 손수 만들어줘야해요

@@ -34,13 +34,12 @@ public ResponseEntity<List<Reservation>> read() {
public ResponseEntity<ResponseDto> create (@RequestBody ReservationRequestDto request) {
Copy link

Choose a reason for hiding this comment

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

여기도 마찬가지! API 스펙을 보고 시그니처를 고쳐주세요.

@@ -34,13 +34,12 @@ public ResponseEntity<List<Reservation>> read() {
public ResponseEntity<ResponseDto> create (@RequestBody ReservationRequestDto request) {
String name = request.getName();
String date = request.getDate();
String time = request.getTime();

Long time = request.getTime();

long id = queryingDAO.createReservation(name, date, time);
Copy link

Choose a reason for hiding this comment

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

DAO 에서 create 연산 시 reservation을 반환하는 것이 자연스러울 것 같아요.

(TimeController에서는 불편을 못느꼈겠지만) 여기서는 Reservation 객체, 그 안의 Time 객체를 직접 생성해서 반환하는 것이 불편하다는 것을 아실 수 있을거예요.

KeyHolder keyHolder = new GeneratedKeyHolder();
jdbcTemplate.update(connection -> {
PreparedStatement ps = connection.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS);
ps.setString(1, name);
ps.setString(2, date);
ps.setString(3, time);
ps.setObject(3, time);
Copy link

Choose a reason for hiding this comment

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

데이터베이스 스키마를 보면 time_id 는 BIGINT 타입이죠, 그럼 java의 Long 으로 사용할 수 있습니당. 제 생각에는 별도로 변환이 필요하지 않은 setLong 메서드를 사용하는 것이 더 효율적일 것 같습니다. 또한 타입을 명시하면 이 코드만으로도 데이터베이스 스키마를 유추할 수 있게 되는 건 덤일 것 같습니다.

this.queryingDAO = queryingDAO;
}
@GetMapping("/time")
public void time () {
Copy link

Choose a reason for hiding this comment

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

위에도 코멘트 드렸는데 연결을 위해서는 시그니처와 반환값을 변경해주셔야합니다! (thymeleaf)

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